dhruvarya-db opened a new pull request, #3431:
URL: https://github.com/apache/iceberg-rust/pull/3431
## Which issue does this PR close?
- None filed. This fixes a bug that surfaces when committing partition specs
produced by partition spec evolution.
## What changes are included in this PR?
Catalogs built on iceberg-rust apply an `AddSpec` update through
`TableMetadataBuilder::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:
- **Several fields on one source column.** In Java,
`updateSpec().addField(hour("ts"))` on a table partitioned by `day(ts)`
produces a spec with both `ts_day` and `ts_hour`. On a format version 1 table,
each removed field stays as a `void` placeholder, so one column can have
several void fields. Binding ran the authoring-time redundancy check, so
`UnboundPartitionSpec::bind`, `PartitionSpecBuilder::new_from_unbound` and
`TableMetadataBuilder::add_partition_spec` rejected these specs. Loading v1
metadata that has only the legacy `partition-spec` field did too. Both now skip
the check, as Java's `PartitionSpec.Builder.add` does.
`PartitionSpecBuilder::add_unbound_field` and
`UnboundPartitionSpecBuilder::add_partition_field` still reject redundant
fields when authoring a spec.
- **A void field named after its source column.** Removing
`identity(category)` from a v1 spec in Java leaves a void field named
`category`. Both the spec builder and
`TableMetadataBuilder::validate_partition_field_names` allowed only identity
fields to share their source column's name. Void fields may now do the same,
matching Java's `alwaysNull(source, name)`. Java goes further and allows any
transform named after its own source column; this PR doesn't.
Metadata with `partition-specs` already 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:
- binding a spec with two time granularities and two void fields on one
column;
- rejecting a void field named after a different column;
- the authoring path still rejecting a second time granularity;
- `TableMetadataBuilder::add_partition_spec` accepting the void placeholders
of a v1 table;
- loading legacy v1 `partition-spec` metadata 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. Please focus on
whether loosening the binding checks is acceptable. The bind path now accepts
any redundant fields, not only the time-granularity and void cases, which
matches Java.
This pull request and its description were written by Isaac.
--
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]
