fix(table): reuse historical partition field IDs on v2+ tables when re-adding a removed field by badalprasadsingh · Pull Request #1641 · apache/iceberg-go

badalprasadsingh · 2026-08-04T17:10:13Z

Description

UpdateSpec.partitionField failed to reuse a previously-removed partition field's ID when the field was re-added, allocating a brand-new ID instead. This violates the Iceberg field-ID identity contract for format v2+ tables.

This PR contains 2 fixes:

Re-adding without an explicit name
Format-version gating (adding support for V2+ tables)

Backward Compatibility

No impact on existing tables. This code runs only while building a new partition-spec update.

Tests

Added TestUpdateSpecReuseHistoricalFieldID covering all necessary edge-cases.

Signed-off-by: badalprasadsingh <badal@datazip.io>

zeroshade

Thanks for tightening historical partition-field reuse. The exact source-ID plus canonical-transform comparison is sound: it distinguishes bucket/truncate parameters correctly, and limiting reuse to format v2+ is the right boundary.

The remaining blocker is the same-update remove/re-add-with-a-different-name case described inline. It allocates a fresh partition field ID where Java treats the operation as undo-delete plus rename and preserves the ID. Because partition field IDs are permanent once committed, that result cannot be undone after commit.

There is an important overlap with #1651 in table/update_spec.go: historical lookup must use the staged metadata so earlier operations in the transaction are visible, while concurrency requirements must use the original catalog snapshot. Suggested fix: keep those two metadata sources distinct when combining the changes.

For the unnamed case, choosing the first matching historical name is deterministic but establishes precedence among multiple historical names. Suggested fix: document that precedence. Please also add direct v1 coverage, a parameterized-transform mismatch, unchanged last-partition-id on reuse, a committed removal followed by a later different-name add, and multiple-historical-name precedence.

zeroshade · 2026-08-06T22:01:36Z

