ozankabak commented on code in PR #5661:
URL: https://github.com/apache/arrow-datafusion/pull/5661#discussion_r1155147104
##########
datafusion/core/src/physical_optimizer/sort_enforcement.rs:
##########
@@ -1432,22 +1514,22 @@ mod tests {
let physical_plan = sort_preserving_merge_exec(sort_exprs, union);
// Input is an invalid plan. In this case rule should add required
sorting in appropriate places.
- // First ParquetExec has output ordering(nullable_col@0 ASC). However,
it doesn't satisfy required ordering
- // of SortPreservingMergeExec. Hence rule should remove unnecessary
sort for second child of the UnionExec
- // and put a sort above Union to satisfy required ordering.
+ // First ParquetExec has output ordering(nullable_col@0 ASC). However,
it doesn't satisfy the
+ // required ordering of SortPreservingMergeExec.
let expected_input = vec![
"SortPreservingMergeExec: [nullable_col@0 ASC,non_nullable_col@1
ASC]",
" UnionExec",
" ParquetExec: limit=None, partitions={1 group: [[x]]},
output_ordering=[nullable_col@0 ASC], projection=[nullable_col,
non_nullable_col]",
" SortExec: expr=[nullable_col@0 ASC,non_nullable_col@1 ASC]",
" ParquetExec: limit=None, partitions={1 group: [[x]]},
projection=[nullable_col, non_nullable_col]",
];
- // should remove unnecessary sorting from below and move it to top
+
let expected_optimized = vec![
"SortPreservingMergeExec: [nullable_col@0 ASC,non_nullable_col@1
ASC]",
- " SortExec: expr=[nullable_col@0 ASC,non_nullable_col@1 ASC]",
- " UnionExec",
+ " UnionExec",
Review Comment:
Yes, all looks good here. We move sorts down whenever doing so can result in
sorting smaller datasets. In this case, having the sort above `UnionExec` or
below doesn't matter in that regard, and the default tendency is to push sorts
down in such cases.
--
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]
