blackmwk commented on code in PR #3267:
URL: https://github.com/apache/iceberg-rust/pull/3267#discussion_r4129161827
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -264,20 +264,146 @@ impl PartitionKey {
/// Reference to [`UnboundPartitionSpec`].
pub type UnboundPartitionSpecRef = Arc<UnboundPartitionSpec>;
/// Unbound partition field can be built without a schema and later bound to a
schema.
+///
+/// The fields are private so that an instance is known to be well formed once
built: in
+/// particular `source_ids` always holds at least one id. Construct one through
+/// [`UnboundPartitionSpec::builder`], or read one out of a partition spec
JSON.
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, TypedBuilder)]
-#[serde(rename_all = "kebab-case")]
+#[serde(
+ try_from = "self::_serde::UnboundPartitionFieldSerde",
+ into = "self::_serde::UnboundPartitionFieldSerde"
+)]
+#[builder(
+ builder_method(vis = "pub(crate)"),
+ builder_type(vis = "pub(crate)"),
+ build_method(vis = "pub(crate)")
Review Comment:
They could be pub available.
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -264,20 +264,146 @@ impl PartitionKey {
/// Reference to [`UnboundPartitionSpec`].
pub type UnboundPartitionSpecRef = Arc<UnboundPartitionSpec>;
/// Unbound partition field can be built without a schema and later bound to a
schema.
+///
+/// The fields are private so that an instance is known to be well formed once
built: in
+/// particular `source_ids` always holds at least one id. Construct one through
+/// [`UnboundPartitionSpec::builder`], or read one out of a partition spec
JSON.
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, TypedBuilder)]
-#[serde(rename_all = "kebab-case")]
+#[serde(
+ try_from = "self::_serde::UnboundPartitionFieldSerde",
+ into = "self::_serde::UnboundPartitionFieldSerde"
+)]
+#[builder(
+ builder_method(vis = "pub(crate)"),
+ builder_type(vis = "pub(crate)"),
+ build_method(vis = "pub(crate)")
+)]
pub struct UnboundPartitionField {
- /// A source column id from the table’s schema
- pub source_id: i32,
+ /// The source column ids from the table’s schema. A single-argument
transform reads one
+ /// id; a v3 multi-argument transform reads several.
+ source_ids: Vec<i32>,
/// A partition field id that is used to identify a partition field and is
unique within a partition spec.
/// In v2 table metadata, it is unique across all partition specs.
#[builder(default, setter(strip_option(fallback = field_id_opt)))]
- #[serde(skip_serializing_if = "Option::is_none")]
- pub field_id: Option<i32>,
+ field_id: Option<i32>,
/// A partition name.
- pub name: String,
+ name: String,
/// A transform that is applied to the source column to produce a
partition value.
- pub transform: Transform,
+ transform: Transform,
+}
+
+impl UnboundPartitionField {
+ /// The single source column id this field reads.
+ ///
+ /// Returns an error for a multi-argument field, which reads several
columns and therefore
+ /// has no single source id. Use [`Self::source_ids`] to handle both
shapes.
+ pub fn source_id(&self) -> Result<i32> {
+ match self.source_ids.as_slice() {
+ [source_id] => Ok(*source_id),
+ source_ids => Err(invalid_data!(
+ "Partition field '{}' reads {} source columns and has no
single source id",
+ self.name,
+ source_ids.len()
+ )),
+ }
+ }
+
+ /// The source column ids this field reads, in order. Never empty.
+ pub fn source_ids(&self) -> &[i32] {
+ &self.source_ids
+ }
+
+ /// The partition field id, when one was assigned.
+ pub fn field_id(&self) -> Option<i32> {
+ self.field_id
+ }
+
+ /// The partition name.
+ pub fn name(&self) -> &str {
+ &self.name
+ }
+
+ /// The transform applied to the source columns to produce a partition
value.
+ pub fn transform(&self) -> Transform {
+ self.transform
+ }
+
+ /// Return this field with the given partition field id assigned.
+ pub(crate) fn with_field_id(self, field_id: i32) -> Self {
Review Comment:
I'm fine this make this pub, it's unbound, so the user is supposed to know
it has not been validated.
##########
crates/iceberg/src/spec/partition.rs:
##########
Review Comment:
This is actually not a good api design, it exposes internal of
`UnboundPartitionField`. I would change this api's signature so accept a
`UnboundPartitionField`
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -850,19 +850,19 @@ impl TableMetadataBuilder {
.partition_specs
.values()
.flat_map(|spec| spec.fields())
- .map(|field| ((field.source_id, &field.transform), field.field_id))
+ .map(|field| ((vec![field.source_id], field.transform),
field.field_id))
.collect();
// Create new fields with reused field IDs where possible
let fields = unbound_spec
.fields
.into_iter()
- .map(|mut field| {
- if field.field_id.is_none()
+ .map(|field| {
+ if field.field_id().is_none()
&& let Some(&existing_field_id) =
- equivalent_field_ids.get(&(field.source_id,
&field.transform))
+
equivalent_field_ids.get(&(field.source_ids().to_vec(), field.transform()))
{
- field.field_id = Some(existing_field_id);
+ return field.with_field_id(existing_field_id);
Review Comment:
nit: this is odd. You can remove the return and change it to
```rust
if xxx {
field.with_field_id(existing_field_id)
} else {
field
}
```
Not a block, just minor readability improvement.
--
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]
