refactor(spec): make UnboundPartitionField fields private and carry source-ids by moomindani · Pull Request #3267 · apache/iceberg-rust

moomindani · 2026-09-22T22:10:53Z

Which issue does this PR close?

Part of #3172 and #2801. First of the three per-struct PRs @blackmwk asked for in #2802, starting with UnboundPartitionField as he suggested. #2802 stays open and will be narrowed to PartitionField once this lands.

What changes are included in this PR?

UnboundPartitionField gets the #3172 treatment — private fields with accessors — and with it the v3 source-ids spelling.

The four fields are private, with source_id() -> Result<i32>, source_ids() -> &[i32], field_id(), name() and transform() accessors. The derived TypedBuilder is public and, following FileScanTask, its build() returns Result and rejects an empty source_ids; with_field_id is public too.
source_ids: Vec<i32> replaces source_id: i32 and always holds at least one id. source_id() returns the single id, and an error for a multi-argument field rather than quietly handing back the first one.
A _serde module reads either spelling — source-id, or source-ids for a v3 multi-argument transform — and writes back the one that matches the field. An empty source-ids, a missing id, and a field that carries both source-id and source-ids are all rejected.
Binding a multi-argument field now fails with a clear error. A bound PartitionField still carries a single source_id, so until that struct is converted there is nowhere to put the extra ids, and failing loudly beats dropping them on the way into TableCreation or TableUpdate::AddSpec.
check_for_redundant_partitions compares the whole id list instead of one id.
Breaking: UnboundPartitionSpecBuilder::add_partition_field takes an UnboundPartitionField instead of a source id, name and transform, so it can carry a multi-argument field. add_partition_field_internal is folded into it.

