You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "viirya (via GitHub)" <gi...@apache.org> on 2026/07/03 17:48:51 UTC
[PR] feat(transaction): support transform-based sort orders [iceberg-rust]
viirya opened a new pull request, #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765
## What changes are included in this PR?
`ReplaceSortOrderAction` only supported sorting by a column's raw value (an implicit identity transform) — `asc`/`desc` hardcoded `Transform::Identity` and only accepted a column name. There was no way to declare a sort order using a transform (`bucket[N]`, `year`, `truncate[W]`, etc.), which Java's `SortOrderBuilder.asc/desc(Term, NullOrder)` supports.
Adds `asc_with_transform` / `desc_with_transform`:
```rust
tx.replace_sort_order()
.asc_with_transform("event_time", Transform::Year, NullOrder::First)
.desc_with_transform("id", Transform::Bucket(16), NullOrder::Last);
```
`asc`/`desc` are unchanged (same signature, same behavior) — they now delegate to the new methods with `Transform::Identity`, so no existing caller is affected.
Whether a transform is valid for the source column's type is checked at commit time (via the existing `SortOrder::builder().build(schema)` → `check_compatibility`), matching the timing of Java's `SortOrder.Builder.build()` — an incompatible transform is only rejected once the table schema is available, not at the `asc_with_transform` call site.
### Scope
This only extends the **metadata-declaration** API — which sort order (including which transform) can be recorded on a table. Whether the write path actually sorts a `RecordBatch` to match a table's declared sort order is a separate, pre-existing gap: no writer in `crates/iceberg/src/writer/` reads `SortOrder` or sorts data today, for *any* sort order, including the pre-existing identity case. This PR does not touch that; it only makes a wider set of sort orders declarable.
## Are these changes tested?
- `test_replace_sort_order_with_transform` — the builder records the requested transform per field.
- `test_replace_sort_order_with_transform_commits` — end-to-end commit; the resulting `TableUpdate::AddSortOrder`'s `SortField` carries the requested transform.
- `test_replace_sort_order_rejects_incompatible_transform` — `Transform::Year` on a `long` column is rejected at commit time with `ErrorKind::Unexpected`, exercising the existing `check_compatibility` path.
- Existing `test_replace_sort_order` (plain `asc`/`desc`) passes unchanged.
Full `iceberg` lib suite passes; clippy and rustfmt clean.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "TwinklerG (via GitHub)" <gi...@apache.org>.
TwinklerG commented on PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#issuecomment-5176054573
I am refactoring it and it will come back soon.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#issuecomment-5767967138
Thank you @laskoviymishka @mbutrovich @huan233usc @NikitaMatskevich
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#issuecomment-5533872074
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#discussion_r3972354009
##########
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:
Done in 0fba334f. The round-trip test now also asserts `NullOrder::First` for the first field and `NullOrder::Last` for the second field after reloading the metadata.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#issuecomment-5018427327
Hi @viirya @mbutrovich does it make sense to review && push #2665 to be merged first, I think it should be the long term goal and match Java's behavior and capability. @TwinklerG is working with me internally to complete the term support and I think we can priorities the term/transform support if there's enough review input.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#issuecomment-4908722805
@CTTY Could you take a look at this? Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#issuecomment-5607717804
Thanks @laskoviymishka! Addressed all five comments in 0fba334f, including the Unknown/Void guard and preserving `DataInvalid` for incompatible transforms.
All 1,380 library tests pass, along with Clippy for all iceberg targets/features and formatting checks. Ready for another look—thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "NikitaMatskevich (via GitHub)" <gi...@apache.org>.
NikitaMatskevich commented on code in PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#discussion_r3524973406
##########
crates/iceberg/src/transaction/sort_order.rs:
##########
@@ -159,14 +195,90 @@ 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() {
+ use std::sync::Arc;
+
+ use crate::TableUpdate;
+
+ 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();
Review Comment:
Can we also add an integration test that would write a metadata.json with updated sort order, and then another operation would read this json and correctly parse the transform?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#issuecomment-5072518242
CC @CTTY @blackmwk @kevinjqliu for committer review, as time permits. Thank you in advance!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#issuecomment-5733147186
Thanks @laskoviymishka for the follow-up review and approval! In 9082bf7a, I made the remaining two validation branches return `DataInvalid` and documented the intentional `Void` restriction on the action API. All 36 sort-related tests and formatting checks pass.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#discussion_r3526989390
##########
crates/iceberg/src/transaction/sort_order.rs:
##########
@@ -159,14 +195,90 @@ 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() {
+ use std::sync::Arc;
+
+ use crate::TableUpdate;
+
+ 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();
Review Comment:
Good idea — added in eee40b8f: `test_sort_order_transform_survives_metadata_json_round_trip`.
It commits a transform-based sort order (`bucket[16]` asc + `truncate[4]` desc) through the in-crate memory catalog, which serializes the updated table metadata to an actual metadata.json file (`TableMetadata::write_to` on `FileIO`), then reloads the table via `catalog.load_table` — which reads and parses that file back (`TableMetadata::read_from`) — and asserts both transforms and directions survive. So the transform string (`"bucket[16]"` etc.) is exercised through the real serialize-file-parse path, not just the in-memory `TableUpdate`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "TwinklerG (via GitHub)" <gi...@apache.org>.
TwinklerG commented on PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#issuecomment-5077285852
> This LGTM after revision. Thanks for addressing the feedback, @viirya! I agree with @advancedxy that it might be nice to get #2665 in first, but its diff is over 10x the number of lines, has no reviews at the moment, and merge conflicts. I will take a review pass on #2665, but IMO it doesn't make sense to block this PR on #2665 at this point. #2665 can be adjusted to adopt the feedback we presented here.
Thanks @mbutrovich ! I am the author of #2665 . I agree with you. I do not think it is suitable to merge it soon as it is ready for production, although I think it deserves a review.
Currently I am still working with @advancedxy on #2665 to make the APIs and inner implementation better. It is not stable now, but its idea remains the same——expand `Term` to enum and add support for Transform Bound Predicate such as `bucket(10, "id") = 5`, `year(ts) = 54`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#issuecomment-5072347817
Thank you @mbutrovich. I agree.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #2765:
URL: https://github.com/apache/iceberg-rust/pull/2765#issuecomment-4963387565
Agreed on the direction, @mbutrovich — converging on a transform-carrying `Term` is the better end state. I checked #2665 and it is indeed turning `Term` (today just `pub type Term = Reference`) into an enum, so once that lands, a single `asc(term, null_order)` mirrors Java's actual type rather than its overload, and the `_with_transform` pair becomes redundant.
Done in afe2c270:
- Added the convergence note to both `asc_with_transform`/`desc_with_transform` doc comments — they're documented as the minimal non-breaking step today, expected to be deprecated in favor of Term-based `asc`/`desc` after #2665.
- Hoisted the scattered in-test `use` statements to the `mod tests` top (nit 1).
On nit 2 — agreed there's overlap; keeping both since the unit test pinpoints the `ActionCommit` output in isolation while the round-trip covers the serde path, and the unit test is where a failure would localize faster. Cheap to keep.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transaction): support transform-based sort orders [iceberg-rust]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org