fix(spec): allow void partition fields to reuse their source column name - #3215
fix(spec): allow void partition fields to reuse their source column name#3215dchahla wants to merge 2 commits into
Conversation
A partition field whose name matches a schema column is accepted only for the identity transform. Void belongs in the same allowance: it always produces null, so it cannot carry a value that disagrees with the column it shares a name with. Rejecting it blocks the usual way a v1 table drops a partition field, which is to rewrite the field's transform from identity to void while leaving its name and source alone, so a spec that was valid before the edit stops binding after it. The rule is enforced in two places, PartitionSpecBuilder and TableMetadataBuilder, and both are updated so they agree. In both paths a void field whose name matches a column it is not sourced from is still rejected, and transforms other than identity and void are still rejected on a name collision. The two error messages named the identity transform and would have been misleading once void is permitted, so they now name both.
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
The core change looks right to me — extending the name-collision allowance to void, gated on a matching source_id, mirrors Java's checkAndAddPartitionName exactly, and mirroring the rule in both PartitionSpecBuilder and TableMetadataBuilder is the correct instinct. void always producing null is the right reason the reuse is safe.
I'd hold this briefly before merging though. The main thing: the "unblocks the v1 drop partition field workflow" claim is only half-true as written. The new evolution test pins field_id: Some(1000) and never goes through build() — but for a real v1 caller that pin isn't a test convenience. reuse_partition_field_ids keys on (source_id, transform), so once the transform flips to void the old id isn't reused, the builder assigns a fresh one, and the sequential-id gate rejects the single-field spec. So the workflow still fails unless the caller carries the old field_id forward — and that failure lands in a completely different validation layer. I'd either add a case that goes through build() (and one that omits the pin to show the sequential-id error), or note the remaining constraint in the description.
A few things I'd want before merge:
- the v1-workflow caveat above — a test through
build()or a description note - the function docstring on
check_name_does_not_collide_with_schemastill says rule 2 is "identity" only - assert on the error messages in the new negative cases so they can't silently stop exercising the rule
The rest (the comment wording, error-string alignment, a cross-ref between the two predicate sites) is small and inline.
One thing that's explicitly not this PR: Java doesn't require a valid source at all for void, so it'll write a void field whose source_id points at a schema-absent column, which we reject earlier in check_transform_compatibility. That divergence predates this change — might be worth a follow-up issue, but I wouldn't touch it here.
Once those are addressed, happy to take another pass and approve.
|
|
||
| // Rewriting the identity field to void is how a v1 table drops a partition field. | ||
| builder() | ||
| .add_partition_spec(spec(1, Transform::Void)) |
There was a problem hiding this comment.
This proves add_partition_spec succeeds, but the spec closure pins field_id: Some(1000) and we never go through build().
For a real v1 drop that pin isn't a test convenience — reuse_partition_field_ids keys on (source_id, transform), so once the transform flips to void the old id isn't reused, the builder assigns a fresh one, and the sequential-id gate rejects the single-field spec. So "unblocks the v1 drop partition field workflow" only holds if the caller carries the old field_id forward.
I'd add a case that omits the pin to show the sequential-id error is what fires, and call out the pinning requirement in the description. wdyt?
| // A void transform always produces null, so like identity it cannot carry a | ||
| // value that disagrees with the schema column it shares a name with. Rewriting | ||
| // an identity field to void is how a v1 table drops a partition field. | ||
| if matches!(field.transform, Transform::Identity | Transform::Void) { |
There was a problem hiding this comment.
The function doc comment just above (rule 2) still reads "AND the transformation is identity" — it doesn't mention void, so it now contradicts this branch. I'd update rule 2 to "identity or void" so the docstring matches the code.
| name: "id".to_string(), | ||
| transform: Transform::Void, | ||
| }) | ||
| .unwrap_err(); |
There was a problem hiding this comment.
This unwrap_err() doesn't check why it failed, so it can't distinguish a name-collision rejection from any other error on that path. Same for the bind case below (:1359) and the two evolution cases in table_metadata_builder.rs (:3214, :3219).
It happens to fail for the right reason today because add_unbound_field reaches check_name_does_not_collide_with_schema first, but a reordering of the checks would let these pass while silently no longer exercising the rule.
I'd assert on the message — contains("sourced from different field") for the source-id mismatch and contains("identity or void transform") for the wrong-transform case — matching the existing test_collision_with_schema_name pattern.
| match schema.field_by_name(field.name.as_str()) { | ||
| Some(schema_collision) => { | ||
| if field.transform == Transform::Identity { | ||
| // A void transform always produces null, so like identity it cannot carry a |
There was a problem hiding this comment.
The "like identity it cannot carry a value that disagrees" framing is a little off — identity does carry the source column's value, that's the whole point; void just always produces null. The reason the allowance is safe is that both keep the name/source_id pairing internally consistent (and Java treats them identically in checkAndAddPartitionName). I'd reword so it doesn't imply identity is value-free.
| ErrorKind::DataInvalid, | ||
| format!( | ||
| "Cannot create identity partition sourced from different field in schema. Field name '{}' has id `{}` in schema but partition source id is `{}`", | ||
| "Cannot create partition sourced from different field in schema. Field name '{}' has id `{}` in schema but partition source id is `{}`", |
There was a problem hiding this comment.
Dropping "identity" here makes the message generic — it no longer tells the user which transforms trigger the check, and the sibling message on the wrong-transform branch does name "identity or void transform." I'd keep the qualifier: "Cannot create identity or void partition sourced from different field...".
While we're aligning strings, the two "conflicts with schema field" messages also differ by a stray colon (partition.rs:624 has name: '{}', table_metadata_builder.rs:805 has name '{}') — worth unifying since both are public-facing.
| partition_field.transform == crate::spec::Transform::Identity; | ||
| // A void transform always produces null, so like identity it cannot carry a | ||
| // value that disagrees with the schema column it shares a name with. | ||
| let is_allowed_transform = matches!( |
There was a problem hiding this comment.
This "allowed transforms for name-sharing" predicate now lives in two files (here and check_name_does_not_collide_with_schema in partition.rs) with no compile-time link, so a future third transform has to be added in both by hand. A one-line cross-ref comment ("keep in sync with partition.rs") would cost nothing.
Minor while we're here: partition.rs uses the imported Transform short name — adding Transform to the use block here would let this read matches!(..., Transform::Identity | Transform::Void) and match the other site.
2 participants
A partition field whose name matches a schema column is accepted only for the identity transform. Void belongs in the same allowance: it always produces null, so it cannot carry a value that disagrees with the column it shares a name with. Rejecting it blocks the usual way a v1 table drops a partition field, which is to rewrite the field's transform from identity to void while leaving its name and source alone, so a spec that was valid before the edit stops binding after it.
Which issue does this PR close?
check_name_does_not_collide_with_schemaincorrectly rejects Void transform partitions whose names collide with schema columns #2567What changes are included in this PR?
The rule is enforced in two places, PartitionSpecBuilder and TableMetadataBuilder, and both are updated so they agree. In both paths a void field whose name matches a column it is not sourced from is still rejected, and transforms other than identity and void are still rejected on a name collision.
The two error messages named the identity transform and would have been misleading once void is permitted, so they now name both.
Are these changes tested?
unit tests added alongside the existing collision tests in each file.
spec::partition::tests::test_builder_collision_is_ok_for_void_transformscovers thePartitionSpecBuilderpath: a void field reusing the name of the column it is sourced fromnow binds, the allowance is not specific to one column, and a void field named after a column
it is not sourced from is still rejected.
spec::partition::tests::test_bind_collision_is_ok_for_void_transformscovers the same rulereached through
UnboundPartitionSpec::bind, which is the path a spec takes when it is editedand re-bound.
spec::table_metadata_builder::tests::test_partition_spec_evolution_allows_void_reusing_its_source_column_namecovers the metadata path end to end. It builds v1 metadata partitioned by an identity field
named after its source column, then adds a spec that rewrites that field to void. It also
checks that a mismatched source id and a bucket transform are still rejected. The partition
field id is pinned so the v1 sequential-id rule is not what decides those cases.
test_partition_spec_evolution_validates_schema_field_name_conflictsalready asserted on theerror text, so its assertion was updated to the new wording.
cargo test -p iceberg --lib spec::passes with 465 tests.cargo fmt --checkandcargo clippy -p iceberg --all-targetsare clean on the toolchain pinned byrust-toolchain.toml.AI Disclosure