+				// Reuse the historical field's ID when no explicit name is
+				// requested (match on source + transform alone) or when the
+				// requested name matches.
+				if len(name) == 0 || field.Name == name {


A remove and re-add of the same source/transform with a different name in this same UpdateSpec skips this branch and falls through to newFieldId(). Java treats that operation as undo-delete plus rename, preserving the current field ID. Since partition field IDs are permanent once committed, allocating the fresh ID here cannot be undone after commit. Suggested fix: detect a field deleted by this update separately, restore its current ID, and apply the requested rename; only a different-name add in a later update after a committed removal should receive a fresh ID.

zeroshade · 2026-08-06T22:01:36Z

+		// fresh field ID is allocated after the last assigned one (1001 -> 1002).
+		reAdded := newSpec.FieldsBySourceID(1)
+		require.Len(t, reAdded, 1)
+		assert.Equal(t, iceberg.PartitionDataIDStart+2, reAdded[0].FieldID)


This expectation codifies the incorrect same-update behavior. The remove/re-add with id_renamed should preserve PartitionDataIDStart and rename the restored field. Suggested fix: change this assertion accordingly, then add a distinct case that commits the removal to the catalog and performs the different-name add in a later update, where a fresh ID is expected.

laskoviymishka

Nice work on this. The core comparison is right: matching on source ID plus the canonical transform string is the correct way to distinguish bucket/truncate parameters, and gating reuse on format v2+ is the right boundary.

zeroshade's already got the main blocker and I'm with them on it: the same-update remove then re-add-with-a-different-name case allocates a fresh field ID where Java treats it as undo-delete plus rename and keeps the original ID. Since partition field IDs are permanent once committed, that one can't be walked back. I won't re-litigate the fix, zeroshade's inline covers it.

The angle I'd add is on the tests. The "different name allocates a new field ID" subtest asserts 1002 as the expected result, which locks in exactly the divergence zeroshade flagged. So the suite is currently green against the buggy path rather than against Java. Once the impl is fixed that assertion has to flip to 1000, and I'd add a LastAssignedFieldID check on the reuse path too, since the early return skips the counter and a bug that reuses the ID but still bumps it would pass everything we have today.

A few things I'd want before merge:

flip the different-name assertion to 1000 (name "id_renamed") once the impl matches Java
assert LastAssignedFieldID is unchanged on the reuse path
document which spec wins on the unnamed-reuse path, and confirm the returned historical name isn't stale after a source-column rename
agreed with zeroshade's remaining test asks (v1 negative coverage, transform-parameter mismatch, precedence)

Once the different-name case is preserving the ID and the tests assert against Java's numbers, happy to take another pass.

laskoviymishka · 2026-08-08T07:58:37Z

+		// fresh field ID is allocated after the last assigned one (1001 -> 1002).
+		reAdded := newSpec.FieldsBySourceID(1)
+		require.Len(t, reAdded, 1)
+		assert.Equal(t, iceberg.PartitionDataIDStart+2, reAdded[0].FieldID)


This assertion codifies the exact divergence zeroshade flagged as the expected result. Per Java, a same-update remove then re-add with a different name is undo-delete plus rename, so this should be PartitionDataIDStart (1000) with name "id_renamed", not 1002.

As written the suite is green against the buggy path instead of against Java, so even a correct fix would fail right here. I'd flip it to 1000 once the impl preserves the ID.

laskoviymishka · 2026-08-08T07:58:38Z

+				// Reuse the historical field's ID when no explicit name is
+				// requested (match on source + transform alone) or when the
+				// requested name matches.
+				if len(name) == 0 || field.Name == name {


On the no-name branch we return the first (source, transform) match while iterating specs in ascending spec-ID order, and hand back that historical field.Name. If the same source+transform carried different names across specs (renamed before removal), this returns the oldest name, which can be stale for the current schema.

I'd document which spec is meant to win here and add a multi-spec test, which also covers zeroshade's precedence note. wdyt?

laskoviymishka · 2026-08-08T07:58:38Z

+		require.Len(t, reAdded, 1)
+		assert.Equal(t, iceberg.PartitionDataIDStart, reAdded[0].FieldID)
+		assert.Equal(t, "id_identity", reAdded[0].Name)
+		assert.Equal(t, iceberg.IdentityTransform{}, reAdded[0].Transform)


While we're here, I'd add a LastAssignedFieldID assertion on this reuse path. The early return skips the counter, so a bug that reuses the ID but still bumps last-partition-id would pass every assertion we have today. Something like assert.Equal(t, iceberg.PartitionDataIDStart+1, newSpec.LastAssignedFieldID()) pins zeroshade's last-partition-id ask to a concrete check.

laskoviymishka · 2026-08-08T07:58:38Z

@@ -369,11 +370,14 @@ func (us *UpdateSpec) partitionField(key transformKey, name string) (iceberg.Par
 		}
 		for _, field := range historicalFields {
 			if field.SourceID() == sourceId && field.Transform.String() == transformName {


The comparison logic here is sound for canonical transform strings. The one case I wanted to flag: this is a textual match on Transform.String(), so a non-canonical string from externally-written metadata (something like "bucket[016]" vs "bucket[16]") would miss a semantically identical historical field and allocate a fresh ID.

If ParseTransform + String() round-trips canonically for every transform we can just note that assumption in a comment; otherwise a structural compare would be safer. wdyt?

laskoviymishka · 2026-08-08T07:58:38Z

+		reAdded := newSpec.FieldsBySourceID(1)
+		require.Len(t, reAdded, 1)
+		assert.Equal(t, iceberg.PartitionDataIDStart, reAdded[0].FieldID)
+		assert.Equal(t, "id_identity", reAdded[0].Name)


Small consistency thing: subtests 1 and 4 assert the reused Transform but this one doesn't. Worth adding assert.Equal(t, iceberg.IdentityTransform{}, reAdded[0].Transform) so the matching-name path proves it recycles the transform too.

fix: resuse historical field id during update spec

1fae175

Signed-off-by: badalprasadsingh <badal@datazip.io>

badalprasadsingh requested a review from zeroshade as a code owner August 4, 2026 17:10

tanmayrauth approved these changes Aug 5, 2026

View reviewed changes

zeroshade requested changes Aug 6, 2026

View reviewed changes

zeroshade mentioned this pull request Aug 6, 2026

fix(table): update spec reading stale metadata in chained transactions #1651

Open

laskoviymishka requested changes Aug 8, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix(table): reuse historical partition field IDs on v2+ tables when re-adding a removed field - #1641

fix(table): reuse historical partition field IDs on v2+ tables when re-adding a removed field#1641
badalprasadsingh wants to merge 1 commit into
apache:mainfrom
badalprasadsingh:fix/update-spec-reuse-historical-field-id

badalprasadsingh commented Aug 4, 2026

Uh oh!

zeroshade left a comment

Uh oh!

zeroshade Aug 6, 2026

Uh oh!

zeroshade Aug 6, 2026

Uh oh!

laskoviymishka left a comment

Uh oh!

laskoviymishka Aug 8, 2026

Uh oh!

laskoviymishka Aug 8, 2026

Uh oh!

laskoviymishka Aug 8, 2026

Uh oh!

laskoviymishka Aug 8, 2026

Uh oh!

laskoviymishka Aug 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

badalprasadsingh commented Aug 4, 2026

Description

Backward Compatibility

Tests

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

zeroshade Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

zeroshade Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants