[FLINK-37132][transform] validate meta schema in Multi Transform #3865
Conversation
|
@yuxiqian Could you help review the code? |
| } | ||
| } | ||
|
|
||
| public static void isMetaSchemaCompatible( |
There was a problem hiding this comment.
nit: this is not a pure function returning Boolean, so validateSchemaMetadataCompatibility might be a better name.
| @@ -452,6 +455,43 @@ public static Schema inferWiderSchema(@Nullable Schema lSchema, Schema rSchema) | |||
| return lSchema.copy(mergedColumns); | |||
| } | |||
|
|
|||
| public static void validateMetaSchemaCompatibility(LinkedHashSet<Schema> schemas) { | |||
There was a problem hiding this comment.
I wonder if is this method really reusable enough to be a utility method?
Seems the argument type is quite specific (it must be a LinkedHashSet instead of a generic schemas' collection). Also, it didn't return a "Yes or No" boolean and let the caller to decide what to do, but throws an exception if it's incompatible.
There was a problem hiding this comment.
[but throws an exception if it's incompatible]
yeal,I think this kind of check is mandatory and cannot be ignored, and the exception is fatal, so I designed it to return void, but returning boolean seems to be good too. What do you think?
There was a problem hiding this comment.
My concern is if we're throwing an exception in the utility method, it might be less universal. But your implementation is more reasonable, because we can't return any reason message to indicate the incompatible reason by returning a boolean.
Let's just keep it as is, with just a few changes:
- throwing a checked exception would be better and clearer for any method callers
- change
LinkedHashSetto more genericCollection
There was a problem hiding this comment.
I have updated the code, could you please help review it again,thanks~
| @@ -1116,6 +1117,27 @@ void testGetLeastCommonType() { | |||
| MAP)); | |||
| } | |||
|
|
|||
| @Test | |||
| void testTransformColumn() { | |||
| Assertions.assertThatCode( | |||
There was a problem hiding this comment.
We may use assertThatThrownBy as we're surely expecting an error
| return null; | ||
| } else if (schemas.size() == 1) { | ||
| return schemas.get(0); | ||
| public static Schema getCommonSchema(LinkedHashSet<Schema> schemas) { |
There was a problem hiding this comment.
This is a breaking API change since the parameter type constraints are getting stricter. I think changing List => Collection is acceptable since it's getting wider.
MOBIN-F
force-pushed
the
release-FLINK-37132
branch
from
cb2de5d to
866f0ab
Compare
In Multi Transform, verify that column counts, metadata fields like primaryKeys, partitionKeys, and options are consistent