zeroshade commented on code in PR #1641:
URL: https://github.com/apache/iceberg-go/pull/1641#discussion_r3732110680
##########
table/update_spec.go:
##########
@@ -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 {
- if len(name) > 0 && field.Name == name {
+ // 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 {
Review Comment:
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.
##########
table/update_spec_test.go:
##########
@@ -568,3 +568,126 @@ func TestUpdateSpecCommit(t *testing.T) {
assert.Nil(t, err)
})
}
+
+func TestUpdateSpecReuseHistoricalFieldID(t *testing.T) {
+ t.Run("re-add deleted field without a name reuses historical field ID",
func(t *testing.T) {
+ txn := testPartitionedTable.NewTransaction()
+ specUpdate := table.NewUpdateSpec(txn, false)
+
+ _, _, err := specUpdate.
+ RemoveField("id_identity").
+ AddField("id", iceberg.IdentityTransform{}, "").
+ BuildUpdates()
+ require.NoError(t, err)
+
+ newSpec, err := specUpdate.Apply()
+ require.NoError(t, err)
+
+ // The re-added field on source 1 must recycle the original
field ID
+ // (PartitionDataIDStart == 1000) and the historical name,
rather than
+ // allocating a brand-new field ID.
+ 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)
+ assert.Equal(t, iceberg.IdentityTransform{},
reAdded[0].Transform)
+
+ // The untouched field must remain unchanged.
+ untouched := newSpec.FieldsBySourceID(5)
+ require.Len(t, untouched, 1)
+ assert.Equal(t, iceberg.PartitionDataIDStart+1,
untouched[0].FieldID)
+ assert.Equal(t, "street_void", untouched[0].Name)
+ })
+
+ t.Run("re-add deleted field with matching name reuses historical field
ID", func(t *testing.T) {
+ txn := testPartitionedTable.NewTransaction()
+ specUpdate := table.NewUpdateSpec(txn, false)
+
+ _, _, err := specUpdate.
+ RemoveField("id_identity").
+ AddField("id", iceberg.IdentityTransform{},
"id_identity").
+ BuildUpdates()
+ require.NoError(t, err)
+
+ newSpec, err := specUpdate.Apply()
+ require.NoError(t, err)
+
+ 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)
+ })
+
+ t.Run("re-add deleted field with a different name allocates a new field
ID", func(t *testing.T) {
+ txn := testPartitionedTable.NewTransaction()
+ specUpdate := table.NewUpdateSpec(txn, false)
+
+ _, _, err := specUpdate.
+ RemoveField("id_identity").
+ AddField("id", iceberg.IdentityTransform{},
"id_renamed").
+ BuildUpdates()
+ require.NoError(t, err)
+
+ newSpec, err := specUpdate.Apply()
+ require.NoError(t, err)
+
+ // A different explicit name must NOT reuse the historical
field ID; a
+ // 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)
Review Comment:
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.
--
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]
