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

moomindani · 2026-07-11T01:34:38Z

Which issue does this PR close?

Part of #3172 and #2801. Second of the three per-struct PRs @blackmwk asked for, after #3267 (UnboundPartitionField). This PR is now narrowed to PartitionField; SortField follows, and #2801 closes with it.

What changes are included in this PR?

PartitionField gets the same treatment #3267 gave UnboundPartitionField.

The four fields are private, with source_id() -> Result<i32>, source_ids() -> &[i32], field_id(), name() and transform() accessors. The derived TypedBuilder is kept pub(crate), as you suggested for the bound struct, and its build() returns Result, following refactor(spec): make UnboundPartitionField fields private and carry source-ids #3267.
source_ids: Vec<i32> replaces source_id: i32 and always holds at least one id. The _serde module reads source-id or source-ids (not both) and writes back the one that matches the field; the read and write rules are shared with UnboundPartitionField.
Binding a multi-argument field now succeeds, since a bound field can carry every id. The bound-to-unbound conversion keeps them all as well.
Transform compatibility is checked for every source id: each must exist in the schema, which is the point @Stefan-Dienst raised on refactor(spec): make UnboundPartitionField fields private and carry source-ids #3267.
is_compatible_with, partition_type, the field-id reuse in table_metadata_builder, and the missing-source checks all consider the whole id list.
Call sites across the crate move to the accessors. Where a site needs one column (identity constants, projections, partition values), it either skips a multi-argument field or returns its source_id() error; it never quietly takes the first id.

One decision worth your call:

A multi-argument transform binds and reads as Transform::Unknown, and build() rejects a multi-argument field with any other transform. Multi-argument transforms cannot be evaluated yet (result_type, projection and the transform functions all take one input), and the spec requires v3 readers to read such tables while ignoring them. UnboundPartitionField keeps the original transform, since it only carries what it was given. The cost is the one already tracked in Unrecognized transform names fail parsing instead of becoming Transform::Unknown #2789: writing the field back writes unknown rather than the original name.

Are these changes tested?

Yes — five new unit tests in spec/partition.rs, one of them replacing #3267's test_binding_a_multi_argument_field_fails_loudly, which now fails by design:

binding a multi-argument field keeps its ids and binds as unknown, and a missing source column is still rejected;
reading source-ids and writing back the matching spelling;
rejecting an empty source-ids, a missing id, and a field carrying both spellings;
the builder rejecting an empty source_ids and a multi-argument field whose transform is not unknown;
is_compatible_with telling apart two fields that share a first id.

cargo test -p iceberg --lib passes (1801 tests), as do the rest, sql, glue and hms catalog unit tests. cargo fmt, cargo clippy --workspace --all-targets --all-features -- -D warnings and a full workspace build are clean, and crates/iceberg/public-api.txt is regenerated: four public fields and the public builder() out, five accessors and From<PartitionField> for Result<PartitionField> in.

AI Disclosure

This pull request and its description were written by Isaac.

moomindani · 2026-07-29T01:45:15Z

Gentle ping — open for ~2.5 weeks, CI green, no review yet.

This one is a correctness gap rather than a feature: the v3 spec serializes partition and sort fields whose transform takes multiple arguments with a source-ids list instead of a single source-id, and deserializing such metadata currently fails with missing field source-id. So a v3 table containing any multi-argument transform can't be read at all.

Note it touches public-api.txt, so it's worth a look from that angle too.

@blackmwk @CTTY would either of you have time for a review? Closes #2801, part of #2411.

moomindani · 2026-09-15T23:49:18Z

Still waiting for review, opened 9 July. Mergeable and green on current main.

This is the read side of multi-argument transforms: v3 writes partition and sort fields with source-ids, and without this a reader fails on those tables. It pairs with #2790 — an unrecognized transform name has to degrade to unknown, and a multi-argument field has to be readable — but the two are independent and can be reviewed in either order.

@CTTY @kevinjqliu a review would be appreciated.

Stefan-Dienst

Hi @moomindani ,

I am new to the iceberg-rust repo, so if my comments maybe confusing just feel free to ignore them. But I have used your PR to become a bit more familiar with the spec module and had a few thoughts while doing so that I wanted to share with you.

Stefan-Dienst · 2026-09-17T12:08:45Z

Do you think it maybe worth to add tests for the table metadata that use multiple arguments for partition and sort?