Not in this PR, and not lost: PartitionField (#2802, to be narrowed once this lands) and SortField get the same treatment, and with them the multi-argument read support and the Transform::Unknown mapping that #2801 is about.

Are these changes tested?

Yes — five new unit tests in spec/partition.rs: reading a source-ids-only field and writing it back, the single-id round trip, the three malformed-id rejections, binding a multi-argument field failing loudly, and the builder rejecting an empty source_ids. cargo test -p iceberg --lib passes (1769 tests), with cargo fmt, cargo clippy --workspace --all-targets --all-features -- -D warnings and a full workspace build clean. crates/iceberg/public-api.txt is regenerated: four public fields out, five accessors and with_field_id in, builder() still public, From<UnboundPartitionField> for Result<UnboundPartitionField> added, and the new add_partition_field signature.

AI Disclosure

This pull request and its description were written by Isaac.

The four public fields are replaced by accessors, and the derived builder's visibility is limited to the crate, so an instance can only be built inside the crate or read out of a partition spec JSON. That is what lets the struct guarantee its own invariant: source_ids always holds at least one id. source_ids: Vec<i32> replaces source_id: i32. source_id() returns the single id, and an error for a multi-argument field rather than quietly handing back the first one. The serde module reads either spelling -- source-id, or source-ids for a v3 multi-argument transform -- and writes back the one that matches the field. Binding a multi-argument field now fails with a clear error. A bound PartitionField still carries a single source_id, so until that struct is converted there is nowhere to put the extra ids, and failing loudly beats dropping them on the way into TableCreation or TableUpdate::AddSpec.

Stefan-Dienst

Hi @moomindani ,

Thanks for the split PR. I did a quick pass, as I am interested in how this whole thing will land. Only flagged minor things. Looks good otherwise.

Stefan-Dienst · 2026-09-25T19:02:35Z

+                (Some(source_id), Some(source_ids)) => {
+                    // Tolerated for readers, but the two must agree


Why is this tolerated? Isn't source-id and source-ids mutually exclusive in v3?

Good catch — nothing needs the tolerance. The spec only rules on what a writer emits, no writer emits both (Java still writes only source-id), and PyIceberg already rejects the pair as mutually exclusive, so accepting a matching pair only let the two readers disagree. 8fcd795 now rejects source-id and source-ids together, including when they agree.

Stefan-Dienst · 2026-09-25T19:05:34Z

+                    source_ids
+                }
+                (None, None) => {
+                    return Err(invalid_data!("missing field `source-id`"));


nit: missing field "could also be" source-ids. Maybe rephrase to "Either source-id or source-ids must be present".

Done in 8fcd795, with your wording.

Stefan-Dienst · 2026-09-25T19:20:13Z


    /// Ensure that the transformation of the field is compatible with type of the field
    /// in the schema. Implicitly also checks if the source field exists in the schema.
    fn check_transform_compatibility(field: &UnboundPartitionField, schema: &Schema) -> Result<()> {


Shouldn't this be changed to check transform compatibility for all source ids?

This is deliberate for now: field.source_id()? here is what makes binding a multi-argument field fail, which test_binding_a_multi_argument_field_fails_loudly pins. Checking every source id needs a multi-input Transform::result_type, and a bound PartitionField still holds one id, so that check belongs with the PartitionField conversion (#2802). I added a comment in 8fcd795 so the intent is visible at the call site.

PyIceberg already treats the two spellings as mutually exclusive, and no writer emits both, so accepting a matching pair only let the two readers disagree. Also name both spellings in the missing-id error and note why check_transform_compatibility rejects a multi-argument field. Co-authored-by: Isaac <no-reply@databricks.com>

blackmwk

Thanks @moomindani for this pr!

Address review: the UnboundPartitionField builder and with_field_id are public, since an unbound field is not expected to be validated yet, and UnboundPartitionSpecBuilder::add_partition_field now takes the field rather than a single source id, name and transform. With the builder public an empty source_ids can be built, so serializing one no longer panics and both spec builders reject it with a clear error. Co-authored-by: Isaac <no-reply@databricks.com>

Stefan-Dienst · 2026-09-29T08:33:15Z

@moomindani Thanks for the changes.

Maybe we should take #3172 (comment) into account and first have a preceding PR that warns about the removal of the pub fields of UnboundPartitionField.

blackmwk · 2026-09-29T11:20:12Z

+/// Being unbound, a field built through [`UnboundPartitionField::builder`] is not validated;
+/// it is checked when added to an [`UnboundPartitionSpecBuilder`] and again when bound to a
+/// schema.
 #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, TypedBuilder)]


I think this is one missing point: the builder's return type should be Result<UnboundPartitionField>, since we should not allow empty source ids. You can learn from what we did in FileScanTask to make TypedBuilder to return a Result and add check there.

Done in 4e72bee, following FileScanTask: build() now returns Result<UnboundPartitionField> and rejects an empty source_ids, so an instance is always well formed again and the spec builders no longer carry their own check.

blackmwk · 2026-09-29T11:21:01Z

+        fn try_from(value: UnboundPartitionFieldSerde) -> Result<Self, Error> {
+            let source_ids = match (value.source_id, value.source_ids) {
+                (Some(source_id), None) => vec![source_id],
+                (None, Some(source_ids)) if !source_ids.is_empty() => source_ids,


This check should be done in the builder I mentioned in https://github.com/apache/iceberg-rust/pull/3267/changes#r4132801710

Done in 4e72bee — deserialization now builds through the same builder, so the empty check lives only there.

blackmwk · 2026-09-29T11:25:20Z

@moomindani Thanks for the changes.

Maybe we should take #3172 (comment) into account and first have a preceding PR that warns about the removal of the pub fields of UnboundPartitionField.

I replied in original issue, let's continue discussion there.

Address review: the builder's build() now returns Result<UnboundPartitionField> and rejects an empty source_ids, following FileScanTask. Deserialization goes through the same builder, so the check lives in one place and the spec builders no longer need their own. Co-authored-by: Isaac <no-reply@databricks.com>

blackmwk

Thanks @moomindani for this pr!

apache#3267 made UnboundPartitionField's fields private and gave it source-ids, so UnboundPartitionSpecBuilder::add_partition_field now takes the field rather than its parts. The spec-evolution fixture built both of its specs the old way. Git merges the two changes without a word and only the compiler notices, so the port is part of this merge rather than a commit after it. Co-authored-by: Rexwell Minnis <rexminnis@gmail.com>

moomindani mentioned this pull request Sep 22, 2026

feat(spec): read partition and sort fields with multi-argument transforms (source-ids) #2802

Open

Stefan-Dienst reviewed Sep 25, 2026

View reviewed changes

blackmwk reviewed Sep 29, 2026

View reviewed changes

Comment thread crates/iceberg/src/spec/partition.rs Outdated

Comment thread crates/iceberg/src/spec/partition.rs Outdated

Comment thread crates/iceberg/src/spec/partition.rs Outdated

Comment thread crates/iceberg/src/spec/table_metadata_builder.rs Outdated

blackmwk reviewed Sep 29, 2026

View reviewed changes

blackmwk approved these changes Sep 29, 2026

View reviewed changes

blackmwk added this pull request to the merge queue Sep 29, 2026

Merged via the queue into apache:main with commit deab069 Sep 29, 2026
25 checks passed

		(Some(source_id), Some(source_ids)) => {
		// Tolerated for readers, but the two must agree

Conversation

moomindani commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

Uh oh!

Stefan-Dienst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackmwk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Stefan-Dienst commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackmwk commented Sep 29, 2026

Uh oh!

blackmwk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moomindani commented Sep 22, 2026 •

edited

Loading

Stefan-Dienst commented Sep 29, 2026 •

edited

Loading