rdblue commented on a change in pull request #499: Add persistent IDs to
partition fields (WIP)
URL: https://github.com/apache/incubator-iceberg/pull/499#discussion_r361009104
##########
File path: core/src/main/java/org/apache/iceberg/PartitionSpecParser.java
##########
@@ -146,8 +150,13 @@ private static void
buildFromJsonFields(PartitionSpec.Builder builder, JsonNode
String name = JsonUtil.getString(NAME, element);
String transform = JsonUtil.getString(TRANSFORM, element);
int sourceId = JsonUtil.getInt(SOURCE_ID, element);
-
- builder.add(sourceId, name, transform);
+ // to handle the backward compatibility where partitionFieldId was not
part of the partitionSpec schema.
+ if (element.has(FIELD_ID)) {
+ partitionFieldId = JsonUtil.getInt(FIELD_ID, element);
+ } else {
+ partitionFieldId = partitionFieldId + 1;
+ }
+ builder.add(sourceId, partitionFieldId, name, transform);
Review comment:
It seems odd that `partitionFieldId` is incremented from the last value when
missing. If a spec has one missing field ID, then it will be assigned based on
the previous field's ID. I don't think this would cause problems because we
expect either all fields to have assigned IDs, or no fields to have them.
I'd prefer to keep the logic for those cases separate to make this easier to
follow. It isn't a good practice to rely on a hidden assumption that either all
fields have ids or none do.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