Stefan-Dienst · 2026-09-17T12:14:20Z

+#[serde(
+    try_from = "_serde_partition_field::PartitionFieldSerde",
+    into = "_serde_partition_field::PartitionFieldSerde"
+)]
 pub struct PartitionField {
    /// A source column id from the table’s schema
    pub source_id: i32,
+    /// Source column ids when the transform takes multiple arguments (v3 multi-argument
+    /// transforms). `None` for single-argument transforms, where `source_id` is used instead.
+    /// When set, `source_id` holds the first id so that existing consumers keep working.
+    #[builder(default)]
+    pub source_ids: Option<Vec<i32>>,


Here I am unsure if implementing the serde for PartitionField is the best approach. The first thing I stumbled over when reading this, was that the spec version was not explicit. See for example Schema, where the spec version is handled explicitly in the serde: https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/spec/schema/_serde.rs

I don't know if it is worth doing here, because the differences between the specs are rather minor, but it may be worth to consider.

(Same argument for SortField)

I don't think we should add such a filed, for in memory data structures, we should just use source_ids: Vec<i32>

Just to clarify: I did not mean to add a version field, but have a different struct for the different spec versions. For example something like:

#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)] #[serde(rename_all = "kebab-case")] pub(super) struct PartitionFieldV3 { #[serde(default, skip_serializing_if = "Option::is_none")] source_id: Option<i32>, #[serde(default, skip_serializing_if = "Option::is_none")] source_ids: Option<Vec<i32>>, field_id: i32, name: String, transform: Option<String>, } #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)] #[serde(rename_all = "kebab-case")] pub(super) struct PartitionFieldV2 { source_id: i32, field_id: i32, name: String, transform: Option<String> } #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)] #[serde(rename_all = "kebab-case")] pub(super) struct PartitionFieldV1 { source_id: i32, name: String, transform: Option<String> }

and then use this in the table metadata serdes like TableMetadataV2.
Then the in memory structure could be changed to (taking #3172 into account)

pub struct PartitionField { source_ids: Vec<i32>, [...]

and the impl TryFrom and impl From handle the different specs versions.

Does this make sense?

Stefan-Dienst · 2026-09-17T12:19:20Z

the datafusion integration will be migrated, see #3029. I don't know if these changes still need to be part of the PR.

Stefan-Dienst · 2026-09-17T12:22:10Z

+    /// transforms). `None` for single-argument transforms, where `source_id` is used instead.
+    /// When set, `source_id` holds the first id so that existing consumers keep working.
+    #[builder(default)]
+    pub source_ids: Option<Vec<i32>>,


With the addition of the source_ids the equivalent_ignoring_names function here, maybe needs revisiting.

Stefan-Dienst · 2026-09-17T12:30:36Z

+    /// transforms). `None` for single-argument transforms, where `source_id` is used instead.
+    /// When set, `source_id` holds the first id so that existing consumers keep working.
+    #[builder(default)]
+    pub source_ids: Option<Vec<i32>>,


The PartitionSpecBuilder & UnboundPartitionSpecBuilder have methods like add_partition_field and add_partition_fields, which do not support adding multi-arguments fields yet.

Maybe worth also updating in this PR.

moomindani · 2026-09-18T07:05:29Z

Thanks for going through this — two of these were real, and I've pushed fixes for both.

equivalent_ignoring_names: you were right that it needed revisiting. It compared only source_id, which for a multi-argument field holds just the first id, so two fields sharing a field id and a first source id but reading different columns compared equal and the cross-spec compatibility check accepted them. It now compares the effective source ids. Worth noting for anyone reading later: a spec-compliant multi-argument field normalizes to Transform::Unknown and compute_unified_partition_type rejects unknown transforms before this point, so the gap is reachable through direct construction or a field that carries both source-id and source-ids, not through the ordinary read path.

The builders: this turned out to be worse than an ergonomics gap. UnboundPartitionField had no source_ids at all, so From<PartitionField> dropped the extra ids — and UnboundPartitionSpec is the wire type for TableCreation and TableUpdate::AddSpec, so a v3 multi-argument spec silently degraded to single-argument on the way back out. source_ids is now on UnboundPartitionField and threaded through both conversions, and its serde shares normalize_transform_sources with the bound field so the spec's source-ids-only form deserializes too. Adding a multi-argument setter to add_partition_field itself is a separate ergonomics change I'd rather do once the read side is settled.

Table metadata tests: added, via a TableMetadataV3MultiArgTransforms.json fixture that pins both a multi-argument partition field and a multi-argument sort field, read and written back.

repartition.rs: those two lines are source_ids: None in struct literals inside mod tests — required for the crate to compile, not new functionality in the DataFusion integration.

Spec-version-explicit serde: I left this as is for now. source-ids is v3-only, skip_serializing_if keeps v1/v2 output byte-identical, and the version-dependent normalization lives in one shared function. Java has no multi-argument implementation to mirror, so there's no reference shape to match yet. Happy to restructure it along schema/_serde.rs lines if a committer prefers that.

moomindani · 2026-09-18T07:23:51Z

Update after rebasing onto current main: the repartition.rs hunk is gone entirely, since #3228 removed the migrated DataFusion crates. So your point landed — that file is no longer part of this PR.

blackmwk

Thanks @moomindani for this pr. While this pr is valuable, I think the correct direction would be like:

Split them into three prs, one for each struct.
For each struct, we should remove all pub filed accessors as mentioned in #3172, and then replace the new ids.

blackmwk · 2026-09-18T09:59:05Z

    }
 }

+mod _serde_partition_field {


Suggested change

mod _serde_partition_field {

mod _serde {

Following others convention.

blackmwk · 2026-09-18T10:01:05Z

+    try_from = "_serde_partition_field::PartitionFieldSerde",
+    into = "_serde_partition_field::PartitionFieldSerde"
+)]
 pub struct PartitionField {


I prefer to do what's mentioned in #3172 first.

blackmwk · 2026-09-18T10:01:43Z

+#[serde(
+    try_from = "_serde_partition_field::PartitionFieldSerde",
+    into = "_serde_partition_field::PartitionFieldSerde"
+)]
 pub struct PartitionField {
    /// A source column id from the table’s schema
    pub source_id: i32,
+    /// Source column ids when the transform takes multiple arguments (v3 multi-argument
+    /// transforms). `None` for single-argument transforms, where `source_id` is used instead.
+    /// When set, `source_id` holds the first id so that existing consumers keep working.
+    #[builder(default)]
+    pub source_ids: Option<Vec<i32>>,


I don't think we should add such a filed, for in memory data structures, we should just use source_ids: Vec<i32>

blackmwk · 2026-09-18T10:02:21Z

+    try_from = "_serde_unbound_partition_field::UnboundPartitionFieldSerde",
+    into = "_serde_unbound_partition_field::UnboundPartitionFieldSerde"
+)]
 pub struct UnboundPartitionField {


Simiarly for PartitionField

blackmwk · 2026-09-18T10:02:32Z

    pub transform: Transform,
 }

