Stefan-Dienst commented on code in PR #2802: URL: https://github.com/apache/iceberg-rust/pull/2802#discussion_r4036638712
########## crates/iceberg/src/spec/table_metadata.rs: ########## Review Comment: Do you think it maybe worth to add tests for the table metadata that use multiple arguments for partition and sort? ########## crates/integrations/datafusion/src/physical_plan/repartition.rs: ########## Review Comment: the datafusion integration will be migrated, see https://github.com/apache/iceberg-rust/issues/3029. I don't know if these changes still need to be part of the PR. ########## crates/iceberg/src/spec/partition.rs: ########## @@ -34,10 +34,18 @@ pub(crate) const DEFAULT_PARTITION_SPEC_ID: i32 = 0; /// Partition fields capture the transform from table data to partition values. #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, TypedBuilder)] -#[serde(rename_all = "kebab-case")] +#[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>>, Review Comment: 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. ########## crates/iceberg/src/spec/partition.rs: ########## @@ -34,10 +34,18 @@ pub(crate) const DEFAULT_PARTITION_SPEC_ID: i32 = 0; /// Partition fields capture the transform from table data to partition values. #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, TypedBuilder)] -#[serde(rename_all = "kebab-case")] +#[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>>, Review Comment: With the addition of the `source_ids` the `equivalent_ignoring_names` function [here](https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/partitioning.rs#L156), maybe needs revisiting. ########## crates/iceberg/src/spec/partition.rs: ########## @@ -34,10 +34,18 @@ pub(crate) const DEFAULT_PARTITION_SPEC_ID: i32 = 0; /// Partition fields capture the transform from table data to partition values. #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, TypedBuilder)] -#[serde(rename_all = "kebab-case")] +#[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>>, Review Comment: 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`) -- 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]
