Core: Fix move/update/makeRequire/makeOptional fail after rename schema (#10830) #12202
Conversation
| @@ -240,7 +240,10 @@ public UpdateSchema makeColumnOptional(String name) { | |||
| } | |||
|
|
|||
| private void internalUpdateColumnRequirement(String name, boolean isOptional) { | |||
| Types.NestedField field = findField(name); | |||
| Types.NestedField field = findFieldFromUpdate(name); | |||
| if (field == null) { | |||
There was a problem hiding this comment.
this would be better handled in findFieldFromUpdate.
There was a problem hiding this comment.
Thanks for code review! I hope findFieldFromUpdate will find field from updates only, but add a new method do findFieldFromUpdate first and then findField if findFieldFromUpdate return null will be better, because there are duplicated codes, is that means?
There was a problem hiding this comment.
There was a problem hiding this comment.
You may want to check https://github.com/apache/iceberg/pull/12211/files#diff-a642a12eac1cb0d3f5bf883427d2971025300182e21b630e5536e54a9fbd8bcaR402
Finished! I have reformed the code
| List<Types.NestedField> updatedFields = | ||
| updates.values().stream().filter(f -> f.name().equals(name)).collect(Collectors.toList()); |
There was a problem hiding this comment.
nit: The size of updatedFields should be 0 or 1, right? How about verifying the size with Preconditions.checkArgument method?
There was a problem hiding this comment.
nit: The size of
updatedFieldsshould be 0 or 1, right? How about verifying the size withPreconditions.checkArgumentmethod?
Thanks for review!
I have thought about the size of updatedFields before, as you say it should be 0 or 1, but if your call renameColumn for different column to a same name , the size will greater than 1, but validation has been set in applyChanges, so I don't add a Preconditions.checkArgument
Should I add a Preconditions.checkArgument? Add would be better, validate violation at method call
Some SchemaUpdate API will fail after renameColumn including updateColumn, updateColumnDoc, requireColumn, makeColumnOptional, moveFirst, moveBefore and moveAfter. We don't fix every api after rename because some can be avoid by appropriate API restructuring, for example, renameColumn after renameColumn can be simplified to one rename, deleteColumn after renameColumn can be simplified to delete only because the field no need any more