feat(transaction): Implement TransactionAction for ReplaceSortOrderAction #1441
Conversation
| direction: SortDirection, | ||
| null_order: NullOrder, | ||
| } | ||
|
|
There was a problem hiding this comment.
PendingSortField is added to defer the construction of Vec<SortField> from asc/desc to commit to avoid passing Table to methods like asc
There was a problem hiding this comment.
Maybe add this as comment for PendingSortField
| /// Adds a field for sorting in ascending order. | ||
| pub fn asc(self, name: &str, null_order: NullOrder) -> Result<Self> { | ||
| pub fn asc(self, name: &str, null_order: NullOrder) -> Self { |
There was a problem hiding this comment.
I noticed that in iceberg-java user can provide an expression here to indicates the transform of the sort field. We can do this here or open issue to do in the future.
There was a problem hiding this comment.
Good point! I think supporting Transform based sorting in ReplaceSortOrderAction is a bigger effort and should be treated separately. I've created an issue here to track: #1443
| let sort_fields: Result<Vec<SortField>> = pending_sort_fields | ||
| .iter() | ||
| .map(|p| { | ||
| let field_id = table |
There was a problem hiding this comment.
how about convert code here as a function of PendingSortField, e.g. PendingSortField::to_sort_field
Which issue does this PR close?
Related Issues:
Transaction::commitmethod #1387TableMetadatato new location. #1388What changes are included in this PR?
TransactionActiontrait forReplaceSortOrderActionAre these changes tested?
Added unit tests