You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "badalprasadsingh (via GitHub)" <gi...@apache.org> on 2026/08/06 20:41:04 UTC
[PR] fix(table): reject AddColumn/UnionByNameWith targeting a column staged for deletion [iceberg-go]
badalprasadsingh opened a new pull request, #1688:
URL: https://github.com/apache/iceberg-go/pull/1688
### Description
Fixes #1687
Right now, `UpdateSchema.addColumn` and `unionAddColumn` resolved the parent field against the original schema without checking whether that parent was already staged for deletion in the same builder chain. This let `Apply()`/`Commit()` succeed while silently dropping the newly added nested column, because `applyChanges.Field` drops deleted fields (and everything staged under them) before considering pending additions.
Added an explicit `u.isDeleted(parentID)` check right after resolving the parent field in both `addColumn` and `unionAddColumn`, returning an error instead of silently staging an addition that will be discarded.
### Testing
Added: the required **regression** tests for it along with, a **symmetry** test and an, **invariant** test
--
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): reject AddColumn/UnionByNameWith targeting a column staged for deletion [iceberg-go]
Posted by "badalprasadsingh (via GitHub)" <gi...@apache.org>.
badalprasadsingh commented on PR #1688:
URL: https://github.com/apache/iceberg-go/pull/1688#issuecomment-5209776913
Addressed. Added shared ancestor traversal, which now prevents the silent-loss in both the operation orders.
Thanks for the review @zeroshade 🤩
--
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): reject AddColumn/UnionByNameWith targeting a column staged for deletion [iceberg-go]
Posted by "badalprasadsingh (via GitHub)" <gi...@apache.org>.
badalprasadsingh commented on code in PR #1688:
URL: https://github.com/apache/iceberg-go/pull/1688#discussion_r3732324562
##########
table/update_schema.go:
##########
@@ -746,6 +750,10 @@ func (u *UpdateSchema) unionAddColumn(path []string, newField iceberg.NestedFiel
}
parentID = parentField.ID
+
+ if u.isDeleted(parentID) {
Review Comment:
Done.
Now, `unionAddColumn` uses `findDeletedAncestor` and names the deleted ancestor. Covered by `TestUnionByNameRejectsAddUnderDeletedAncestor` in both orders (delete-then-union and union-then-delete).
--
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): reject AddColumn/UnionByNameWith targeting a column staged for deletion [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1688:
URL: https://github.com/apache/iceberg-go/pull/1688#discussion_r3732111537
##########
table/update_schema.go:
##########
@@ -320,6 +320,10 @@ func (u *UpdateSchema) addColumn(path []string, fieldType iceberg.Type, doc stri
}
parentID = parentField.ID
+
+ if u.isDeleted(parentID) {
Review Comment:
This checks only the immediate parent. If `outer` is staged for deletion and the add targets `outer.inner.new_leaf`, `parentID` is `inner`, so this passes and the later schema application silently discards the requested addition with `outer`. **Suggested fix:** walk all ancestors of the resolved parent, reject if any is staged for deletion, and report the actual deleted ancestor.
##########
table/update_schema.go:
##########
@@ -746,6 +750,10 @@ func (u *UpdateSchema) unionAddColumn(path []string, newField iceberg.NestedFiel
}
parentID = parentField.ID
+
+ if u.isDeleted(parentID) {
Review Comment:
`UnionByNameWith` has the same immediate-parent gap: an addition below a descendant of a deleted ancestor is accepted and then silently lost. **Suggested fix:** apply the same full ancestor-deletion check here and report the deleted ancestor rather than only the immediate parent 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): reject AddColumn/UnionByNameWith targeting a column staged for deletion [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: 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): reject AddColumn/UnionByNameWith targeting a column staged for deletion [iceberg-go]
Posted by "badalprasadsingh (via GitHub)" <gi...@apache.org>.
badalprasadsingh commented on PR #1688:
URL: https://github.com/apache/iceberg-go/pull/1688#issuecomment-5218401926
Will merge the #1686 into this, and create a single PR.
Drafting it now. Please review it again once, I re-open 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: 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): reject AddColumn/UnionByNameWith targeting a column staged for deletion [iceberg-go]
Posted by "badalprasadsingh (via GitHub)" <gi...@apache.org>.
badalprasadsingh commented on code in PR #1688:
URL: https://github.com/apache/iceberg-go/pull/1688#discussion_r3732321834
##########
table/update_schema.go:
##########
@@ -320,6 +320,10 @@ func (u *UpdateSchema) addColumn(path []string, fieldType iceberg.Type, doc stri
}
parentID = parentField.ID
+
+ if u.isDeleted(parentID) {
Review Comment:
Done.
Now, `findDeletedAncestor` walks the full ancestor chain and reports the deleted ancestor's name.
The case: `delete outer`, add `outer.inner.new_leaf` : now errors `cannot add to a column that will be deleted: outer`. This is now covered by `TestAddColumnUnderDeletedAncestor` (delete-then-add, all nesting kinds).
--
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