zeroshade commented on code in PR #2523:
URL: https://github.com/apache/iceberg-go/pull/2523#discussion_r4541233893
##########
table/update_schema.go:
##########
@@ -1135,7 +1135,7 @@ func (u *UpdateSchema) Apply() (*iceberg.Schema, error) {
field, ok =
newSchema.FindFieldByNameCaseInsensitive(name)
}
if !ok {
- return nil, fmt.Errorf("identifier field not found:
%s", name)
+ return nil, fmt.Errorf("identifier field not found: %s
(to delete an identifier field, also update the identifier fields with
SetIdentifierField)", name)
Review Comment:
On the open question in the description: please match Java.
[`SchemaUpdate.applyChanges`](https://github.com/apache/iceberg/blob/52fc764127402ade8170b1b53ddd95fde13cb75c/core/src/main/java/org/apache/iceberg/SchemaUpdate.java#L541-L562)
looks up each identifier name in the *original* schema and rejects it if that
field is in `deletes`. Two reasons to do the same here:
- The error says what actually happened. "identifier field not found: id (to
delete …)" reads like a typo in the name, not a deleted column.
- Deleting an identifier column and adding a new one with the same name
would otherwise move the identifier to a different field ID whose existing rows
have no values. Java rejects that.
For example, before the `newSchema` lookup (honouring `u.caseSensitive` as
below):
```go
old, ok := u.schema.FindFieldByName(name) // or
FindFieldByNameCaseInsensitive
if ok {
if _, deleted := u.deletes[old.ID]; deleted {
return nil, fmt.Errorf("cannot delete identifier field %s: to
force deletion, also update the identifier fields with SetIdentifierField",
name)
}
}
```
A renamed identifier won't be found by its new name in the old schema, so it
skips this check, as in Java. With this in place, the "not found" message can
go back to its original wording. Please add a delete-then-add-same-name case to
`TestDeleteIdentifierColumn` too.
--
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]
