viirya commented on code in PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#discussion_r4048709441
##########
crates/iceberg/src/spec/sort.rs:
##########
@@ -195,14 +195,7 @@ impl SortOrderBuilder {
}
let field_transform = sort_field.transform;
- if field_transform.result_type(source_type).is_err() {
- return Err(Error::new(
- ErrorKind::Unexpected,
- format!(
- "Invalid source type {source_type} for
transform {field_transform}"
- ),
- ));
- }
+ field_transform.result_type(source_type)?;
Review Comment:
Done in 9082bf7a. Both the missing-column and non-primitive-source branches
now return `DataInvalid`. Updated the existing tests for missing columns, list
fields, and variant fields to assert the error kind as well as the message.
##########
crates/iceberg/src/transaction/sort_order.rs:
##########
@@ -44,9 +45,16 @@ impl PendingSortField {
)
})?;
+ if matches!(self.transform, Transform::Unknown | Transform::Void) {
Review Comment:
Agreed that `check_compatibility` is the shared validation point for both
paths. For this PR, I'd keep the guard scoped to the replace-sort-order action
as an explicit policy for what this API authors.
Given your note about Java accepting `Void`, moving the guard into the
shared builder would broaden that restriction to existing callers. I'd leave
that as a follow-up where we can decide the builder's behavior for `Unknown`
and `Void` separately.
##########
crates/iceberg/src/transaction/sort_order.rs:
##########
@@ -65,24 +73,63 @@ impl ReplaceSortOrderAction {
}
}
- /// Adds a field for sorting in ascending order.
+ /// Adds a field for sorting in ascending order, sorting by the column's
raw value
+ /// (an identity transform). To sort by a transform of the column instead
(e.g.
+ /// `bucket[N]`, `year`, `truncate[W]`), use [`Self::asc_with_transform`].
pub fn asc(self, name: &str, null_order: NullOrder) -> Self {
- self.add_sort_field(name, SortDirection::Ascending, null_order)
+ self.asc_with_transform(name, Transform::Identity, null_order)
}
- /// Adds a field for sorting in descending order.
+ /// Adds a field for sorting in descending order, sorting by the column's
raw value
+ /// (an identity transform). To sort by a transform of the column instead
(e.g.
+ /// `bucket[N]`, `year`, `truncate[W]`), use [`Self::desc_with_transform`].
pub fn desc(self, name: &str, null_order: NullOrder) -> Self {
- self.add_sort_field(name, SortDirection::Descending, null_order)
+ self.desc_with_transform(name, Transform::Identity, null_order)
+ }
+
+ /// Adds a field for sorting in ascending order by a transform of the
column's value
+ /// (e.g. `Transform::Bucket(16)`, `Transform::Year`,
`Transform::Truncate(4)`).
+ ///
+ /// Whether the transform is valid for the column's type is checked at
commit time,
+ /// once the table schema is available (mirroring Java's
`SortOrder.Builder.build()`).
+ /// `Transform::Unknown` and `Transform::Void` are rejected at commit time
with
Review Comment:
I'd keep the action strict. Added a comment and API documentation in
9082bf7a explaining that rejecting `Void` is intentional: an all-null sort key
adds no ordering information.
To clarify the scope, the guard only applies when declaring a replacement
sort order through this action. It does not affect reading or serializing
existing metadata containing `Void`, though recreating that order through this
action would be rejected.
--
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]
