xanderbailey commented on code in PR #3413:
URL: https://github.com/apache/iceberg-rust/pull/3413#discussion_r4252366188
##########
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:
This is a nice idea, I've riffed on this, rather than the loop 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?
--
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]
