ShivamSharma43 opened a new pull request, #2523:
URL: https://github.com/apache/iceberg-go/pull/2523
Deleting a column that is an identifier field succeeded and silently removed
it from the schema's identifier fields. For example,
`DeleteColumn([]string{"id"})` on a table whose identifier field is `id`
produced a schema with no identifier fields and no error.
### Cause
`deleteColumn` called `delete(u.identifierFieldNames, fullName)`, so `Apply`
never saw the deleted identifier field. That only removed an exact name match.
Deleting the struct that contains an identifier field, or deleting the column
with a different case in case-insensitive mode, already failed in `Apply` with
`identifier field not found`.
### Fix
`deleteColumn` no longer changes the identifier field names. The existing
check in `Apply` now rejects a deleted identifier field, unless
`SetIdentifierField` replaces the identifier fields in the same update (in
either order). This matches Java's
[`SchemaUpdate.applyChanges`](https://github.com/apache/iceberg/blob/52fc764127402ade8170b1b53ddd95fde13cb75c/core/src/main/java/org/apache/iceberg/SchemaUpdate.java#L541-L562)
("To force deletion, also call setIdentifierFields to update identifier
fields") and its `testDeleteIdentifierFieldColumns*` tests. PyIceberg's
`delete_column` also leaves the identifier names alone, and `_apply` fails the
same way.
The `Apply` error now says that deleting an identifier field also needs
`SetIdentifierField`. It still starts with `identifier field not found: <name>`.
One difference from Java remains. If an identifier column is deleted and a
new column with the same name is added in the same update, the identifier moves
to the new column (PyIceberg does this too). Java rejects it, because it checks
deletes against the old schema. If you'd rather match Java there as well, I'm
happy to add that check.
### Testing
- New `TestDeleteIdentifierColumn`: deleting the identifier column fails,
deleting it and then clearing the identifier fields succeeds, and replacing the
identifier fields and then deleting the old one succeeds. The first case fails
on main.
- The existing `TestSetIdentifierField` cases pass unchanged.
- `go vet` and `golangci-lint run ./table/` (v2.14.0) are clean. `go test
./table/...` passes except for `TestWriteMapType`, two
`TestRewriteDataFiles_PartialProgress*` tests and
`TestGetReferencedFiles_OverwriteThenExpireExcludesTombstones`. These fail on
my Windows machine because of path handling, and they fail in exactly the same
way on unmodified main.
Closes #2522
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
