laskoviymishka commented on code in PR #1688:
URL: https://github.com/apache/iceberg-go/pull/1688#discussion_r3740291793
##########
table/update_schema.go:
##########
@@ -320,6 +352,15 @@ func (u *UpdateSchema) addColumn(path []string, fieldType
iceberg.Type, doc stri
}
parentID = parentField.ID
+
+ if ancestorID, ok := u.findDeletedAncestor(parentID); ok {
+ deletedName, found :=
u.schema.FindColumnName(ancestorID)
+ if !found {
+ deletedName = parentFullPath
Review Comment:
When `FindColumnName(ancestorID)` misses, the fallback uses
`parentFullPath`, which is the immediate parent's path, not the deleted
ancestor's. For the deleted-`outer` / add-`outer.inner.new_leaf` case the error
would then read `...: outer.inner` and point at the wrong field.
In practice an id that's in `u.deletes` should always resolve, so this is a
dead branch today — but it's a wrong-field branch, and the same fallback is
duplicated in `unionAddColumn`. I'd resolve the fallback name from `ancestorID`
itself (or just drop the name) so the two paths can't drift into lying.
##########
table/update_schema.go:
##########
@@ -320,6 +352,15 @@ func (u *UpdateSchema) addColumn(path []string, fieldType
iceberg.Type, doc stri
}
parentID = parentField.ID
+
+ if ancestorID, ok := u.findDeletedAncestor(parentID); ok {
Review Comment:
This walks the full ancestor chain, but Java's `SchemaUpdate` only checks
the immediate resolved parent (`!deletes.contains(parentId)`), and PyIceberg
does the same. So
`DeleteColumn(["outer"]).AddColumn(["outer","inner","new_leaf"])` is rejected
here but succeeds against Java/Py — a real cross-client divergence where the
same script passes on one engine and fails at `Apply()` on ours.
The reason Java can afford the narrow check is that its apply-time tree-drop
makes "delete wins" a defined, non-lossy outcome. Ours drops silently, which is
exactly the hole @zeroshade asked to close — so narrowing back to
`isDeleted(parentID)` would reopen it unless we also made the drop explicit.
So I don't think "just match Java" is right here given the thread above. But
I'd want the divergence chosen deliberately: either keep the walk and document
that we're intentionally stricter than Java/Py, or narrow the check and fix the
silent drop at apply time. wdyt — and worth pulling @zeroshade in since it's
his ancestor-awareness ask?
##########
table/update_schema.go:
##########
@@ -376,7 +417,10 @@ func (u *UpdateSchema) deleteColumn(path []string) error {
return fmt.Errorf("field not found: %s", fullName)
}
- if _, ok := u.adds[field.ID]; ok {
+ // Reject deletion when an addition is staged beneath this field or any
of its descendants;
+ // applyChanges drops the whole subtree, so any such add keyed under a
descendant
+ // would otherwise be silently discarded.
+ if u.hasStagedAddUnder(field.ID) {
Review Comment:
`hasStagedAddUnder` closes the add-under-deleted-subtree hole, but the same
mechanism still exists for staged updates. `UpdateColumn(["a","b","c"])` then
`DeleteColumn(["a"])` stores the update keyed at `c`'s id, this guard doesn't
see it, and `applyChanges` drops the subtree before the update runs — lost
exactly the way adds were.
I'd add a `hasStagedUpdateUnder` twin over `u.updates` and call it alongside
this one, plus a test that registers an update on a grandchild then deletes the
ancestor. Otherwise a reader sees the add case fixed and reasonably concludes
the whole class is closed.
##########
table/update_schema.go:
##########
@@ -265,6 +265,38 @@ func (u *UpdateSchema) findParentID(fieldID int) int {
return parentID
}
+// findDeletedAncestor walks fieldID and all of its ancestors (following the
parent chain
+// up to the table root) and returns the id of the first field staged for
deletion, if any.
+//
+// This prevents adding a column under a deleted ancestor, even when the
+// immediate parent is still present.
+func (u *UpdateSchema) findDeletedAncestor(fieldID int) (int, bool) {
+ for id := fieldID; id != TableRootID; id = u.findParentID(id) {
+ if u.isDeleted(id) {
+ return id, true
+ }
+ }
+
+ return TableRootID, false
+}
+
+// hasStagedAddUnder reports whether any column addition is staged
+// beneath fieldID or any of its descendants.
+func (u *UpdateSchema) hasStagedAddUnder(fieldID int) bool {
+ for addParentID, added := range u.adds {
+ if len(added) == 0 {
+ continue
+ }
+ for id := addParentID; id != TableRootID; id =
u.findParentID(id) {
Review Comment:
The termination here is correct but silent and load-bearing in two ways. The
`id != TableRootID` bound means a top-level pending add (parent == root) never
matches, so `AddColumn(["new_top"])` then `DeleteColumn(["unrelated_top"])`
correctly succeeds — and separately, `findParentID` returns `TableRootID` for
ids it doesn't know while `u.parentID` is only seeded from the committed
schema, so a freshly-assigned add id exits the walk immediately rather than
looping forever.
Both are right today, but they're resting on an undocumented coupling
between the sentinel and this bound. I'd add a one-line comment naming the
invariant and a test for the root-level-add-then-unrelated-delete case, so this
can't quietly regress into either a false block or a spin if the bound ever
changes.
##########
table/update_schema_test.go:
##########
@@ -407,6 +407,197 @@ func TestAddColumn(t *testing.T) {
assert.ErrorIs(t, err, iceberg.ErrInvalidSchema)
assert.Contains(t, err.Error(), "is not supported until v3")
})
+
+ t.Run("test add column under a deleted parent is rejected", func(t
*testing.T) {
+ table := New([]string{"id"}, testMetadata, "", nil, nil)
+ txn := table.NewTransaction()
+
+ _, err := NewUpdateSchema(txn, true, true).
+ DeleteColumn([]string{"address"}).
+ AddColumn([]string{"address", "code"},
iceberg.PrimitiveTypes.String, "", false, nil).
+ Apply()
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "cannot add to a column that
will be deleted: address")
+ })
+
+ t.Run("test add column under a deleted nested parent is rejected",
func(t *testing.T) {
+ schema := iceberg.NewSchema(1,
Review Comment:
This inline schema is the same three-level shape as
`threeLevelStructSchema()` you add lower in the file. Could this subtest use
`txnForSchema(t, threeLevelStructSchema())` like the table-driven cases do?
Keeps the one fixture in one place.
##########
table/update_schema_test.go:
##########
@@ -407,6 +407,197 @@ func TestAddColumn(t *testing.T) {
assert.ErrorIs(t, err, iceberg.ErrInvalidSchema)
assert.Contains(t, err.Error(), "is not supported until v3")
})
+
+ t.Run("test add column under a deleted parent is rejected", func(t
*testing.T) {
+ table := New([]string{"id"}, testMetadata, "", nil, nil)
+ txn := table.NewTransaction()
+
+ _, err := NewUpdateSchema(txn, true, true).
+ DeleteColumn([]string{"address"}).
+ AddColumn([]string{"address", "code"},
iceberg.PrimitiveTypes.String, "", false, nil).
+ Apply()
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "cannot add to a column that
will be deleted: address")
+ })
+
+ t.Run("test add column under a deleted nested parent is rejected",
func(t *testing.T) {
+ schema := iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int32, Required: true},
+ iceberg.NestedField{ID: 2, Name: "outer", Required:
false, Type: &iceberg.StructType{
+ FieldList: []iceberg.NestedField{
+ {ID: 3, Name: "inner", Required: false,
Type: &iceberg.StructType{
+ FieldList:
[]iceberg.NestedField{
+ {ID: 4, Name: "leaf",
Type: iceberg.PrimitiveTypes.String, Required: false},
+ },
+ }},
+ },
+ }},
+ )
+ meta, err := NewMetadata(schema, nil, UnsortedSortOrder, "",
nil)
+ require.NoError(t, err)
+
+ table := New([]string{"id"}, meta, "", nil, nil)
+ txn := table.NewTransaction()
+
+ _, err = NewUpdateSchema(txn, true, true).
+ DeleteColumn([]string{"outer", "inner"}).
+ AddColumn([]string{"outer", "inner", "new_leaf"},
iceberg.PrimitiveTypes.String, "", false, nil).
+ Apply()
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "cannot add to a column that
will be deleted: outer.inner")
+ })
+
+ t.Run("test add column then delete its parent is rejected", func(t
*testing.T) {
+ // Reverse operation order: staging the child add before the
parent delete.
+ // This path is caught by deleteColumn's "field that has
additions cannot be deleted" guard,
+ // The two guards are symmetric and neither order can silently
drop data.
+ table := New([]string{"id"}, testMetadata, "", nil, nil)
+ txn := table.NewTransaction()
+
+ _, err := NewUpdateSchema(txn, true, true).
+ AddColumn([]string{"address", "code"},
iceberg.PrimitiveTypes.String, "", false, nil).
+ DeleteColumn([]string{"address"}).
+ Apply()
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "field that has additions
cannot be deleted: address")
+ })
+
+ // Invariant test: should not regress
+ t.Run("test delete nested field then re-add same name under same parent
is allowed", func(t *testing.T) {
+ table := New([]string{"id"}, testMetadata, "", nil, nil)
+ txn := table.NewTransaction()
+
+ newSchema, err := NewUpdateSchema(txn, true, true).
+ DeleteColumn([]string{"address", "city"}).
+ AddColumn([]string{"address", "city"},
iceberg.PrimitiveTypes.Int64, "recreated", false, nil).
+ Apply()
+ require.NoError(t, err)
+
+ addressField, ok := newSchema.FindFieldByName("address")
+ require.True(t, ok)
+ structType, ok := addressField.Type.(*iceberg.StructType)
+ require.True(t, ok)
+
+ fields := structType.Fields()
+ require.Len(t, fields, 2)
+
+ cityField, ok := newSchema.FindFieldByName("address.city")
+ require.True(t, ok)
+ assert.Equal(t, iceberg.PrimitiveTypes.Int64, cityField.Type)
+ assert.Equal(t, "recreated", cityField.Doc)
+ // The re-added field must receive a fresh id, not reuse the
deleted one.
+ assert.NotEqual(t, 5, cityField.ID)
Review Comment:
`NotEqual(t, 5, cityField.ID)` proves the id isn't reused, which is the
important part. While we're here, `assert.Greater(t, cityField.ID,
testMetadata.LastColumnID())` would also pin the monotonicity invariant —
otherwise a future regression that hands out some other stale-but-different id
still passes this.
--
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]
