RussellSpitzer commented on code in PR #13835:
URL: https://github.com/apache/iceberg/pull/13835#discussion_r2283587270
##########
api/src/main/java/org/apache/iceberg/PartitionSpec.java:
##########
@@ -388,26 +388,23 @@ private int nextFieldId() {
return lastAssignedFieldId.incrementAndGet();
}
- private void checkAndAddPartitionName(String name) {
- checkAndAddPartitionName(name, null);
- }
-
Builder checkConflicts(boolean check) {
checkConflicts = check;
return this;
}
- private void checkAndAddPartitionName(String name, Integer sourceColumnId)
{
+ private void checkAndAddPartitionName(
Review Comment:
This is a personal style thing I have, but I would avoid functions whose
behavior changes based on the input arguements. Although this looks like we are
taking in the transform, we are actually just essentially using it as a boolean.
checkAndAddPartitionName(..., Boolean allowNameConflict)
I dislike this kind of call when we probably should just have a completely
different function for those transforms which don't allow the conflict.
So currently (and in the patch) the method is
If (check conflicts)
if (transform is identity or void)
check that the name is not a duplicate or matches the same name as
the source of the transform
else
check that the name isn't a duplicate
check name isn't empty
check name isn't already used as a partition
This is just me but I feel like the fact that we have a special case in the
middle of this method has always rubbed me wrong.
I would rather we have two versions
checkPartitionNameUniqueAndAdd(String name)
checkPartitionNameUniqueOrIdenticalAndAdd(String name, Integer sourceId)
That way we wouldn't be hiding the fact that identity and void have this
weird behavior
I also probably would take the "partitionNames.add" out of this method too,
not a big fan of the side-effect here either.
These are all my personal style issues here but since we are refactoring
anyway I think it's probably worth cleaning up
--
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]
