add lastAssignedFieldId to table metadata #2089

jun-he · 2021-01-14T08:38:27Z

Follow up #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.

yyanyy

Thanks for making the change! LGTM.

One question: do you intend to add lastAssignedFieldId to the JSON file in another PR, or you don't think it's necessary (so that this change is mostly to make lastAssignedFieldId available in table metadata class)? I thought we want to persist it in JSON file, and if that's the case, I'll echo my question from this PR:

Is this just to ensure implementation correctness per spec? As it seems that from the current implementation getting this from PartitionSpec::lastAssignedFieldId should be good enough, as there's no table operation to actually remove a partition field from specs?

jun-he · 2021-01-15T06:03:07Z

@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.

rdblue · 2021-01-15T20:35:21Z

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.

rdblue · 2021-01-15T20:37:31Z

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.

jun-he · 2021-01-16T07:50:04Z

@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.

yyanyy

LGTM, just one comment

yyanyy · 2021-01-19T21:45:15Z

core/src/main/java/org/apache/iceberg/TableMetadataParser.java

    }

+    int lastAssignedFieldId = Optional.ofNullable(JsonUtil.getIntOrNull(LAST_ASSIGNED_FIELD_ID, node))
+        .orElseGet(() -> specs.stream().mapToInt(PartitionSpec::lastAssignedFieldId).max().orElse(999));


Do we want it to be a required field in v2 and fail validation if it's missing?

It makes sense as the V2 is not released yet. I will update it accordingly.

jun-he · 2021-01-24T07:37:52Z

Will update the doc after the change is merged.

jun-he · 2021-01-27T07:15:10Z

@rdblue can you please take another look? thanks.

rdblue · 2021-01-29T23:20:59Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

    return specsById;
  }

+  int lastAssignedFieldId() {


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.

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.

rdblue · 2021-01-29T23:42:42Z

core/src/main/java/org/apache/iceberg/TableMetadataParser.java


+    int lastAssignedFieldId;
+    if (formatVersion > 1) {
+      lastAssignedFieldId = JsonUtil.getInt(LAST_ASSIGNED_FIELD_ID, node);


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

rdblue · 2021-01-29T23:43:04Z

core/src/main/java/org/apache/iceberg/TableMetadataParser.java

  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";


I think this should be last-assigned-partition-id.

rdblue · 2021-01-29T23:50:30Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

    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()),


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.

rdblue · 2021-01-29T23:53:18Z

Thank you, @jun-he! I think there are few main improvements needed:

We should use lastAssignedPartitionId instead of lastAssignedFieldId in all cases to be more specific
For a table replace, we need to reassign the partition field IDs and reuse any existing IDs for equivalent fields
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.

jun-he · 2021-01-30T17:13:03Z

@rdblue thanks for the comments. Updated the PR with the renamed field. I will send separate PRs for item 2 and 3. Thanks.

rdblue · 2021-01-30T21:48:16Z

core/src/test/resources/TableMetadataV2MissingLastAssignedPartitionId.json

+  "location": "s3://bucket/test/location",
+  "last-sequence-number": 34,
+  "last-updated-ms": 1602638573590,
+  "last-column-id": 3,


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.

rdblue · 2021-01-30T21:49:00Z

Thanks, @jun-he!

add lastAssignedFieldId to table metadata.

61abe19

github-actions bot added the core label Jan 14, 2021

yyanyy approved these changes Jan 14, 2021

View reviewed changes

yyanyy mentioned this pull request Jan 16, 2021

Core: add schema id and schemas to table metadata #2096

Merged

address the comments

afc646e

yyanyy reviewed Jan 19, 2021

View reviewed changes

Make sure that last assigned field id is required in v2

1a2bf75

yyanyy approved these changes Jan 26, 2021

View reviewed changes

rdblue reviewed Jan 29, 2021

View reviewed changes

rename the id field to lastAssignedPartitionId

d4f0f70

rdblue reviewed Jan 30, 2021

View reviewed changes

rdblue approved these changes Jan 30, 2021

View reviewed changes

rdblue merged commit b3e01b7 into apache:master Jan 30, 2021

jun-he deleted the jun/update-tablemetadata branch January 31, 2021 07:04

This was referenced Jan 31, 2021

Rename last-assigned-partition-id to last-partition-id in table metadata #2188

Merged

add the descriptions for last-partition-id in docs #2224

Merged

jun-he mentioned this pull request Mar 6, 2021

Core: reassign the partition field IDs and reuse any existing IDs #2284

Closed

wypoon mentioned this pull request Apr 3, 2021

Core: add schema id to snapshot #2275

Merged

rdblue mentioned this pull request Aug 1, 2021

Core: Fix partition field IDs in table replacement #2906

Merged

add lastAssignedFieldId to table metadata #2089

add lastAssignedFieldId to table metadata #2089

Uh oh!

Conversation

jun-he commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yyanyy left a comment

Choose a reason for hiding this comment

Uh oh!

jun-he commented Jan 15, 2021

Uh oh!

rdblue commented Jan 15, 2021

Uh oh!

rdblue commented Jan 15, 2021

Uh oh!

jun-he commented Jan 16, 2021

Uh oh!

yyanyy left a comment

Choose a reason for hiding this comment

Uh oh!

yyanyy Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

jun-he Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

jun-he commented Jan 24, 2021

Uh oh!

jun-he commented Jan 27, 2021

Uh oh!

rdblue Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jun-he Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

rdblue Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

rdblue Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

rdblue Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

rdblue commented Jan 29, 2021

Uh oh!

jun-he commented Jan 30, 2021

Uh oh!

rdblue Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

rdblue commented Jan 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jun-he commented Jan 14, 2021 •

edited

Loading

rdblue Jan 29, 2021 •

edited

Loading