Flink: Clean up UpdateSchema instantiator method#13640
Conversation
| () -> | ||
| EvolveSchemaVisitor.visit( | ||
| loadUpdateApi(currentSchema, 3), currentSchema, targetSchema)) | ||
| loadUpdateApi(currentSchema), currentSchema, targetSchema)) |
There was a problem hiding this comment.
Actually this is not quite right, in testReplaceListWithPrimitive lastColumnId is 3, but schema.highestFieldId() is 2.
There was a problem hiding this comment.
That observation is correct, but it makes no difference for the correctness of the test. The columns are matched by name, not by id. Before we even add the column, where the lastColumnId would come into place for generating a new field id, the test will already fail with the expected error message.
Generally, any value for lastColumnId >= the highest id (2 here) will work (the ids must not overlap). Note that the SchemaUpdate code will increase the provided value by 1 before using it as an id for a new field.
There was a problem hiding this comment.
Okay, wondering though that was is accidental, that a bigger id than the highest was used here, or it was intentional. I suppose it is the former.
There was a problem hiding this comment.
Yes, good attention to detail. The refactoring is precisely to prevent setting the wrong id. 3 wasn't wrong, but it could have been 2 as well (the max current field id). At runtime, SchemaUpdate will set this id automatically, but we are using the test constructor here.
|
Merged to main. |
…od in TestEvolveSchemaVisitor
3 participants
Currently, we need to supply the highest field id when instantiating the SchemaUpdate constructor. This can be automated.