laskoviymishka commented on code in PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#discussion_r3966647306
##########
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:
One footgun the new public surface opens up: `asc_with_transform(name,
Transform::Unknown, ...)` and `Transform::Void` both sail through commit-time
validation. `Unknown.result_type()` returns `Ok(String)` and
`Void.result_type()` returns `Ok(input_type)`, so `check_compatibility` accepts
them and we happily write `"transform": "unknown"` / `"transform": "void"` to
metadata.
`Unknown` is our read-side forward-compat sentinel — there's no spec string
for it, and no other client can execute it (Java's `UnknownTransform.apply()`
throws, PyIceberg can't apply it at all). Java's builder never lets you
construct one; you only reach `UnknownTransform` by parsing existing metadata,
so this is a path Java deliberately doesn't expose. `Void` loads everywhere but
produces an all-null sort key — a silent no-op.
I'd guard both with a `DataInvalid` in `add_sort_field` (or
`to_sort_field`), something like `matches!(transform, Transform::Unknown |
Transform::Void)`. Cheap, and it keeps us from minting sort orders no client
can actually order by. wdyt?
##########
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:
These two doc blocks are character-for-character identical apart from the
first line. I'd keep the shared text in one place and cross-reference — e.g.
`See [Self::asc_with_transform]` — so the commit-time-validation and #2665
notes don't drift out of sync later. Minor, non-blocking.
##########
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:
This pins `Unexpected` for what's really invalid caller input — a `year`
transform on a `long` column is a `DataInvalid` situation, not "the runtime hit
a state we didn't anticipate." The root cause is one layer down in
`spec/sort.rs`: `check_compatibility` calls `result_type(...).is_err()`, throws
away the original error (which is already `DataInvalid` from `transform.rs`),
and re-wraps it as `Unexpected`.
That's pre-existing, but this PR's new API is the first public path that
actually reaches it, and this test now locks the wrong kind in. I'd at minimum
not cement it — better still, propagate the original in `sort.rs`
(`result_type(source_type)?;`) and assert `DataInvalid` here. If we'd rather
defer the `sort.rs` fix, a comment noting the known inconsistency plus a
follow-up issue would keep the test reading as intent rather than accident.
##########
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:
Small thing: this indexes `updates[0]` and then `sort_order.fields[0]`
without asserting the lengths first, so an empty list gives an opaque index
panic instead of a readable failure. The round-trip test below already does
`assert_eq!(fields.len(), 2)` first — worth matching that here with an
`assert_eq!(updates.len(), 2, ...)`.
While we're in these tests, `let ... else { panic! }` (or `assert_matches!`)
reads a bit cleaner than the `match { ... panic! }`, and the error-path test
just below could be `let err = commit(...).await.expect_err("year on a long
column should be rejected")`.
##########
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);
+ }
+
+ #[tokio::test]
+ async fn test_sort_order_transform_survives_metadata_json_round_trip() {
+ // Commit a transform-based sort order through a real catalog: the
memory
+ // catalog serializes the updated table metadata to a metadata.json
file
+ // (`TableMetadata::write_to`), and `load_table` reads that file back
and
+ // parses it (`TableMetadata::read_from`). This exercises the full
+ // JSON round-trip of the transform (e.g. `"bucket[16]"`), not just the
+ // in-memory `TableUpdate`.
+ let catalog = new_memory_catalog().await;
+ let table = make_v3_minimal_table_in_catalog(&catalog).await;
+
+ let tx = Transaction::new(&table);
+ let tx = 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 committed = tx.commit(&catalog).await.unwrap();
+
+ // Reload from the catalog: this parses the metadata.json written
above.
+ let reloaded =
catalog.load_table(committed.identifier()).await.unwrap();
+ let sort_order = reloaded.metadata().default_sort_order();
+
+ assert_eq!(sort_order.fields.len(), 2);
+ assert_eq!(sort_order.fields[0].transform, Transform::Bucket(16));
+ assert_eq!(sort_order.fields[0].direction, SortDirection::Ascending);
Review Comment:
Since this one's billed as the full round-trip, I'd assert `null_order`
survives too — right now we check transform and direction but not that
`First`/`Last` came back. One more `assert_eq!` per field closes the gap.
--
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]
