You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/01/14 08:38:41 UTC
[GitHub] [iceberg] jun-he opened a new pull request #2089: add lastAssignedFieldId to table metadata
jun-he opened a new pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089
Follow up https://github.com/apache/iceberg/pull/2083.
Add lastAssignedFieldId to table metadata.
To keep it simple, lastAssignedFieldId across all partition specs will be computed at initialization without persisting to the JSON file.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jun-he commented on pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
jun-he commented on pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#issuecomment-768086489
@rdblue can you please take another look? thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#discussion_r567323414
##########
File path: core/src/test/resources/TableMetadataV2MissingLastAssignedPartitionId.json
##########
@@ -0,0 +1,71 @@
+{
+ "format-version": 2,
+ "table-uuid": "9c12d441-03fe-4693-9a96-a0705ddf69c1",
+ "location": "s3://bucket/test/location",
+ "last-sequence-number": 34,
+ "last-updated-ms": 1602638573590,
+ "last-column-id": 3,
Review comment:
I just realized that this is `last-column-id` and not `last-assigned-column-id`. I think we should make the metadata property `last-partition-id` instead of `last-assigned-partition-id` to match. I'll merge this and we can fix it in a follow-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#discussion_r567154516
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -708,15 +716,16 @@ public TableMetadata buildReplacement(Schema updatedSchema, PartitionSpec update
return new TableMetadata(null, formatVersion, uuid, newLocation,
lastSequenceNumber, System.currentTimeMillis(), newLastColumnId.get(), freshSchema,
- specId, specListBuilder.build(), orderId, sortOrdersBuilder.build(), ImmutableMap.copyOf(newProperties),
+ specId, specListBuilder.build(), Math.max(lastAssignedFieldId, freshSpec.lastAssignedFieldId()),
Review comment:
I think that we need to fix the implementation of `freshSpec` because it doesn't accept a `lastAssignedFieldId` or reuse existing partition field IDs. In fact, I think what would currently happen is that the spec would be created independent of the rest of the table metadata and could reuse existing IDs. That's not good, but we also don't want to reassign all of the IDs (though that would be less bad).
We need to create a new spec similar to how we use `assignFreshIds` for the schema, which will reuse IDs for existing fields and only assign new IDs for new fields, starting at the last assigned ID.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jun-he commented on pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
jun-he commented on pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#issuecomment-761523539
@rdblue Thanks for the comment.
It makes sense to add the lastAssignedFieldId to metadata JSON when specs in metadata might be removed.
We already keep the lastAssignedFieldId in the spec JSON. I will add lastAssignedFieldId to table metadata JSON as well.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jun-he commented on a change in pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#discussion_r567277467
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -371,6 +374,10 @@ public PartitionSpec spec(int id) {
return specsById;
}
+ int lastAssignedFieldId() {
Review comment:
Updated the field name in `TableMetadata`.
In `PartitionSpec`, there is also a `lastAssignedFieldId` field. It seems to be fine as it is a field inside partition spec, which already indicates it is about partition.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#issuecomment-761183855
The last assigned partition field ID does need to be added to the spec and the table metadata JSON. It is possible to reuse IDs, but I don't think it is a good idea to do it. There is also no need to reuse them. I think that reuse could only cause problems.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jun-he commented on pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
jun-he commented on pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#issuecomment-760668525
@yyanyy Thanks for the review.
I thought about adding it to metadata JSON file. It seems unnecessary and also adds additional complexity for backward compatibility and maintains its correctness. As the info is already included in specs, we can simply recompute it during initialization.
It seems table metadata is a better place to hold this info than partition spec updater as it is an info for a table, which might be used by other code paths in the future.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#discussion_r567152609
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -91,6 +92,7 @@ private TableMetadataParser() {
static final String PARTITION_SPEC = "partition-spec";
static final String PARTITION_SPECS = "partition-specs";
static final String DEFAULT_SPEC_ID = "default-spec-id";
+ static final String LAST_ASSIGNED_FIELD_ID = "last-assigned-field-Id";
Review comment:
I think this should be `last-assigned-partition-id`.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jun-he commented on pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
jun-he commented on pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#issuecomment-766305987
Will update the doc after the change is merged.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#issuecomment-761184851
I just thought of an example where ID reuse could be bad. The Nessie catalog allows branching at the table metadata level by keeping different metadata file references. A Nessie branch could remove data for one spec and reuse the IDs, which would make compatibility with another branch using the old spec confusing.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#discussion_r567146441
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -371,6 +374,10 @@ public PartitionSpec spec(int id) {
return specsById;
}
+ int lastAssignedFieldId() {
Review comment:
I think this name is potentially confusing. It is the last assigned partition field ID, and "partition" is a key part of it. How about renaming it to `lastAssignedPartitionId` instead?
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -371,6 +374,10 @@ public PartitionSpec spec(int id) {
return specsById;
}
+ int lastAssignedFieldId() {
Review comment:
I think this name is potentially confusing. It is the last assigned partition field ID, and "partition" is a key part of it. How about renaming it to `lastAssignedPartitionId` instead? Same with the instance field.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#issuecomment-770110233
Thank you, @jun-he! I think there are few main improvements needed:
1. We should use `lastAssignedPartitionId` instead of `lastAssignedFieldId` in all cases to be more specific
2. For a table replace, we need to reassign the partition field IDs and reuse any existing IDs for equivalent fields
3. We need to update the spec doc
We can do all 3 in this PR, or just the first one if you prefer to separate them. Just let me know.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#discussion_r567152525
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -288,6 +292,14 @@ static TableMetadata fromJson(FileIO io, InputFile file, JsonNode node) {
schema, TableMetadata.INITIAL_SPEC_ID, node.get(PARTITION_SPEC)));
}
+ int lastAssignedFieldId;
+ if (formatVersion > 1) {
+ lastAssignedFieldId = JsonUtil.getInt(LAST_ASSIGNED_FIELD_ID, node);
Review comment:
Should this use a precondition to check that the last assigned partition ID is present? That would result in a better error message if it is missing, like the other cases: `last-assigned-partition-id must exist in format v2`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] yyanyy commented on a change in pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
yyanyy commented on a change in pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#discussion_r560516987
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -288,6 +292,9 @@ static TableMetadata fromJson(FileIO io, InputFile file, JsonNode node) {
schema, TableMetadata.INITIAL_SPEC_ID, node.get(PARTITION_SPEC)));
}
+ int lastAssignedFieldId = Optional.ofNullable(JsonUtil.getIntOrNull(LAST_ASSIGNED_FIELD_ID, node))
+ .orElseGet(() -> specs.stream().mapToInt(PartitionSpec::lastAssignedFieldId).max().orElse(999));
Review comment:
Do we want it to be a required field in v2 and fail validation if it's missing?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jun-he commented on a change in pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#discussion_r561458394
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -288,6 +292,9 @@ static TableMetadata fromJson(FileIO io, InputFile file, JsonNode node) {
schema, TableMetadata.INITIAL_SPEC_ID, node.get(PARTITION_SPEC)));
}
+ int lastAssignedFieldId = Optional.ofNullable(JsonUtil.getIntOrNull(LAST_ASSIGNED_FIELD_ID, node))
+ .orElseGet(() -> specs.stream().mapToInt(PartitionSpec::lastAssignedFieldId).max().orElse(999));
Review comment:
It makes sense as the V2 is not released yet. I will update it accordingly.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jun-he commented on pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
jun-he commented on pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#issuecomment-770245760
@rdblue thanks for the comments. Updated the PR with the renamed field. I will send separate PRs for item 2 and 3. Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#issuecomment-770285720
Thanks, @jun-he!
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jun-he commented on a change in pull request #2089: add lastAssignedFieldId to table metadata
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #2089:
URL: https://github.com/apache/iceberg/pull/2089#discussion_r561458394
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -288,6 +292,9 @@ static TableMetadata fromJson(FileIO io, InputFile file, JsonNode node) {
schema, TableMetadata.INITIAL_SPEC_ID, node.get(PARTITION_SPEC)));
}
+ int lastAssignedFieldId = Optional.ofNullable(JsonUtil.getIntOrNull(LAST_ASSIGNED_FIELD_ID, node))
+ .orElseGet(() -> specs.stream().mapToInt(PartitionSpec::lastAssignedFieldId).max().orElse(999));
Review comment:
It makes sense as the V2 is not released yet. I will update it accordingly.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org