laskoviymishka commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3966674061
##########
table/metadata.go:
##########
@@ -2053,24 +2058,27 @@ func assignMissingPartitionFieldIDsFromMetadata(b
[]byte, metadata map[string]js
return nil, err
}
field["field-id"] = rawFieldID
+ normalizedLastPartitionID = max(normalizedLastPartitionID,
lastAssignedID)
}
- if usesSpecList {
- rawSpecs, err := json.Marshal(specs)
- if err != nil {
- return nil, err
- }
- metadata["partition-specs"] = rawSpecs
- } else {
- rawFields, err := json.Marshal(specs[0].Fields)
- if err != nil {
- return nil, err
+ if len(missingFields) > 0 {
+ if usesSpecList {
+ rawSpecs, err := json.Marshal(specs)
+ if err != nil {
+ return nil, err
+ }
+ metadata["partition-specs"] = rawSpecs
+ } else {
+ rawFields, err := json.Marshal(specs[0].Fields)
+ if err != nil {
+ return nil, err
+ }
+ metadata["partition-spec"] = rawFields
}
- metadata["partition-spec"] = rawFields
}
if lastPartitionIDSet {
- rawLastPartitionID, err := json.Marshal(lastAssignedID)
+ rawLastPartitionID, err :=
json.Marshal(normalizedLastPartitionID)
Review Comment:
I think doing the repair at read time is the wrong layer here. Once we
rewrite last-partition-id from 999 to 1000, Metadata().LastPartitionSpecID()
returns 1000, and UpdateSpec.BuildUpdates (update_spec.go:190) feeds that
straight into AssertLastAssignedPartitionID. Against a REST/Java catalog that
still persists 999, that assertion is a guaranteed false 409, and because every
reload re-normalizes the same bytes, it never clears. So for the exact
stale-counter table this targets, spec evolution goes from silently allocating
a colliding id to permanently un-committable on REST.
Java only recomputes last-partition-id from specs when the field is absent;
present-but-stale is used verbatim, and the allocation floor is applied at
evolution time, not at parse. I'd move the max-field-id scan to the write side:
compute the floor as max(lastPartitionID, max field id across b.specs) in
MetadataBuilder.AddPartitionSpec before binding, and leave the persisted
counter untouched until a new spec actually builds. That keeps
AssertLastAssignedPartitionID(999) matching the catalog, still gives the new
field 1001 with no collision, and lets the server self-heal to 1001 on commit.
wdyt?
##########
table/metadata_preflight_test.go:
##########
@@ -85,6 +86,88 @@ func TestParseMetadataBytesAssignsMissingPartitionFieldIDs(t
*testing.T) {
}
}
+func TestParseMetadataBytesNormalizesStaleLastPartitionID(t *testing.T) {
+ data := strings.Replace(ExampleTableMetadataV2,
+ `"last-partition-id": 1000`, `"last-partition-id": 999`, 1)
+
+ parsed, err := ParseMetadataBytes([]byte(data))
+ require.NoError(t, err)
+ require.NotNil(t, parsed.LastPartitionSpecID())
+ assert.Equal(t, 1000, *parsed.LastPartitionSpecID())
+
+ update := NewUpdateSpec(New(nil, parsed, "", nil,
nil).NewTransaction(), false).
+ AddField("x", iceberg.BucketTransform{NumBuckets: 16},
"x_bucket")
+ _, _, err = update.BuildUpdates()
Review Comment:
This is the piece that gives false confidence on the case above.
BuildUpdates emits AssertLastAssignedPartitionID from parsed, and we then
validate it against that same normalized metadata (1000 == 1000), so it passes
locally, but it never compares the emitted requirement against the original
persisted 999, which is what a REST catalog checks. If we assert that update's
AssertLastAssignedPartitionID requirement equals 999 here, this test goes red
and surfaces the desync.
##########
table/metadata.go:
##########
@@ -2053,24 +2058,27 @@ func assignMissingPartitionFieldIDsFromMetadata(b
[]byte, metadata map[string]js
return nil, err
}
field["field-id"] = rawFieldID
+ normalizedLastPartitionID = max(normalizedLastPartitionID,
lastAssignedID)
Review Comment:
Small thing: this max is always lastAssignedID. Both cursors start from the
same value and after the lastAssignedID++ above it's strictly greater, so the
max can never pick normalizedLastPartitionID. It reads like it's guarding a
case that can't happen. I'd just write normalizedLastPartitionID =
lastAssignedID. Same idea for the field-scan update up at line 2045: it's dead
when lastPartitionIDSet is false, so it could be gated on that. Not blocking.
##########
table/metadata_preflight_test.go:
##########
@@ -85,6 +86,88 @@ func TestParseMetadataBytesAssignsMissingPartitionFieldIDs(t
*testing.T) {
}
}
+func TestParseMetadataBytesNormalizesStaleLastPartitionID(t *testing.T) {
+ data := strings.Replace(ExampleTableMetadataV2,
Review Comment:
This only exercises the fix if the Replace actually lands. If the fixture
spacing ever changes and the replace silently no-ops, last-partition-id stays
1000 and both assertions pass without touching the fix. I'd add a
require.Contains right after, asserting the "last-partition-id": 999 substring
is present in data, so a missed replace fails loudly.
##########
table/metadata_preflight_test.go:
##########
@@ -85,6 +86,88 @@ func TestParseMetadataBytesAssignsMissingPartitionFieldIDs(t
*testing.T) {
}
}
+func TestParseMetadataBytesNormalizesStaleLastPartitionID(t *testing.T) {
+ data := strings.Replace(ExampleTableMetadataV2,
+ `"last-partition-id": 1000`, `"last-partition-id": 999`, 1)
+
+ parsed, err := ParseMetadataBytes([]byte(data))
+ require.NoError(t, err)
+ require.NotNil(t, parsed.LastPartitionSpecID())
+ assert.Equal(t, 1000, *parsed.LastPartitionSpecID())
+
+ update := NewUpdateSpec(New(nil, parsed, "", nil,
nil).NewTransaction(), false).
+ AddField("x", iceberg.BucketTransform{NumBuckets: 16},
"x_bucket")
+ _, _, err = update.BuildUpdates()
+ require.NoError(t, err)
+ updated, err := update.Apply()
+ require.NoError(t, err)
+ require.Equal(t, 2, updated.NumFields())
+ assert.Equal(t, 1001, updated.Field(1).FieldID)
+}
+
+func TestAssignMissingPartitionFieldIDsPreservesConsistentMetadata(t
*testing.T) {
+ for _, tt := range []struct {
+ name string
+ input string
+ }{
+ {
+ name: "counter below assignment floor with no fields",
+ input:
`{"last-updated-ms":0,"last-partition-id":0,"partition-specs":[{"spec-id":0,"fields":[]}]}`,
+ },
+ {
+ name: "counter above greatest field ID",
+ input:
`{"last-updated-ms":0,"last-partition-id":1001,"partition-specs":[{"spec-id":0,"fields":[{"field-id":1000}]}]}`,
+ },
+ } {
+ t.Run(tt.name, func(t *testing.T) {
+ normalized, err :=
assignMissingPartitionFieldIDs([]byte(tt.input))
+ require.NoError(t, err)
+ assert.Equal(t, tt.input, string(normalized))
+ })
+ }
+}
+
+func TestAssignMissingPartitionFieldIDsNormalizesStaleCounter(t *testing.T) {
Review Comment:
While we're here, every stale-counter case uses a single spec in list form.
Could we add a two-spec case where the stale counter sits below a field id in
the second spec, and one using the V1 partition-spec key? The scan crosses all
specs and the re-marshal path differs for the non-list form, so those are the
two branches currently unexercised. Low priority.
--
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]
