Stefan-Dienst commented on code in PR #3267:
URL: https://github.com/apache/iceberg-rust/pull/3267#discussion_r4107771805
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -264,20 +264,147 @@ 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 {
+ 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`. Both spellings are read; 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,
+ (None, Some(_)) => {
+ return Err(invalid_data!("Empty source-ids is not
allowed"));
+ }
+ (Some(source_id), Some(source_ids)) => {
+ // Tolerated for readers, but the two must agree
+ if source_ids.first() != Some(&source_id) {
+ return Err(invalid_data!(
+ "source-id {source_id} does not match the first
entry of source-ids {source_ids:?}"
+ ));
+ }
+ source_ids
+ }
+ (None, None) => {
+ return Err(invalid_data!("missing field `source-id`"));
Review Comment:
nit: missing field "could also be" `source-ids`. Maybe rephrase to "Either
`source-id` or `source-ids` must be present".
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -264,20 +264,147 @@ 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 {
+ 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`. Both spellings are read; 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,
+ (None, Some(_)) => {
+ return Err(invalid_data!("Empty source-ids is not
allowed"));
+ }
+ (Some(source_id), Some(source_ids)) => {
+ // Tolerated for readers, but the two must agree
Review Comment:
Why is this tolerated? Isn't `source-id` and `source-ids` mutually exclusive
in v3?
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -619,11 +746,9 @@ impl PartitionSpecBuilder {
/// Ensure that the transformation of the field is compatible with type of
the field
/// in the schema. Implicitly also checks if the source field exists in
the schema.
fn check_transform_compatibility(field: &UnboundPartitionField, schema:
&Schema) -> Result<()> {
Review Comment:
Shouldn't this be changed to check transform compatibility for all source
ids?
--
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]
