laskoviymishka commented on code in PR #1641:
URL: https://github.com/apache/iceberg-go/pull/1641#discussion_r3740296400
##########
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 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.
##########
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:
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?
##########
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)
Review Comment:
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.
##########
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)
Review Comment:
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.
##########
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 {
Review Comment:
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?
--
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]
