xanderbailey opened a new pull request, #3420: URL: https://github.com/apache/iceberg-rust/pull/3420
## Which issue does this PR close? - Closes #3411 Working towards #3056 Supersedes #3412, which scoped the check to the current spec. That was the wrong axis — see @mbutrovich's review there. ## What changes are included in this PR? `validate_schema_field_names` rejected a new schema field whose name matched a partition field name in *any* spec, unless some schema already had that name. Iceberg keeps specs forever, so the name stayed reserved even when the spec was only retained to interpret old metadata. Java runs an equivalent check in [`checkAndAddPartitionName`](https://github.com/apache/iceberg/blob/4eb9cb7c3d53cb8080724eebf6e6898871ee23b0/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L407-L427), reached from `Builder.add` as `setCurrentSchema` rebuilds every spec. It skips a field whose source column is absent from the new schema, and otherwise allows the name only for the source column itself. This PR adopts that rule. Every spec stays in scope, so nothing previously rejected is accepted now unless its source column is gone. The `name_exists_in_any_schema` escape hatch is dropped — it was a Rust-only approximation of the identity-partition case, and it allowed a name that an old schema had even when it resolved to a different field in the new one. The check now iterates partition fields rather than schema names, so the result no longer depends on whether the schema or the spec is added first. `partition_name_exists` loses its only caller and is removed, along with its two unit tests. ### Not addressed `build()` rebinds the default spec with `bind()`, which requires the source column to exist, so dropping the source column of the *current* spec still fails. Java uses `buildUnchecked()` there and tolerates it. Happy to follow up separately if that's worth matching. ## Are these changes tested? Unit tests in `table_metadata_builder.rs`: - `..._allows_a_superseded_partition_field_name_once_its_source_is_dropped` — the #3411 case, corrected: the source column is dropped. Fails on `main`. - `..._rejects_a_partition_field_name_while_its_source_remains` — both with spec 0 default and superseded. Passes on `main` too; it pins that this PR does not loosen the case Java rejects. - `..._partition_name_check_is_order_independent` — drops the column and the partitioning in one set of changes, both orders. Fails on `main`. The three existing `test_schema_evolution_*` tests keep their accept/reject outcomes; only their asserted error message changed. ## AI Disclosure Claude Code was used to trace the Java call paths, write the tests, and draft this description. I reviewed the result. -- 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]
