rdblue commented on a change in pull request #3632:
URL: https://github.com/apache/iceberg/pull/3632#discussion_r767338036
##########
File path: core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java
##########
@@ -205,6 +209,10 @@ public BaseUpdatePartitionSpec removeField(Term term) {
@Override
public BaseUpdatePartitionSpec renameField(String name, String newName) {
+ return renameFieldInternal(name, newName, false);
+ }
+
+ private BaseUpdatePartitionSpec renameFieldInternal(String name, String
newName, boolean allowDeleteFirst) {
Review comment:
I don't think that there is a need for this internal version. The
`addField` method should create this rename directly using
`renames.put(existingField.name(), existingField.name() + "_" +
existingField.fieldId());`
This method checks whether the new name references a deleted field and
renames the deleted field if it is. I don't think that is needed here because
we know this field is newly deleted. Next, it checks that the field has not
been added. We know that it isn't a newly added field because the current
method is `addField` and we can't add more than one. Next, it looks up the
field to rename and makes sure it exists. But we already know it exists because
that's what `existingField` is. Finally, it this would skip the final delete
check. So we know that all of those checks are going to succeed from here and
it is safe to add the rename directly.
That's a lot cleaner because then we don't need an extra method that takes a
boolean argument.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
