You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "mattfaltyn (via GitHub)" <gi...@apache.org> on 2026/09/03 08:47:56 UTC
[PR] fix(table): normalize stale last partition ID [iceberg-go]
mattfaltyn opened a new pull request, #1988:
URL: https://github.com/apache/iceberg-go/pull/1988
Closes #1987.
## Summary
- normalize `last-partition-id` to the greatest assigned partition field ID during metadata preflight
- preserve the unchanged-byte fast path for already-consistent metadata
- cover the all-field-IDs-present case that previously allowed `UpdateSpec` to reuse a historical ID
## Why
Format v2+ partition field IDs are unique across every spec in a table. When persisted metadata contained field ID `1000` but `last-partition-id` was `999`, parsing preserved the stale counter. A later distinct partition transform could then also receive ID `1000`.
The preflight already computes the maximum of `999`, the persisted counter, and all explicit field IDs for legacy-ID repair. This change also writes that maximum when only the counter is stale. Valid metadata still returns the original bytes without re-marshalling.
## Testing
- `go test ./table -run '^(TestParseMetadataBytesNormalizesStaleLastPartitionID|TestParseMetadataBytesAssignsMissingPartitionFieldIDs|TestUpdateSpecReuseHistoricalFieldID)$' -count=1`
- `go run /private/tmp/iceberg-go-counter-repro/partition.go` (two failing-before/passing-after reproductions plus control)
- `make test`
- `make test-race`
- `make test-assert`
- `golangci-lint v2.12.2 run --timeout=10m`
- `git diff --check`
No documentation changes are needed because this restores the existing metadata contract without changing the public API.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): allocate partition IDs from history [iceberg-go]
Posted by "mattfaltyn (via GitHub)" <gi...@apache.org>.
mattfaltyn commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3972675333
##########
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:
> move the max-field-id scan to the write side\n\nThanks—implemented in c9bae41 with one shared history-aware allocation floor.
##########
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:
> assert that update requirement equals 999\n\nGreat catch—covered, and the new field still receives 1001.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): normalize stale last partition ID [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3960485768
##########
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 {
Review Comment:
**nit** — Unknown-spec-key fidelity guard is itself unpinned by tests
The 'if len(missingFields) > 0' guard prevents the stale-counter-only path from round-tripping specs through rawPartitionSpec, which would drop spec-level keys the struct cannot represent. Replacing the condition with 'true' leaves the entire table package green, so the preservation behaviour has no regression test. TestAssignMissingPartitionFieldIDsNormalizesLegacyStaleCounter passes through this path but only asserts last-partition-id, never the spec bytes. Non-blocking; the guard is an improvement over pre-PR behaviour either way.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): allocate partition IDs from history [iceberg-go]
Posted by "mattfaltyn (via GitHub)" <gi...@apache.org>.
mattfaltyn commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3987845556
##########
table/metadata_builder_internal_test.go:
##########
@@ -357,6 +357,33 @@ func TestAddRemovePartitionSpec(t *testing.T) {
require.ErrorContains(t, err, "id 1")
}
+func TestAddPartitionSpecAllocatesAfterHistoricalFieldID(t *testing.T) {
+ data := strings.Replace(ExampleTableMetadataV2,
+ `"last-partition-id": 1000`, `"last-partition-id": 999`, 1)
+ require.Contains(t, data, `"last-partition-id": 999`)
+
+ metadata, err := ParseMetadataBytes([]byte(data))
+ require.NoError(t, err)
+ builder, err := MetadataBuilderFromBase(metadata, "")
+ require.NoError(t, err)
+
+ addedSpec, err := iceberg.NewPartitionSpecOpts(
+ iceberg.WithSpecID(1),
+ iceberg.AddPartitionFieldBySourceID(1, "x_bucket", iceberg.BucketTransform{NumBuckets: 16}, builder.CurrentSchema(), nil),
+ )
+ require.NoError(t, err)
+ require.NoError(t, builder.AddPartitionSpec(&addedSpec, false))
+
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ added := rebuilt.PartitionSpecByID(1)
+ require.NotNil(t, added)
+ require.Equal(t, 1, added.NumFields())
+ assert.Equal(t, 1001, added.Field(0).FieldID)
+ require.NotNil(t, rebuilt.LastPartitionSpecID())
+ assert.Equal(t, 1001, *rebuilt.LastPartitionSpecID())
Review Comment:
> “Could we add a case with `b.lastPartitionID = nil` and a spec containing `field-id` 1000?”
Added. The nil-counter subtest confirms the next field ID is 1001. Thanks!
##########
table/metadata.go:
##########
@@ -700,14 +700,27 @@ func (b *MetadataBuilder) AddSchema(schema *iceberg.Schema) error {
return nil
}
+func partitionFieldIDFloor(lastPartitionID *int, specs []iceberg.PartitionSpec) int {
+ floor := partitionFieldStartID - 1
Review Comment:
> “Use `iceberg.PartitionDataIDStart - 1` here so they cannot drift.”
Updated. The helper now uses the canonical Iceberg constant. Nice catch!
##########
table/metadata.go:
##########
@@ -700,14 +700,27 @@ func (b *MetadataBuilder) AddSchema(schema *iceberg.Schema) error {
return nil
}
+func partitionFieldIDFloor(lastPartitionID *int, specs []iceberg.PartitionSpec) int {
+ floor := partitionFieldStartID - 1
+ if lastPartitionID != nil {
+ floor = max(floor, *lastPartitionID)
+ }
+ for _, spec := range specs {
+ floor = max(floor, spec.LastAssignedFieldID())
+ }
+
+ return floor
+}
+
func (b *MetadataBuilder) AddPartitionSpec(spec *iceberg.PartitionSpec, initial bool) error {
newSpecID := b.reuseOrCreateNewPartitionSpecID(*spec)
curSchema := b.CurrentSchema()
if curSchema == nil {
return errors.New("can't add sort order with no current schema")
}
- freshSpec, err := spec.BindToSchema(curSchema, b.lastPartitionID, &newSpecID)
+ fieldIDFloor := partitionFieldIDFloor(b.lastPartitionID, b.specs)
+ freshSpec, err := spec.BindToSchema(curSchema, &fieldIDFloor, &newSpecID)
Review Comment:
> “`fieldIDFloor` is already in scope and always `>= prev`.”
Updated. Write-back now uses `max(maxFieldID, fieldIDFloor)`, with an empty-spec regression test. Thanks!
##########
table/metadata.go:
##########
@@ -252,9 +252,9 @@ type Metadata interface {
// DefaultPartitionSpec is the ID of the current spec that writerFactory should
// use by default.
DefaultPartitionSpec() int
- // LastPartitionSpecID is the highest assigned partition field ID across
- // all partition specs for the table. This is used to ensure partition
- // fields are always assigned an unused ID when evolving specs.
+ // LastPartitionSpecID returns the persisted last assigned partition field ID.
+ // Allocation also scans partition spec history because metadata written by
Review Comment:
> “This second sentence describes `partitionFieldIDFloor`, not this method.”
Updated. The method doc now focuses on the persisted counter, and the history scan is documented on the helper. 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): normalize stale last partition ID [iceberg-go]
Posted by "mattfaltyn (via GitHub)" <gi...@apache.org>.
mattfaltyn commented on PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#issuecomment-5557842249
> Adding one case with a sub-1000 partition field ID and a lower counter (expect the greatest field ID, not 999) would pin it.
Thanks for the precise mutation-based recommendation. I added TestAssignMissingPartitionFieldIDsNormalizesLegacyStaleCounter and confirmed it fails with actual 999 when the write site uses the allocation cursor.
> Issue #1987 asks for coverage that asserts both that the parsed counter becomes 1000 and that the next distinct partition field receives 1001.
Great call. I extended TestParseMetadataBytesNormalizesStaleLastPartitionID with a bucket[16](x) assertion for field ID 1001.
Both updates are in e01dbb2. The full test suite and golangci-lint pass. Thank you for the thorough review!
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): normalize stale last partition ID [iceberg-go]
Posted by "mattfaltyn (via GitHub)" <gi...@apache.org>.
mattfaltyn commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3927520149
##########
table/metadata.go:
##########
@@ -2016,11 +2016,13 @@ func assignMissingPartitionFieldIDsFromMetadata(b []byte, metadata map[string]js
}
lastAssignedID := iceberg.PartitionDataIDStart - 1
+ lastPartitionID := lastAssignedID
Review Comment:
Thanks for catching this. Fixed in ebf0a16 by separating the persisted counter target from the 999 allocation cursor. Empty specs with sub-999 counters now retain the original bytes and catalog-visible value, while stale counters with assigned fields still normalize correctly.
##########
table/metadata_preflight_test.go:
##########
@@ -85,6 +85,16 @@ func TestParseMetadataBytesAssignsMissingPartitionFieldIDs(t *testing.T) {
}
}
+func TestParseMetadataBytesNormalizesStaleLastPartitionID(t *testing.T) {
+ data := strings.Replace(ExampleTableMetadataV2,
Review Comment:
Thanks for identifying the missing branches. Added coverage in ebf0a16 for sub-999 counters with no assigned fields, counters above the greatest assigned field ID, and stale counters combined with a missing field ID. The first two cases also verify the unchanged-byte fast path.
##########
table/metadata.go:
##########
@@ -2055,18 +2057,20 @@ func assignMissingPartitionFieldIDsFromMetadata(b []byte, metadata map[string]js
field["field-id"] = rawFieldID
}
- 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 {
Review Comment:
Thanks for flagging this. I confirmed the unknown partition-spec key loss predates this change and left it unchanged to keep #1987 focused. The stale-counter-only path introduced here continues to avoid partition-spec re-marshalling.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): allocate partition IDs from history [iceberg-go]
Posted by "mattfaltyn (via GitHub)" <gi...@apache.org>.
mattfaltyn commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3972675333
##########
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:
> move the max-field-id scan to the write side
Thanks—implemented in c9bae41 with one shared history-aware allocation floor.
##########
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:
> assert that update requirement equals 999
Great catch—covered, and the new field still receives 1001.
##########
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:
> this max is always lastAssignedID
Thanks—removed along with the parser-side normalization path.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): allocate partition IDs from history [iceberg-go]
Posted by "mattfaltyn (via GitHub)" <gi...@apache.org>.
mattfaltyn commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3987845498
##########
table/metadata.go:
##########
@@ -252,9 +252,9 @@ type Metadata interface {
// DefaultPartitionSpec is the ID of the current spec that writerFactory should
// use by default.
DefaultPartitionSpec() int
- // LastPartitionSpecID is the highest assigned partition field ID across
- // all partition specs for the table. This is used to ensure partition
- // fields are always assigned an unused ID when evolving specs.
+ // LastPartitionSpecID returns the persisted last assigned partition field ID.
+ // Allocation also scans partition spec history because metadata written by
Review Comment:
> “This second sentence describes `partitionFieldIDFloor`, not this method.”
Done — the method doc now focuses on the persisted counter, and the history scan is documented on the helper. Thanks!
##########
table/metadata_builder_internal_test.go:
##########
@@ -357,6 +357,33 @@ func TestAddRemovePartitionSpec(t *testing.T) {
require.ErrorContains(t, err, "id 1")
}
+func TestAddPartitionSpecAllocatesAfterHistoricalFieldID(t *testing.T) {
+ data := strings.Replace(ExampleTableMetadataV2,
+ `"last-partition-id": 1000`, `"last-partition-id": 999`, 1)
+ require.Contains(t, data, `"last-partition-id": 999`)
+
+ metadata, err := ParseMetadataBytes([]byte(data))
+ require.NoError(t, err)
+ builder, err := MetadataBuilderFromBase(metadata, "")
+ require.NoError(t, err)
+
+ addedSpec, err := iceberg.NewPartitionSpecOpts(
+ iceberg.WithSpecID(1),
+ iceberg.AddPartitionFieldBySourceID(1, "x_bucket", iceberg.BucketTransform{NumBuckets: 16}, builder.CurrentSchema(), nil),
+ )
+ require.NoError(t, err)
+ require.NoError(t, builder.AddPartitionSpec(&addedSpec, false))
+
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ added := rebuilt.PartitionSpecByID(1)
+ require.NotNil(t, added)
+ require.Equal(t, 1, added.NumFields())
+ assert.Equal(t, 1001, added.Field(0).FieldID)
+ require.NotNil(t, rebuilt.LastPartitionSpecID())
+ assert.Equal(t, 1001, *rebuilt.LastPartitionSpecID())
Review Comment:
> “Could we add a case with `b.lastPartitionID = nil` and a spec containing `field-id` 1000?”
Done — the nil-counter subtest confirms the next field ID is 1001. Thanks!
##########
table/metadata.go:
##########
@@ -700,14 +700,27 @@ func (b *MetadataBuilder) AddSchema(schema *iceberg.Schema) error {
return nil
}
+func partitionFieldIDFloor(lastPartitionID *int, specs []iceberg.PartitionSpec) int {
+ floor := partitionFieldStartID - 1
Review Comment:
> “Use `iceberg.PartitionDataIDStart - 1` here so they cannot drift.”
Done — the helper now uses the canonical Iceberg constant. Nice catch!
##########
table/metadata.go:
##########
@@ -700,14 +700,27 @@ func (b *MetadataBuilder) AddSchema(schema *iceberg.Schema) error {
return nil
}
+func partitionFieldIDFloor(lastPartitionID *int, specs []iceberg.PartitionSpec) int {
+ floor := partitionFieldStartID - 1
+ if lastPartitionID != nil {
+ floor = max(floor, *lastPartitionID)
+ }
+ for _, spec := range specs {
+ floor = max(floor, spec.LastAssignedFieldID())
+ }
+
+ return floor
+}
+
func (b *MetadataBuilder) AddPartitionSpec(spec *iceberg.PartitionSpec, initial bool) error {
newSpecID := b.reuseOrCreateNewPartitionSpecID(*spec)
curSchema := b.CurrentSchema()
if curSchema == nil {
return errors.New("can't add sort order with no current schema")
}
- freshSpec, err := spec.BindToSchema(curSchema, b.lastPartitionID, &newSpecID)
+ fieldIDFloor := partitionFieldIDFloor(b.lastPartitionID, b.specs)
+ freshSpec, err := spec.BindToSchema(curSchema, &fieldIDFloor, &newSpecID)
Review Comment:
> “`fieldIDFloor` is already in scope and always `>= prev`.”
Done — write-back now uses `max(maxFieldID, fieldIDFloor)`, with an empty-spec regression test. Great suggestion!
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): normalize stale last partition ID [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3937952489
##########
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)
if err != nil {
Review Comment:
**minor** — Floor/persist separation at the write site survives mutation of the whole table package
metadata.go:2081 persists normalizedLastPartitionID rather than lastAssignedID — the headline guarantee of commit ebf0a16. Substituting lastAssignedID there leaves every test in ./table passing. The two values only diverge at this line when no field IDs are missing and the persisted counter plus all partition field IDs are below 999, which is reachable with legacy v1 metadata whose partition field IDs predate the 1000 floor. All three added test cases hit the early return at metadata.go:2050 and never reach line 2081, so the separation is asserted in only one of its two directions. Behavior at head is correct; this is a coverage gap, not a live bug. Adding one case with a sub-1000 partition field ID and a lower counter (expect the greatest field ID, not 999) would pin it.
##########
table/metadata_preflight_test.go:
##########
@@ -85,6 +85,66 @@ func TestParseMetadataBytesAssignsMissingPartitionFieldIDs(t *testing.T) {
}
}
+func TestParseMetadataBytesNormalizesStaleLastPartitionID(t *testing.T) {
+ data := strings.Replace(ExampleTableMetadataV2,
Review Comment:
**nit** — End-to-end allocation half of the issue's suggested regression coverage is not asserted
Issue #1987 asks for coverage that asserts both that the parsed counter becomes 1000 and that the next distinct partition field receives 1001. TestParseMetadataBytesNormalizesStaleLastPartitionID asserts only the former. I verified the latter holds today, so this is purely about locking in the user-visible symptom (the cross-spec ID collision) rather than only its parse-level cause.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): normalize stale last partition ID [iceberg-go]
Posted by "mattfaltyn (via GitHub)" <gi...@apache.org>.
mattfaltyn commented on PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#issuecomment-5530333194
Thanks for the thorough review and clear reproduction. The counter-floor regression and requested boundary coverage are addressed in ebf0a16. I also updated the PR description to distinguish the newly added regression tests from the pre-existing historical-ID test. All local unit, race, assert, and lint checks pass.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): allocate partition IDs from history [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3987734230
##########
table/metadata.go:
##########
@@ -252,9 +252,9 @@ type Metadata interface {
// DefaultPartitionSpec is the ID of the current spec that writerFactory should
// use by default.
DefaultPartitionSpec() int
- // LastPartitionSpecID is the highest assigned partition field ID across
- // all partition specs for the table. This is used to ensure partition
- // fields are always assigned an unused ID when evolving specs.
+ // LastPartitionSpecID returns the persisted last assigned partition field ID.
+ // Allocation also scans partition spec history because metadata written by
Review Comment:
This second sentence describes `partitionFieldIDFloor`, not this method. `LastPartitionSpecID` just returns the stored `*int` and scans nothing.
I'd keep the doc focused on what it returns (the persisted counter, which may be stale) and move the "allocation scans spec history" note down onto `partitionFieldIDFloor`, which has no doc today. That way a `Metadata` implementer reading this knows the value can be stale and that allocation needs the floor helper rather than this value alone.
##########
table/metadata_builder_internal_test.go:
##########
@@ -357,6 +357,33 @@ func TestAddRemovePartitionSpec(t *testing.T) {
require.ErrorContains(t, err, "id 1")
}
+func TestAddPartitionSpecAllocatesAfterHistoricalFieldID(t *testing.T) {
+ data := strings.Replace(ExampleTableMetadataV2,
+ `"last-partition-id": 1000`, `"last-partition-id": 999`, 1)
+ require.Contains(t, data, `"last-partition-id": 999`)
+
+ metadata, err := ParseMetadataBytes([]byte(data))
+ require.NoError(t, err)
+ builder, err := MetadataBuilderFromBase(metadata, "")
+ require.NoError(t, err)
+
+ addedSpec, err := iceberg.NewPartitionSpecOpts(
+ iceberg.WithSpecID(1),
+ iceberg.AddPartitionFieldBySourceID(1, "x_bucket", iceberg.BucketTransform{NumBuckets: 16}, builder.CurrentSchema(), nil),
+ )
+ require.NoError(t, err)
+ require.NoError(t, builder.AddPartitionSpec(&addedSpec, false))
+
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ added := rebuilt.PartitionSpecByID(1)
+ require.NotNil(t, added)
+ require.Equal(t, 1, added.NumFields())
+ assert.Equal(t, 1001, added.Field(0).FieldID)
+ require.NotNil(t, rebuilt.LastPartitionSpecID())
+ assert.Equal(t, 1001, *rebuilt.LastPartitionSpecID())
Review Comment:
This covers the stale-999 path nicely. The one branch it doesn't reach is nil `lastPartitionID` with a spec above the start, a V1 table, or metadata with no last-partition-id. If someone drops the spec-scan loop in the helper, that branch would regress and this test would still pass.
Could we add a case with `b.lastPartitionID = nil` and a spec containing `field-id` 1000, asserting the next allocated ID is 1001?
##########
table/metadata.go:
##########
@@ -700,14 +700,27 @@ func (b *MetadataBuilder) AddSchema(schema *iceberg.Schema) error {
return nil
}
+func partitionFieldIDFloor(lastPartitionID *int, specs []iceberg.PartitionSpec) int {
+ floor := partitionFieldStartID - 1
+ if lastPartitionID != nil {
+ floor = max(floor, *lastPartitionID)
+ }
+ for _, spec := range specs {
+ floor = max(floor, spec.LastAssignedFieldID())
+ }
+
+ return floor
+}
+
func (b *MetadataBuilder) AddPartitionSpec(spec *iceberg.PartitionSpec, initial bool) error {
newSpecID := b.reuseOrCreateNewPartitionSpecID(*spec)
curSchema := b.CurrentSchema()
if curSchema == nil {
return errors.New("can't add sort order with no current schema")
}
- freshSpec, err := spec.BindToSchema(curSchema, b.lastPartitionID, &newSpecID)
+ fieldIDFloor := partitionFieldIDFloor(b.lastPartitionID, b.specs)
+ freshSpec, err := spec.BindToSchema(curSchema, &fieldIDFloor, &newSpecID)
Review Comment:
The floor made it into `BindToSchema`, but the `b.lastPartitionID` write-back a few lines down still uses `max(maxFieldID, prev)` with `prev = *b.lastPartitionID`. For a non-empty spec that's fine, `maxFieldID` heals it. For an empty spec (unpartition) `maxFieldID` is 0 and `prev` is the stale 999, so the counter we serialize stays stale, and a reader that trusts it without scanning specs can still allocate 1000 and collide, the exact case this PR is closing on the `NewUpdateSpec` path.
`fieldIDFloor` is already in scope and always `>= prev`, so:
```go
lastPartitionID := max(maxFieldID, fieldIDFloor)
```
The `prev` block and its nil-guard fall out as dead code, and the assertion value is unaffected since that's drawn from the base `Metadata`. wdyt?
##########
table/metadata.go:
##########
@@ -700,14 +700,27 @@ func (b *MetadataBuilder) AddSchema(schema *iceberg.Schema) error {
return nil
}
+func partitionFieldIDFloor(lastPartitionID *int, specs []iceberg.PartitionSpec) int {
+ floor := partitionFieldStartID - 1
Review Comment:
`partitionFieldStartID` and `iceberg.PartitionDataIDStart` are both 1000, but the `assignMissingPartitionFieldIDs` / nil-guard paths use the latter and there's no compile-time link between them. This helper is now a third call site where a silent divergence would produce a wrong floor. I'd use `iceberg.PartitionDataIDStart - 1` here so they can't drift.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): normalize stale last partition ID [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3926375136
##########
table/metadata.go:
##########
@@ -2016,11 +2016,13 @@ func assignMissingPartitionFieldIDsFromMetadata(b []byte, metadata map[string]js
}
lastAssignedID := iceberg.PartitionDataIDStart - 1
+ lastPartitionID := lastAssignedID
Review Comment:
**major** — 999 assignment floor is conflated with the persisted counter, rewriting collision-free sub-999 values
lastAssignedID is seeded to iceberg.PartitionDataIDStart-1 (999) at line 2018 and then max'd with the persisted counter and all field IDs. The same variable is written back to last-partition-id at line 2077. For metadata with NO assigned partition field IDs, there is no collision to repair, yet any persisted counter below 999 (0, 5, 998) is rewritten to 999 and the unchanged-byte fast path is lost. Because update_spec.go:190-197 derives AssertLastAssignedPartitionID from this normalized value and rest.go:1626-1636 sends it to the catalog, a spec-changing commit against a REST catalog that persisted 0 now asserts 999 and is rejected -- before this PR the client sent 0 and matched. Fix: compute the persist target as max(persistedCounter, maxAssignedFieldID) as a variable separate from the assignment floor, so tables with no assigned field IDs are left untouched. If the 999 floor is deliberate (the linked issue does request 'the greatest of 999 and every explicit partition field ID')
, add explicit test coverage for sub-999 counters and state the intentional mutation in the PR description, since the description currently claims only that already-consistent metadata is preserved.
<details><summary>Evidence</summary>
```text
Probe P8 (sweep, unpartitioned v2, partition-specs [{spec-id:0,fields:[]}]): 'persisted=0 -> parsed=999 rewritten=true', 'persisted=5 -> parsed=999 rewritten=true', 'persisted=998 -> parsed=999 rewritten=true', 'persisted=999 -> parsed=999 rewritten=false'. Probe P10: 'client asserts 999 against catalog holding 0 -> requirement failed: last assigned partition id has changed: expected 999, found 0'. Probe P11: 're-serialized last-partition-id = 999 (was 0 on disk)'. Shape present in repo fixtures: cmd/iceberg/snapshots_test.go:45, cmd/iceberg/branch_tag_test.go:49, cmd/iceberg/partition_stats_test.go:44.
```
</details>
##########
table/metadata.go:
##########
@@ -2055,18 +2057,20 @@ func assignMissingPartitionFieldIDsFromMetadata(b []byte, metadata map[string]js
field["field-id"] = rawFieldID
}
- 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 {
Review Comment:
**minor** — Missing-field re-marshal path silently drops unknown partition-spec keys
The new 'if len(missingFields) > 0' guard correctly keeps the stale-counter-only path from round-tripping specs through rawPartitionSpec (which carries only spec-id and fields). But the missing-field path it now wraps still does, so any other key on a partition-spec object is dropped on rewrite. This is pre-existing rather than introduced -- flagging it because the PR restructured exactly this block and the asymmetry between the two paths is now visible in the diff.
##########
table/metadata_preflight_test.go:
##########
@@ -85,6 +85,16 @@ func TestParseMetadataBytesAssignsMissingPartitionFieldIDs(t *testing.T) {
}
}
+func TestParseMetadataBytesNormalizesStaleLastPartitionID(t *testing.T) {
+ data := strings.Replace(ExampleTableMetadataV2,
Review Comment:
**minor** — New early-return condition adds three branches, only one is tested
The condition at metadata.go:2047 introduces distinct branches: counter below max field ID (tested), counter above max field ID (must stay untouched), counter below the 999 floor with no field IDs, and stale counter combined with a missing field-id. Only the first has a test. I verified the untested ones behave as follows -- add cases for them so the condition is pinned: counter-above-max stays untouched, and stale-counter-plus-missing-field-id assigns correctly.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): allocate partition IDs from history [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): normalize stale last partition ID [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): allocate partition IDs from history [iceberg-go]
Posted by "mattfaltyn (via GitHub)" <gi...@apache.org>.
mattfaltyn commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3972677489
##########
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:
> add a require.Contains
Good call—added explicit fixture guards so silent replacement failures go red.
##########
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:
> add a two-spec case and one using the V1 partition-spec key
Thanks—the two-spec history case is covered; the existing V1 missing-ID test continues to cover its re-marshal path.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): allocate partition IDs from history [iceberg-go]
Posted by "mattfaltyn (via GitHub)" <gi...@apache.org>.
mattfaltyn commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3972676707
##########
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:
> this max is always lastAssignedID\n\nThanks—removed along with the parser-side normalization path.
##########
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:
> add a require.Contains\n\nGood call—added explicit fixture guards so silent replacement failures go red.
##########
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:
> add a two-spec case and one using the V1 partition-spec key\n\nThanks—the two-spec history case is covered; the existing V1 missing-ID test continues to cover its re-marshal path.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] fix(table): normalize stale last partition ID [iceberg-go]
Posted by "mattfaltyn (via GitHub)" <gi...@apache.org>.
mattfaltyn commented on PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#issuecomment-5608145588
> I'd move the max-field-id scan to the write side.
Implemented in c9bae41. Parsing now preserves the catalog-visible counter, while `UpdateSpec` and `MetadataBuilder.AddPartitionSpec` share a history-aware allocation floor. A stale `999` requirement still matches the catalog and the new field receives `1001`.
> If we assert that update's AssertLastAssignedPartitionID requirement equals 999 here, this test goes red and surfaces the desync.
Covered, together with explicit fixture guards and a two-spec historical case. The V1 missing-ID re-marshal path remains covered by the existing test; stale-counter re-marshalling is no longer part of the fix. Full tests, targeted race coverage, assert tests, vet, and lint are clean. Thanks for the sharp review!
--
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: issues-unsubscribe@iceberg.apache.org
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