Repository navigation
fix(spec): Skip the partition name check once the source column is dropped - #3420
Open
xanderbailey wants to merge 2 commits into
Open
xanderbailey wants to merge 2 commits into
xanderbailey wants to merge 2 commits into
Conversation
…opped
`validate_schema_field_names` rejected any 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 a name used by a spec
whose source column has since been dropped stayed reserved even though
the spec is only retained to interpret old metadata.
Iceberg Java runs the same check, in `checkAndAddPartitionName`, 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:
boolean sourceFieldExists = schema.findField(sourceColumnId) != null;
if (sourceFieldExists) {
Preconditions.checkArgument(
schemaField == null || schemaField.fieldId() == sourceColumnId, ...);
}
Adopt that rule. It keeps every spec in scope, so nothing that was
rejected before is accepted now unless its source column is gone, and it
drops the `name_exists_in_any_schema` escape hatch, which was a
Rust-only approximation of the identity-partition case.
The check now reads partition fields rather than schema names, so its
result no longer depends on whether the schema or the spec is added
first.
`partition_name_exists` loses its only caller and is removed.
Note a related divergence this does not address: `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.
xanderbailey
marked this pull request as draft
October 12, 2026 21:32
xanderbailey
marked this pull request as ready for review
October 13, 2026 12:57
JeonDaehong
reviewed
Oct 13, 2026
JeonDaehong
left a comment
JeonDaehong
left a comment
Contributor
There was a problem hiding this comment.
Thanks for this! I wanted to double-check the part about the result no longer depending on order, so I tried a few orderings on Java (core at 059e7ea94, JDK 21) and on this branch.
Setup: an unpartitioned data: string table, spec bucket(data, 16) AS bucket_data, and a new schema data, bucket_data (id 2).
| order | Java | this PR (and main) |
|---|---|---|
| add schema → add spec → set current schema | rejected | accepted |
| add spec → add current schema | rejected | rejected |
| add spec → add schema (not current) | accepted | rejected |
As far as I can tell, Java only runs this check in setCurrentSchema, when it rebuilds every spec against the new schema. Here it runs in add_schema, so the first order gets through and the third is stricter than Java. Running the check from set_current_schema instead would line up with Java in all three cases. The first one already happens on main, though, so a follow-up is fine if you'd rather keep this PR focused.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Working towards #3056
Supersedes #3412, which scoped the check to the current spec. That was not correct - see @mbutrovich's review there.
What changes are included in this PR?
validate_schema_field_namesrejected 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, reached fromBuilder.addassetCurrentSchemarebuilds 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.
name_exists_in_any_schemais dropped now, it was our own 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.
Not addressed
build()rebinds the default spec withbind(), which requires the source column to exist, so dropping the source column of the current spec still fails. Java usesbuildUnchecked()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:AI Disclosure
Claude Code was used to trace the Java call paths, write the tests. I reviewed the result.