moomindani commented on code in PR #3267:
URL: https://github.com/apache/iceberg-rust/pull/3267#discussion_r4117561668
##########
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:
Good catch — nothing needs the tolerance. The spec only rules on what a
writer emits, no writer emits both (Java still writes only `source-id`), and
PyIceberg already rejects the pair as mutually exclusive, so accepting a
matching pair only let the two readers disagree. 8fcd795 now rejects
`source-id` and `source-ids` together, including when they agree.
##########
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:
Done in 8fcd795, with your wording.
##########
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:
This is deliberate for now: `field.source_id()?` here is what makes binding
a multi-argument field fail, which
`test_binding_a_multi_argument_field_fails_loudly` pins. Checking every source
id needs a multi-input `Transform::result_type`, and a bound `PartitionField`
still holds one id, so that check belongs with the `PartitionField` conversion
(#2802). I added a comment in 8fcd795 so the intent is visible at the call site.
--
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]
