kbendick commented on a change in pull request #3845:
URL: https://github.com/apache/iceberg/pull/3845#discussion_r778373438
##########
File path: api/src/main/java/org/apache/iceberg/PartitionSpec.java
##########
@@ -337,16 +337,22 @@ private void checkAndAddPartitionName(String name) {
}
private void checkAndAddPartitionName(String name, Integer sourceColumnId)
{
+ checkAndAddPartitionName(name, sourceColumnId, true);
Review comment:
Nit: If we leave it as a boolean argument, please add an in-line comment
indicating what it is, like `true /* checkConflict */)`.
That can be done at the end though 😄
##########
File path: api/src/main/java/org/apache/iceberg/PartitionSpec.java
##########
@@ -337,16 +337,22 @@ private void checkAndAddPartitionName(String name) {
}
private void checkAndAddPartitionName(String name, Integer sourceColumnId)
{
+ checkAndAddPartitionName(name, sourceColumnId, true);
+ }
+
+ private void checkAndAddPartitionName(String name, Integer sourceColumnId,
boolean checkConflict) {
Types.NestedField schemaField = schema.findField(name);
- if (sourceColumnId != null) {
- // for identity transform case we allow conflicts between partition
and schema field name as
- // long as they are sourced from the same schema field
- Preconditions.checkArgument(schemaField == null ||
schemaField.fieldId() == sourceColumnId,
- "Cannot create identity partition sourced from different field in
schema: %s", name);
- } else {
- // for all other transforms we don't allow conflicts between partition
name and schema field name
- Preconditions.checkArgument(schemaField == null,
- "Cannot create partition from name that exists in schema: %s",
name);
+ if (checkConflict) {
+ if (sourceColumnId != null) {
+ // for identity transform case we allow conflicts between partition
and schema field name as
+ // long as they are sourced from the same schema field
+ Preconditions.checkArgument(schemaField == null ||
schemaField.fieldId() == sourceColumnId,
+ "Cannot create identity partition sourced from different
field in schema: %s", name);
+ } else {
+ // for all other transforms we don't allow conflicts between
partition name and schema field name
+ Preconditions.checkArgument(schemaField == null,
+ "Cannot create partition from name that exists in schema:
%s", name);
Review comment:
Nit: Over-indented (can be fixed before merging).
##########
File path: api/src/main/java/org/apache/iceberg/PartitionSpec.java
##########
@@ -337,16 +337,22 @@ private void checkAndAddPartitionName(String name) {
}
private void checkAndAddPartitionName(String name, Integer sourceColumnId)
{
+ checkAndAddPartitionName(name, sourceColumnId, true);
+ }
+
+ private void checkAndAddPartitionName(String name, Integer sourceColumnId,
boolean checkConflict) {
Types.NestedField schemaField = schema.findField(name);
- if (sourceColumnId != null) {
- // for identity transform case we allow conflicts between partition
and schema field name as
- // long as they are sourced from the same schema field
- Preconditions.checkArgument(schemaField == null ||
schemaField.fieldId() == sourceColumnId,
- "Cannot create identity partition sourced from different field in
schema: %s", name);
- } else {
- // for all other transforms we don't allow conflicts between partition
name and schema field name
- Preconditions.checkArgument(schemaField == null,
- "Cannot create partition from name that exists in schema: %s",
name);
+ if (checkConflict) {
Review comment:
I do agree that it might make sense to add to the builder. However,
since it's a pretty specific occurrence (and really only applies to `identity`)
would that be possibly confusing to users about when it should be called and
what it would be applied to?
It seems that `BaseMetadataTable#transformSpec` is the only place that uses
`identity` with this boolean argument as false. Is it possible to get rid of
that and then `checkAndAddPartitionName` could just have some sort of check for
whether or not this is `identity` transform?
--
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]
