mattfaltyn commented on code in PR #1905:
URL: https://github.com/apache/iceberg-go/pull/1905#discussion_r3875553585
##########
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:
> “fix `moveFields` to not drop on `!found`”
Great catch — `moveFields` now fails closed for missing sources or targets,
with source-delete and direct target-loss coverage.
##########
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:
> “split it by the distinguishing text”
Good suggestion — the two orderings now assert `has been deleted` and
`cannot be deleted` separately.
##########
table/update_schema_test.go:
##########
@@ -887,6 +887,47 @@ func TestMoveColumn(t *testing.T) {
})
}
+func TestMoveRelativeToDeletedColumnRejected(t *testing.T) {
Review Comment:
> “add a positive case”
Agreed — unrelated deletion plus move now succeeds and asserts the final
field order.
--
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]
