moomindani commented on code in PR #3267:
URL: https://github.com/apache/iceberg-rust/pull/3267#discussion_r4132923316
##########
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.
+///
+/// Being unbound, a field built through [`UnboundPartitionField::builder`] is
not validated;
+/// it is checked when added to an [`UnboundPartitionSpecBuilder`] and again
when bound to a
+/// schema.
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, TypedBuilder)]
Review Comment:
Done in 4e72bee, following `FileScanTask`: `build()` now returns
`Result<UnboundPartitionField>` and rejects an empty `source_ids`, so an
instance is always well formed again and the spec builders no longer carry
their own check.
##########
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.
+///
+/// Being unbound, a field built through [`UnboundPartitionField::builder`] is
not validated;
+/// it is checked when added to an [`UnboundPartitionSpecBuilder`] and again
when bound to a
+/// schema.
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, TypedBuilder)]
-#[serde(rename_all = "kebab-case")]
+#[serde(
+ try_from = "self::_serde::UnboundPartitionFieldSerde",
+ into = "self::_serde::UnboundPartitionFieldSerde"
+)]
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,
+ #[builder(setter(into))]
+ 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 once the
field is part
+ /// of a partition spec.
+ 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 fn with_field_id(self, field_id: i32) -> Self {
+ Self {
+ field_id: Some(field_id),
+ ..self
+ }
+ }
+}
+
+mod _serde {
+ use serde::{Deserialize, Serialize};
+
+ use super::UnboundPartitionField;
+ use crate::Error;
+ use crate::error::invalid_data;
+ use crate::spec::Transform;
+
+ /// Per the spec a single-argument field carries `source-id` and a
multi-argument field
+ /// carries `source-ids`. Either spelling is read, but not both at once;
the one that
+ /// matches the field is written.
+ #[derive(Serialize, Deserialize)]
+ #[serde(rename_all = "kebab-case")]
+ pub(super) struct UnboundPartitionFieldSerde {
+ #[serde(default, skip_serializing_if = "Option::is_none")]
+ source_id: Option<i32>,
+ #[serde(default, skip_serializing_if = "Option::is_none")]
+ source_ids: Option<Vec<i32>>,
+ #[serde(default, skip_serializing_if = "Option::is_none")]
+ field_id: Option<i32>,
+ name: String,
+ transform: Transform,
+ }
+
+ impl TryFrom<UnboundPartitionFieldSerde> for UnboundPartitionField {
+ type Error = Error;
+
+ fn try_from(value: UnboundPartitionFieldSerde) -> Result<Self, Error> {
+ let source_ids = match (value.source_id, value.source_ids) {
+ (Some(source_id), None) => vec![source_id],
+ (None, Some(source_ids)) if !source_ids.is_empty() =>
source_ids,
Review Comment:
Done in 4e72bee — deserialization now builds through the same builder, so
the empty check lives only there.
--
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]
