Repository navigation
feat(transaction): add UpdatePartitionSpecAction for partition spec evolution - #3441
dhruvarya-db wants to merge 1 commit into
Conversation
…volution Add `Transaction::update_partition_spec()` to add, remove and rename partition fields. The changes are resolved at commit time with the rules of Java's `BaseUpdatePartitionSpec`, so the same changes give the same spec as Java, including default field names and error messages. Field ids are assigned on the client. A new field that matches a field of an earlier spec reuses its id, preferring the highest id when several match; otherwise it takes the next id after `last-partition-id`. The commit sends `AddSpec` (plus `SetDefaultSpec` unless `add_non_default_spec` is used) and asserts the last assigned partition id and default spec id. Format version 1 tables are rejected for now. The spec builder still rejects specs that keep two time granularities of one column, so adding hour(ts) to a day(ts) table fails at commit on catalogs built on iceberg-rust. Co-authored-by: Isaac <no-reply@databricks.com>
JeonDaehong
left a comment
There was a problem hiding this comment.
I ran 21 spec-update scenarios outside the ported TestUpdatePartitionSpec cases through Java's BaseUpdatePartitionSpec(2, spec, lastAssignedFieldId) (1.12.0-SNAPSHOT) and through resolve_spec on this branch, with the same schema and specs as the tests. 16 give the same fields (id, name, transform, source) or both fail, including the case-insensitive add/remove and the rename/delete conflicts.
The differences:
- Re-adding a removed time-transform field in the same update. See inline. Java's result there looks accidental.
- A non-identity field named after its own source column:
bucket[16](id)namedid,month(ts)namedts. Java'sUpdatePartitionSpecaccepts both. This PR rejects them through the spec builder. Java's ownPartitionSpec.builderFor(...).bucket("id", 16, "id")rejects it as well, so Java isn't consistent here, and I don't think this needs to change. I checked that iceberg-rust main still loads a Java-written table with such a spec. hour(ts)next to an existingday(ts): Java accepts it, and this fails as the description says. That confirms #3431 is needed for parity.
| let source_id = self.source_id(source_name)?; | ||
| let key = (source_id, transform); | ||
| if let Some(existing) = self.transform_to_field.get(&key).copied() { | ||
| if self.deletes.remove(&existing.field_id) { |
There was a problem hiding this comment.
This restores the removed field for every transform, which is what Java intends (rewriteDeleteAndAddField). Java only gets there for some of them, though. Its check is existing.transform().equals(...), and for time transforms the bound day and the new day aren't equal, so Java falls through to the duplicate error:
| same update | Java (1.12.0-SNAPSHOT) | here |
|---|---|---|
remove category, add identity(category) as c |
1000:c |
same |
remove shard, add bucket[16](id) as b |
1002:b |
same |
remove ts_day, add day(ts) |
error: duplicate, conflicts with 1001: ts_day |
1001:ts_day |
remove ts_day, add day(ts) as d |
error | 1001:d |
I think the behavior here is the better one. But since the tests are a port of Java's, nothing covers the time-transform case. A test for it would make the difference from Java deliberate rather than accidental.
| }; | ||
| } | ||
| return Err(invalid_data!( | ||
| "Cannot add duplicate partition field {transform}({source_name}), conflicts with {}", |
There was a problem hiding this comment.
The description says the error messages match Java, but the duplicate ones are worded differently. Java reports Cannot add duplicate partition field d=day(ref(name="ts")), conflicts with 1001: ts_day: day(2), with the requested name and the existing field's id. Here it's Cannot add duplicate partition field day(ts), conflicts with ts_day. That's not a problem in itself, just the description being slightly off.
Which issue does this PR close?
BaseUpdatePartitionSpeclogic to iceberg-rust #2583.What changes are included in this PR?
This adds
Transaction::update_partition_spec(). Until now the only way to evolve a partition spec was to build theAddSpecupdate by hand, choosing every field ID yourself.The logic is a port of Java's
BaseUpdatePartitionSpec, so the same changes give the same spec as Java, including the default field names (id_bucket_16,ts_day) and the error messages.Field IDs are assigned here rather than left to the catalog. A new field that matches one in an earlier spec reuses that field's ID, as the spec requires; a match means the same source column and transform, plus the same name if one is given. When several fields match, the highest ID wins, as in Java's
TableMetadata. Otherwise the field gets the next ID afterlast-partition-id. The commit sendsAddSpec, plusSetDefaultSpecunlessadd_non_default_spec()is called. It also asserts the last assigned partition ID and the default spec ID it started from.Not covered yet:
voidplaceholders, which needs its own handling.hour(ts)to aday(ts)table fails at commit on catalogs built on iceberg-rust. fix(spec): accept partition specs produced by spec evolution #3431 fixes the builder.Are these changes tested?
Yes. The unit tests port Java's
TestUpdatePartitionSpec. The table-level tests portTestTableUpdatePartitionSpec, committing through the memory catalog.AI Disclosure
Written with Claude Opus 5.5 and reviewed by me.