You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@iceberg.apache.org by "xanderbailey (via GitHub)" <gi...@apache.org> on 2026/10/12 15:59:48 UTC

[PR] fix(spec): reuse the highest matching partition field id, not an arbitrary one [iceberg-rust]

xanderbailey opened a new pull request, #3413:
URL: https://github.com/apache/iceberg-rust/pull/3413

`reuse_partition_field_ids` collects (source_id, transform) -> field_id into a HashMap with no merge function, so when several specs give one pair different field ids the last one visited wins. `partition_specs` is itself a HashMap, so the winner changes from run to run and the same input metadata yields different output.

Keep the highest id of each pair, as Iceberg Java does:

Collectors.toMap(
field -> Pair.of(field.sourceId(), field.transform().toString()),
PartitionField::fieldId,
Math::max)

Divergent ids for one pair cannot be produced by this crate, since the reuse rule keeps them converged, but they can be read from tables written before the rule existed or by writers that do not apply it.

## Which issue does this PR close?

- Closes #.

## What changes are included in this PR?

## Are these changes tested?

<!--
Specify what test covers (unit test, integration test, etc.).

If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->

## AI Disclosure

--
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [PR] fix(spec): reuse the highest matching partition field id, not an arbitrary one [iceberg-rust]

Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.

mbutrovich commented on code in PR #3413:
URL: https://github.com/apache/iceberg-rust/pull/3413#discussion_r4259136990


##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -3694,4 +3698,88 @@ mod tests {
         let field_ids: Vec<i32> = spec2.fields().iter().map(|f| f.field_id).collect();
         assert_eq!(field_ids, vec![1000, 1001, 1002]); // Reused 1000, 1001; new 1002
     }
+
+    #[test]
+    fn test_partition_field_id_reuse_picks_the_highest_of_several_candidates() {
+        // These specs give identity(x) four different field ids, which a writer that does not
+        // reuse ids can leave behind. `partition_specs` is a HashMap, so the id reused for a new
+        // spec must not depend on which of them is visited first.
+        let metadata: TableMetadata = serde_json::from_value(serde_json::json!({
+            "format-version": 2,
+            "table-uuid": "9c12d441-03fe-4693-9a96-a0705ddf69c1",
+            "location": "s3://bucket/test/location",
+            "last-sequence-number": 0,
+            "last-updated-ms": 1602638573590i64,
+            "last-column-id": 2,
+            "current-schema-id": 0,
+            "schemas": [{
+                "type": "struct",
+                "schema-id": 0,
+                "fields": [
+                    {"id": 1, "name": "x", "required": true, "type": "long"},
+                    {"id": 2, "name": "y", "required": true, "type": "long"},
+                ],
+            }],
+            "default-spec-id": 3,
+            "last-partition-id": 1006,
+            "partition-specs": [
+                {"spec-id": 0, "fields": [
+                    {"name": "x", "transform": "identity", "source-id": 1, "field-id": 1000},
+                ]},
+                {"spec-id": 1, "fields": [
+                    {"name": "x", "transform": "identity", "source-id": 1, "field-id": 1002},
+                ]},
+                {"spec-id": 2, "fields": [
+                    {"name": "x", "transform": "identity", "source-id": 1, "field-id": 1004},
+                ]},
+                {"spec-id": 3, "fields": [
+                    {"name": "x", "transform": "identity", "source-id": 1, "field-id": 1006},
+                ]},
+            ],
+            "default-sort-order-id": 0,
+            "sort-orders": [{"order-id": 0, "fields": []}],
+        }))
+        .unwrap();
+
+        // Two fields, so this is not compatible with either existing spec and is added as a new
+        // one rather than resolving to an existing spec id.
+        let new_spec = UnboundPartitionSpec::builder()
+            .add_partition_field(
+                UnboundPartitionField::builder()
+                    .source_ids(vec![1])
+                    .name("x")
+                    .transform(Transform::Identity)
+                    .build()
+                    .unwrap(),
+            )
+            .unwrap()
+            .add_partition_field(
+                UnboundPartitionField::builder()
+                    .source_ids(vec![2])
+                    .name("y")
+                    .transform(Transform::Identity)
+                    .build()
+                    .unwrap(),
+            )
+            .unwrap()
+            .build();
+
+        let result = metadata
+            .into_builder(Some(
+                "s3://bucket/test/location/metadata/v1.json".to_string(),
+            ))
+            .add_partition_spec(new_spec)
+            .unwrap()
+            .build()
+            .unwrap();
+
+        let spec = result.metadata.partition_spec_by_id(4).unwrap();
+        let fields: Vec<(i32, i32)> = spec
+            .fields()
+            .iter()
+            .map(|f| (f.source_id, f.field_id))
+            .collect();
+        assert_eq!(fields, vec![(1, 1006), (2, 1007)]);
+        assert_eq!(result.metadata.last_partition_id, 1007);
+    }

Review Comment:
   > I've extracted out `highest_equivalent_field_ids` with a test to show that the result is the same regardless of the input order. I this this encodes the behaviour a little better than just relying on a loop I thnink, WDYT?
   
   I like this better too. When I put the old last-wins `collect()` back into `highest_equivalent_field_ids`, `test_highest_equivalent_field_ids_is_independent_of_field_order` fails on every run, and the reversed iterator covers first-wins as well.
   
   `test_partition_field_id_reuse_picks_the_highest_of_several_candidates` still depends on the `HashMap` order. Against the last-wins `collect()` it passed 13 of 40 runs, so it would only sometimes catch `reuse_partition_field_ids` going back to an inline `collect()` that skips the helper. What do you think about wrapping its deserialize-and-build in the loop from my first comment, so the end-to-end test is deterministic as well?



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org