moomindani commented on code in PR #2802:
URL: https://github.com/apache/iceberg-rust/pull/2802#discussion_r4462110063
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
Review Comment:
Added `test_table_metadata_v3_multi_argument_partition_field` in 2b6554c7: a
v3 metadata document with a `source-ids` partition field reads, binds as
`unknown`, and writes `source-ids` back. The sort-order half goes with the
`SortField` PR.
##########
crates/integrations/datafusion/src/physical_plan/repartition.rs:
##########
Review Comment:
This PR no longer touches the DataFusion integration; those changes went
with the narrowing to `PartitionField`.
##########
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:
Updated: it now compares the whole `source_ids` list, so two fields that
share a first id but differ after it are not equivalent.
##########
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:
`add_unbound_field` / `add_unbound_fields` already take a multi-argument
`UnboundPartitionField` and check every source id. I kept the name-based
`add_partition_field(source_name, ...)` single-column: a several-column form
only becomes useful once a multi-argument transform can be evaluated, and until
then such a field binds as `unknown`.
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -54,6 +62,57 @@ impl PartitionField {
}
}
+mod _serde_partition_field {
Review Comment:
Done, the module is `_serde`.
##########
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 {
Review Comment:
Done in that order: #3267 landed first, and this PR is the `PartitionField`
step.
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -264,21 +324,80 @@ impl PartitionKey {
pub type UnboundPartitionSpecRef = Arc<UnboundPartitionSpec>;
/// Unbound partition field can be built without a schema and later bound to a
schema.
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, TypedBuilder)]
-#[serde(rename_all = "kebab-case")]
+#[serde(
+ try_from = "_serde_unbound_partition_field::UnboundPartitionFieldSerde",
+ into = "_serde_unbound_partition_field::UnboundPartitionFieldSerde"
+)]
pub struct UnboundPartitionField {
Review Comment:
Done, `PartitionField` now follows the same shape as `UnboundPartitionField`
in #3267.
--
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]
