viirya commented on code in PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#discussion_r3972352422
##########
crates/iceberg/src/transaction/sort_order.rs:
##########
@@ -65,24 +66,68 @@ 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()`).
+ ///
+ /// Note: `Term` is currently a plain column reference. Once it becomes
+ /// transform-carrying (#2665), sort-order declaration is expected to
converge on
+ /// Term-based `asc`/`desc` (as in Java's `SortOrderBuilder`), at which
point the
+ /// `_with_transform` variants can be deprecated in its favor.
+ pub fn asc_with_transform(
Review Comment:
Fixed in 0fba334f. Added the guard in `PendingSortField::to_sort_field`,
returning `DataInvalid` at commit time while preserving the builder API. Added
tests covering both transforms through both ascending and descending methods.
##########
crates/iceberg/src/transaction/sort_order.rs:
##########
@@ -65,24 +66,68 @@ 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()`).
+ ///
+ /// Note: `Term` is currently a plain column reference. Once it becomes
+ /// transform-carrying (#2665), sort-order declaration is expected to
converge on
+ /// Term-based `asc`/`desc` (as in Java's `SortOrderBuilder`), at which
point the
+ /// `_with_transform` variants can be deprecated in its favor.
+ pub fn asc_with_transform(
+ self,
+ name: &str,
+ transform: Transform,
+ null_order: NullOrder,
+ ) -> Self {
+ self.add_sort_field(name, transform, SortDirection::Ascending,
null_order)
+ }
+
+ /// Adds a field for sorting in descending order by a transform of the
column's value
Review Comment:
Done in 0fba334f. Kept the shared validation and Term-convergence notes on
`asc_with_transform` and added a cross-reference from `desc_with_transform`.
##########
crates/iceberg/src/transaction/sort_order.rs:
##########
@@ -159,14 +209,115 @@ mod tests {
assert_eq!(replace_sort_order.pending_sort_fields, vec![
PendingSortField {
name: String::from("x"),
+ transform: Transform::Identity,
direction: SortDirection::Ascending,
null_order: NullOrder::First,
},
PendingSortField {
name: String::from("y"),
+ transform: Transform::Identity,
direction: SortDirection::Descending,
null_order: NullOrder::Last,
}
]);
}
+
+ #[test]
+ fn test_replace_sort_order_with_transform() {
+ let table = make_v2_table();
+ let tx = Transaction::new(&table);
+ let replace_sort_order = tx.replace_sort_order();
+
+ let tx = replace_sort_order
+ .asc_with_transform("x", Transform::Bucket(16), NullOrder::First)
+ .desc_with_transform("y", Transform::Truncate(4), NullOrder::Last)
+ .apply(tx)
+ .unwrap();
+
+ let replace_sort_order = (*tx.actions[0])
+ .downcast_ref::<ReplaceSortOrderAction>()
+ .unwrap();
+
+ assert_eq!(replace_sort_order.pending_sort_fields, vec![
+ PendingSortField {
+ name: String::from("x"),
+ transform: Transform::Bucket(16),
+ direction: SortDirection::Ascending,
+ null_order: NullOrder::First,
+ },
+ PendingSortField {
+ name: String::from("y"),
+ transform: Transform::Truncate(4),
+ direction: SortDirection::Descending,
+ null_order: NullOrder::Last,
+ }
+ ]);
+ }
+
+ #[tokio::test]
+ async fn test_replace_sort_order_with_transform_commits() {
+ let table = make_v2_table();
+ let action = Arc::new(ReplaceSortOrderAction::new().asc_with_transform(
+ "x",
+ Transform::Bucket(16),
+ NullOrder::First,
+ ));
+
+ let mut action_commit = TransactionAction::commit(action,
&table).await.unwrap();
+ let updates = action_commit.take_updates();
+
+ let sort_order = match &updates[0] {
Review Comment:
Done in 0fba334f. Added length assertions for both `updates` and
`sort_order.fields`, and switched to `let ... else`. For the error-path test, I
used `.err().expect(...)` because `ActionCommit` does not implement `Debug`,
which `expect_err` requires.
##########
crates/iceberg/src/transaction/sort_order.rs:
##########
@@ -159,14 +209,115 @@ mod tests {
assert_eq!(replace_sort_order.pending_sort_fields, vec![
PendingSortField {
name: String::from("x"),
+ transform: Transform::Identity,
direction: SortDirection::Ascending,
null_order: NullOrder::First,
},
PendingSortField {
name: String::from("y"),
+ transform: Transform::Identity,
direction: SortDirection::Descending,
null_order: NullOrder::Last,
}
]);
}
+
+ #[test]
+ fn test_replace_sort_order_with_transform() {
+ let table = make_v2_table();
+ let tx = Transaction::new(&table);
+ let replace_sort_order = tx.replace_sort_order();
+
+ let tx = replace_sort_order
+ .asc_with_transform("x", Transform::Bucket(16), NullOrder::First)
+ .desc_with_transform("y", Transform::Truncate(4), NullOrder::Last)
+ .apply(tx)
+ .unwrap();
+
+ let replace_sort_order = (*tx.actions[0])
+ .downcast_ref::<ReplaceSortOrderAction>()
+ .unwrap();
+
+ assert_eq!(replace_sort_order.pending_sort_fields, vec![
+ PendingSortField {
+ name: String::from("x"),
+ transform: Transform::Bucket(16),
+ direction: SortDirection::Ascending,
+ null_order: NullOrder::First,
+ },
+ PendingSortField {
+ name: String::from("y"),
+ transform: Transform::Truncate(4),
+ direction: SortDirection::Descending,
+ null_order: NullOrder::Last,
+ }
+ ]);
+ }
+
+ #[tokio::test]
+ async fn test_replace_sort_order_with_transform_commits() {
+ let table = make_v2_table();
+ let action = Arc::new(ReplaceSortOrderAction::new().asc_with_transform(
+ "x",
+ Transform::Bucket(16),
+ NullOrder::First,
+ ));
+
+ let mut action_commit = TransactionAction::commit(action,
&table).await.unwrap();
+ let updates = action_commit.take_updates();
+
+ let sort_order = match &updates[0] {
+ TableUpdate::AddSortOrder { sort_order } => sort_order,
+ other => panic!("expected AddSortOrder, got {other:?}"),
+ };
+ assert_eq!(sort_order.fields[0].transform, Transform::Bucket(16));
+ }
+
+ #[tokio::test]
+ async fn test_replace_sort_order_rejects_incompatible_transform() {
+ let table = make_v2_table();
+ // `x` is a `long` column; `year` only accepts date/timestamp types.
+ let action = Arc::new(ReplaceSortOrderAction::new().asc_with_transform(
+ "x",
+ Transform::Year,
+ NullOrder::First,
+ ));
+
+ let err = match TransactionAction::commit(action, &table).await {
+ Err(e) => e,
+ Ok(_) => panic!("year transform on a long column should be
rejected"),
+ };
+ assert_eq!(err.kind(), ErrorKind::Unexpected);
Review Comment:
Agreed. Fixed in 0fba334f by propagating the original error with
`result_type(source_type)?`. Updated both the sort builder test and the
transaction test to assert `DataInvalid`.
--
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]
