feat(spec): read partition and sort fields with multi-argument transforms (source-ids) by moomindani · Pull Request #2802 · apache/iceberg-rust

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

Which issue does this PR close?

Closes Reading v3 metadata with multi-argument transform fields (source-ids) fails #2801. Part of EPIC: v3 Support Tracking #2411.

What changes are included in this PR?

The v3 spec serializes partition fields and sort fields whose transform takes multiple arguments with a source-ids list instead of the single source-id. Deserializing such metadata failed with missing field source-id, so a v3 table containing any multi-argument transform could not be read at all.

This adds read tolerance, mirroring PyIceberg (apache/iceberg-python#3630):

PartitionField and SortField accept source-ids: a single-element list is normalized onto source-id; a multi-element list is kept in the new source_ids field and the transform maps to Transform::Unknown — multi-argument transforms cannot be evaluated, and the spec requires v3 readers to read such tables ignoring them. source_id holds the first id so existing consumers keep working.
Serialization follows the spec's writer rule: single-argument transforms write only source-id, multi-argument transforms write only source-ids, so the field round-trips.
PartitionSpec::is_compatible_with also compares source_ids, so two multi-arg fields sharing the first id no longer compare as identical.
SortField's Display renders all ids for multi-argument fields.
PartitionSpec::partition_type works unchanged: Transform::Unknown produces string per the spec.

Note: this adds a public source_ids field to PartitionField and SortField (will be flagged by cargo-public-api). Known limitation shared with #2790: Transform::Unknown does not retain the original transform name, so round-tripping writes "unknown"; name preservation stays tracked in #2789.

Are these changes tested?

Yes — new unit tests in spec/partition.rs and spec/sort.rs: multi-arg deserialization and round-trip, single-element normalization, empty source-ids rejection, missing-transform rejection, and partition_type over a multi-arg field. cargo test -p iceberg --lib (1399 tests), cargo clippy -p iceberg --lib --tests, and cargo check --workspace --tests pass locally.

This pull request and its description were written by Claude Fable 5.

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.

…-ids)

UnboundPartitionField had no source-ids, so converting a bound spec to an unbound one dropped the extra source ids and left source-id, the first id, behind. UnboundPartitionSpec is the wire type for table creation and AddSpec updates, so a v3 spec with a multi-argument transform silently degraded to a single-argument field on the way back out. Add source-ids to UnboundPartitionField and thread it through both conversions. Its serde now shares normalize_transform_sources with PartitionField, so an unbound field written the way the spec requires -- source-ids only, no source-id -- also deserializes. Adds a table metadata fixture with multi-argument partition and sort fields to cover both surfaces end to end.

equivalent_ignoring_names compared only source-id, which for a multi-argument field holds just the first source id. Two fields sharing a field id and a first source id but reading different columns compared equal, so the cross-spec compatibility check accepted them as the same field. Compare the effective source ids instead: source-ids for a multi-argument transform, otherwise the single source-id.

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.

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 added 4 commits September 18, 2026 16:07

Read partition and sort fields with multi-argument transforms (source…

565ecae

…-ids)

Update public-api.txt for the new source_ids fields

fc38851

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

Open

Conversation

moomindani commented Jul 11, 2026

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

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!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Stefan-Dienst Sep 18, 2026 •

edited

Loading