laskoviymishka commented on code in PR #1905:
URL: https://github.com/apache/iceberg-go/pull/1905#discussion_r3874718644
##########
table/update_schema.go:
##########
@@ -388,6 +388,11 @@ func (u *UpdateSchema) deleteColumn(path []string) error {
return fmt.Errorf("field that has updates cannot be
deleted: %s", fullName)
}
}
+ for _, move := range u.moves[parentID] {
Review Comment:
the two guards close both orderings in #1903, but they're patching entry
points — the real silent drop is in `moveFields`, where the `!found { continue
}` branch (~line 1370) removes the field from the working slice and then bails
with no error. this PR makes that branch unreachable for the tested orderings,
but it's still a landmine for any future path.
and there's already one it doesn't cover: `MoveBefore(["age"], ["name"])`
then `DeleteColumn(["age"])`. this guard only inspects `move.RelativeTo`, not
`move.FieldID`, so neither guard fires and age's staged move is silently
swallowed at apply (output happens to be correct, but there's no diagnostic).
I'd rather fix `moveFields` to not drop on `!found` — restore the field and
continue, or return an error — so the root is closed in one place instead of
enumerating call-site orderings. wdyt?
##########
table/update_schema_test.go:
##########
@@ -887,6 +887,47 @@ func TestMoveColumn(t *testing.T) {
})
}
+func TestMoveRelativeToDeletedColumnRejected(t *testing.T) {
Review Comment:
all four cases expect an error, so this proves the guards fire but not that
they fire only when they should. I'd add a positive case —
`DeleteColumn(["name"])` then `MoveBefore(["age"], ["id"])` should still
succeed and land in the expected order — so an over-eager guard can't slip
through unnoticed.
##########
table/update_schema_test.go:
##########
@@ -887,6 +887,47 @@ func TestMoveColumn(t *testing.T) {
})
}
+func TestMoveRelativeToDeletedColumnRejected(t *testing.T) {
+ tests := []struct {
+ name string
+ build func(*UpdateSchema) *UpdateSchema
+ }{
+ {
+ name: "delete then move before",
+ build: func(update *UpdateSchema) *UpdateSchema {
+ return
update.DeleteColumn([]string{"name"}).MoveBefore([]string{"age"},
[]string{"name"})
+ },
+ },
+ {
+ name: "move before then delete",
+ build: func(update *UpdateSchema) *UpdateSchema {
+ return update.MoveBefore([]string{"age"},
[]string{"name"}).DeleteColumn([]string{"name"})
+ },
+ },
+ {
+ name: "delete then move after",
+ build: func(update *UpdateSchema) *UpdateSchema {
+ return
update.DeleteColumn([]string{"name"}).MoveAfter([]string{"age"},
[]string{"name"})
+ },
+ },
+ {
+ name: "move after then delete",
+ build: func(update *UpdateSchema) *UpdateSchema {
+ return update.MoveAfter([]string{"age"},
[]string{"name"}).DeleteColumn([]string{"name"})
+ },
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ tbl := New([]string{"id"}, testMetadata, "", nil, nil)
+ _, err :=
tt.build(NewUpdateSchema(tbl.NewTransaction(), true, true)).Apply()
+ require.ErrorContains(t, err, "move target")
Review Comment:
these assertions can't tell the two guards apart — both error messages
contain "move target", so cases 1/3 exercise the `moveColumn` guard and 2/4 the
`deleteColumn` guard, but all four pass on the same substring. if one guard
broke and the other picked up the slack, this would stay green.
I'd split it by the distinguishing text: assert `"cannot be deleted"` for
the move-then-delete cases and `"has been deleted"` for the delete-then-move
cases.
##########
table/update_schema_test.go:
##########
@@ -887,6 +887,47 @@ func TestMoveColumn(t *testing.T) {
})
}
+func TestMoveRelativeToDeletedColumnRejected(t *testing.T) {
+ tests := []struct {
+ name string
+ build func(*UpdateSchema) *UpdateSchema
+ }{
+ {
+ name: "delete then move before",
+ build: func(update *UpdateSchema) *UpdateSchema {
+ return
update.DeleteColumn([]string{"name"}).MoveBefore([]string{"age"},
[]string{"name"})
Review Comment:
these are all flat top-level fields, so the `deleteColumn` guard only ever
scans `u.moves[-1]` (parentID resolves to the root). a nested target like
`address.city` keys its move under `address`'s ID — a different bucket that's
currently untested. one nested case would pin that path down.
--
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]
