moomindani commented on code in PR #3267:
URL: https://github.com/apache/iceberg-rust/pull/3267#discussion_r4130025969
##########
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:
Done in 1d48025 — the builder is public. Since that lets an empty
`source_ids` be built, serializing one no longer panics, and both
`UnboundPartitionSpecBuilder::add_partition_field` and
`PartitionSpecBuilder::add_unbound_field` reject it with a clear error, so the
check happens where the field joins a spec rather than at construction.
##########
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:
Done in 1d48025.
##########
crates/iceberg/src/spec/partition.rs:
##########
Review Comment:
Done in 1d48025: `add_partition_field` now takes an `UnboundPartitionField`,
which also lets it carry a multi-argument field. This is a breaking change to a
public method; all call sites in the workspace are updated.
##########
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:
Done in 1d48025, thanks.
--
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]
