laskoviymishka commented on code in PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#discussion_r4046614599
##########
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:
One wrinkle on the `Void` half of what I asked for, and this is new to me:
Java's `VoidTransform.canTransform` returns `true` for every type, so
`SortOrder.Builder.build()` there happily accepts a void sort field — a
Java-authored table can legitimately carry one (e.g. after a partition field
was voided). Rejecting `Void` outright means we can't round-trip that.
I still think rejecting it is defensible as a deliberate "we won't author
these" policy, and `Unknown` clearly should go regardless. But if we keep the
`Void` rejection I'd drop a one-line comment here noting it's intentionally
stricter than the spec. wdyt — keep it strict, or match Java and let `Void`
through?
##########
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:
This is exactly the change I asked for last round — the transform branch
reports `DataInvalid` now and the test pins it.
The one thing I'd tidy while we're in here: the two branches just above (the
"Cannot find source column" and "Cannot sort by non-primitive source field"
arms around 183/192) still return `Unexpected` for what's really the same class
of user mistake, so the function now hands back two different kinds depending
on which check trips. They're pre-existing and untouched by this PR, so I won't
hold on it — but flipping both to `DataInvalid` here would make the whole
function consistent for callers matching on `.kind()`.
##########
crates/iceberg/src/transaction/sort_order.rs:
##########
@@ -44,9 +45,16 @@ impl PendingSortField {
)
})?;
+ if matches!(self.transform, Transform::Unknown | Transform::Void) {
Review Comment:
The guard does exactly what I wanted on the action path.
One gap: it lives in `to_sort_field`, so a caller building a `SortOrder`
directly via `SortOrder::builder().build()` still gets `Void`/`Unknown` through
unchecked. As the write path grows that second entry point starts to matter.
Not blocking here, but if we ever want the invariant to be uniform,
`check_compatibility` is the spot both paths funnel through. wdyt?
--
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]
