Copilot commented on code in PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#discussion_r3972458076
##########
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:
By propagating `result_type()` directly, the error may lose useful context
about where it occurred (e.g., which source type/transform pair in which sort
field). Consider wrapping the error with additional context (while preserving
the underlying kind) so failures point clearly to `source_type` and
`field_transform` involved.
##########
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:
PR description says an incompatible transform is rejected with
`ErrorKind::Unexpected`, but this change now propagates the error from
`result_type(source_type)` (tests now expect `ErrorKind::DataInvalid`). Either
update the PR description to match the new/actual error kind, or map the error
here to the intended kind consistently.
##########
crates/iceberg/src/transaction/sort_order.rs:
##########
@@ -44,9 +45,16 @@ impl PendingSortField {
)
})?;
+ if matches!(self.transform, Transform::Unknown | Transform::Void) {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!("Cannot sort by transform {}", self.transform),
Review Comment:
The error message doesn’t identify which column triggered the failure.
Including the column name (and possibly direction/null order) would make
commit-time failures much easier to diagnose, e.g. mentioning `self.name`
alongside the transform.
##########
crates/iceberg/src/transaction/sort_order.rs:
##########
@@ -159,14 +211,139 @@ 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();
+
+ assert_eq!(updates.len(), 2);
+ let TableUpdate::AddSortOrder { sort_order } = &updates[0] else {
+ panic!("expected AddSortOrder, got {:?}", updates[0]);
+ };
Review Comment:
This test assumes `AddSortOrder` is always at `updates[0]`, which makes it
brittle if update ordering changes (e.g., adding/removing requirements or
emitting updates in a different order). Prefer finding the
`TableUpdate::AddSortOrder` entry by iterating/matching over `updates` rather
than indexing by position.
--
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]