+mod _serde_unbound_partition_field {


Merge with _serde

moomindani · 2026-09-18T21:58:38Z

Thanks — happy to follow that direction. Before I start the wide mechanical part I'd like to confirm the target shape, since nothing references #3172 yet and this would be the first struct to get it.

Plan, if it matches what you have in mind:

PartitionField: fields private with accessors, source_ids: Vec<i32>, serde module renamed to _serde — this PR, re-cut to that scope.
SortField: the same, as a follow-up.
UnboundPartitionField: the same, plus the bound/unbound conversion, so the multi-argument ids stop being dropped on the way into TableCreation and TableUpdate::AddSpec.

Two questions where guessing would be expensive:

Construction path. The precedent I can see in the crate is PartitionSpec: private fields, spec_id()/fields() accessors, builder. Should PartitionField keep its derived TypedBuilder, or get a validating constructor, given that the point of [EPIC] Remove all pub field accessor. #3172 is for a built instance to be known valid?
source_id once the fields are private. With source_ids: Vec<i32> always populated, should source_id stop being stored and become an accessor returning the first id, or stay as its own field?

One note on sequencing: with the split, the bound-to-unbound conversion keeps dropping the extra ids until step 3 lands, because UnboundPartitionField has nowhere to put them. Fine by me — flagging it so it isn't a surprise in review.

The two smaller points are agreed and will be in the re-cut: _serde naming with the modules merged, and Vec<i32> instead of Option<Vec<i32>>.

blackmwk · 2026-09-22T08:11:22Z

Thanks — happy to follow that direction. Before I start the wide mechanical part I'd like to confirm the target shape, since nothing references #3172 yet and this would be the first struct to get it.

Plan, if it matches what you have in mind:

PartitionField: fields private with accessors, source_ids: Vec<i32>, serde module renamed to _serde — this PR, re-cut to that scope.

SortField: the same, as a follow-up.

UnboundPartitionField: the same, plus the bound/unbound conversion, so the multi-argument ids stop being dropped on the way into TableCreation and TableUpdate::AddSpec.

Two questions where guessing would be expensive:

Construction path. The precedent I can see in the crate is PartitionSpec: private fields, spec_id()/fields() accessors, builder. Should PartitionField keep its derived TypedBuilder, or get a validating constructor, given that the point of [EPIC] Remove all pub field accessor. #3172 is for a built instance to be known valid?

We should still keep the derived TypedBuilder, but limit it's visibility to mod private.

source_id once the fields are private. With source_ids: Vec<i32> always populated, should source_id stop being stored and become an accessor returning the first id, or stay as its own field?

I prefer to keep two methods:

source_id() -> Result<i32>: The return type changed to Result
source_ids() -> &[i32]: To return all source ids.

One note on sequencing: with the split, the bound-to-unbound conversion keeps dropping the extra ids until step 3 lands, because UnboundPartitionField has nowhere to put them. Fine by me — flagging it so it isn't a surprise in review.

The two smaller points are agreed and will be in the re-cut: _serde naming with the modules merged, and Vec<i32> instead of Option<Vec<i32>>.

We could add UnboundPartitionField refactoring first.

moomindani · 2026-09-22T22:11:02Z

Split this out as #3267, starting with UnboundPartitionField as you suggested rather than re-cutting this PR in place — that keeps this one pointed at #2801 and reviewable on its own once the refactors land.

Your answers are applied there: TypedBuilder kept with its visibility limited, source_id() -> Result<i32> alongside source_ids() -> &[i32], source_ids: Vec<i32> always non-empty, and the serde module named _serde. The three decisions I had to derive are called out at the end of its description. Once it lands I will rebase this PR onto it and narrow it to PartitionField, with SortField after that.

The second of the per-struct refactors after UnboundPartitionField. The four fields are private behind accessors, source_ids replaces source_id, and the builder validates at build time. A bound field can now hold several source ids, so binding a multi-argument field succeeds instead of failing. Such a transform cannot be evaluated yet, so it binds and reads as Transform::Unknown, as the spec requires of v3 readers; transform compatibility is checked for every source id. Co-authored-by: Isaac <no-reply@databricks.com>

moomindani · 2026-09-30T03:14:06Z

Narrowed to PartitionField now that #3267 has landed, as planned; SortField comes next and closes #2801. The description is rewritten to match.

moomindani force-pushed the multi-arg-source-ids-read branch from c68a8f5 to cbe2556 Compare August 21, 2026 16:26

Stefan-Dienst reviewed Sep 17, 2026

View reviewed changes

moomindani force-pushed the multi-arg-source-ids-read branch from 75d6423 to a14f0d3 Compare September 18, 2026 07:10

blackmwk requested changes Sep 18, 2026

View reviewed changes

moomindani mentioned this pull request Sep 22, 2026

refactor(spec): make UnboundPartitionField fields private and carry source-ids #3267

Merged

moomindani force-pushed the multi-arg-source-ids-read branch from a14f0d3 to a67f9c4 Compare September 30, 2026 03:13

moomindani changed the title ~~feat(spec): read partition and sort fields with multi-argument transforms (source-ids)~~ refactor(spec): make PartitionField fields private and carry source-ids Sep 30, 2026

mfellner mentioned this pull request Oct 11, 2026

EPIC: v3 Support Tracking #2411

Open

Conversation

moomindani commented Jul 11, 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!

moomindani commented Jul 29, 2026

Uh oh!

moomindani commented Sep 15, 2026

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!

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

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!

moomindani commented Sep 18, 2026

Uh oh!

moomindani commented Sep 18, 2026

Uh oh!

blackmwk 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!

moomindani commented Sep 18, 2026

Uh oh!

blackmwk commented Sep 22, 2026

Uh oh!

moomindani commented Sep 22, 2026

Uh oh!

moomindani commented Sep 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moomindani commented Jul 11, 2026 •

edited

Loading

Stefan-Dienst Sep 18, 2026 •

edited

Loading