Repository navigation
fix(spec): accept partition specs produced by spec evolution - #3431
dhruvarya-db wants to merge 1 commit into
Conversation
Partition spec evolution produces specs that the spec builders rejected: - A source column can have more than one partition field. Adding hour(ts) to a spec that has day(ts) is allowed, and format version 1 tables keep each removed field as a `void` placeholder. Binding an unbound spec (`UnboundPartitionSpec::bind`, `PartitionSpecBuilder::new_from_unbound`, and so `TableMetadataBuilder::add_partition_spec`) and loading the legacy v1 `partition-spec` field ran the authoring-time redundancy check and rejected these specs. Both now skip it, as Java does. `PartitionSpecBuilder::add_unbound_field` and `UnboundPartitionSpecBuilder::add_partition_field` still reject redundant fields. - Removing identity(category) from a format version 1 spec leaves a void field named `category`. Both the spec builder and the table metadata builder allowed only identity fields to share the name of their source column. Void fields may now do the same, matching Java's `alwaysNull`. Co-authored-by: Isaac <no-reply@databricks.com>
There was a problem hiding this comment.
Thanks for this, helping out with a short review. I checked it locally against edc4b74, and it holds up:
- I replayed real iceberg-java 1.12.0 evolutions as
AddSpeccommits: day(ts) plus hour(ts) on v2, and removing, re-adding, then removing identity(category) on v1. All four fail on main and pass here. Loading legacy v1 partition-spec metadata with two void fields does too. - This also fixes more than
AddSpec. build() re-validates the default spec on every commit, so on main, any commit to a table whose spec has both day(ts) and hour(ts) fails, even a property change. With this change it succeeds. I think this is worth mentioning in the main description - Each of the three changes is pinned: reverting any one of them fails a test
- The iceberg lib suite, the catalog unit tests, clippy and fmt pass, and the branch merges cleanly with main
The void-name part overlaps with #3215, which makes the same change and closes #2567. The #2567 repro also passes on this branch, so it might be worth linking both here, so that whichever lands second can drop that part.
For context, one related v1 case still fails, both on main and here: removing identity(category) and then dropping the category column. Java's next spec keeps void fields for the dropped column, and committing it fails with Cannot find partition source field with id 2. (This would be a separate issue/fix)
One small nit inline.
| match schema.field_by_name(field.name.as_str()) { | ||
| Some(schema_collision) => { | ||
| if field.transform == Transform::Identity { | ||
| if matches!(field.transform, Transform::Identity | Transform::Void) { |
There was a problem hiding this comment.
Small one: this branch now covers void too, but the error just below still says "Cannot create identity partition sourced from different field". So does the copy in validate_partition_field_names (table_metadata_builder.rs:769). I'd prefer both say "identity or void partition", which matches what was suggested on #3215. A one-line "keep in sync" note between the two Identity | Void checks might also help.
This branch has not been deployed
3 participants
Which issue does this PR close?
What changes are included in this PR?
Catalogs built on iceberg-rust apply an
AddSpecupdate throughTableMetadataBuilder::add_partition_spec, which binds the spec with the same checks used to author a new one. Those checks reject specs that partition spec evolution legitimately produces, and that Java writes and accepts:updateSpec().addField(hour("ts"))on a table partitioned byday(ts)produces a spec with bothts_dayandts_hour. On a format version 1 table, each removed field stays as avoidplaceholder, so one column can have several void fields. Binding ran the authoring-time redundancy check, soUnboundPartitionSpec::bind,PartitionSpecBuilder::new_from_unboundandTableMetadataBuilder::add_partition_specrejected these specs. Loading v1 metadata that has only the legacypartition-specfield did too. Both now skip the check, as Java'sPartitionSpec.Builder.adddoes.PartitionSpecBuilder::add_unbound_fieldandUnboundPartitionSpecBuilder::add_partition_fieldstill reject redundant fields when authoring a spec.identity(category)from a v1 spec in Java leaves a void field namedcategory. Both the spec builder andTableMetadataBuilder::validate_partition_field_namesallowed only identity fields to share their source column's name. Void fields may now do the same, matching Java'salwaysNull(source, name). Java goes further and allows any transform named after its own source column; this PR doesn't.Metadata with
partition-specsalready loaded, because that field is deserialized without rebinding. Only committing these specs failed.#3413 also edits
validate_partition_field_names, so whichever of the two lands second needs a small rebase.Are these changes tested?
Yes, with unit tests:
TableMetadataBuilder::add_partition_specaccepting the void placeholders of a v1 table;partition-specmetadata with two void fields on one column.Each fails without the fix.
AI Disclosure
This PR was written with Claude Opus 5.5 and reviewed by me.