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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
