Feature/sort enforcement refactor #5228
Conversation
|
For context, this is the first follow-on PR to address one of the minor issues that were discussed in the original PR (#5171). @mingmwang, PTAL. Thanks! |
There was a problem hiding this comment.
I am not an expert in this code, so I would really like @mingmwang to review as well prior to merge.
However, I went over the plan changes and new test and they all make sense to me and the fact that the existing tests all pass means I am 👍 for this PR.
Thank you @mustafasrepo
cc @crepererum who may also be interested in this PR as we are reworking some of our plan construction in IOx as well
| " RepartitionExec: partitioning=RoundRobinBatch(10), input_partitions=10", | ||
| " RepartitionExec: partitioning=RoundRobinBatch(10), input_partitions=0", | ||
| " MemoryExec: partitions=0, partition_sizes=[]", |
There was a problem hiding this comment.
This plan definitely looks better than the input.
| " FilterExec: NOT non_nullable_col@1", | ||
| " SortExec: [nullable_col@0 ASC]", | ||
| " SortExec: [nullable_col@0 ASC]", | ||
| " FilterExec: NOT non_nullable_col@1", |
There was a problem hiding this comment.
This plan change looks better to me as well (do filtering before sort)
|
I will review this PR carefully today. |
|
The change LGTM. |
|
Thanks @mustafasrepo @mingmwang and @ozankabak 🚀 |
|
Benchmark runs are scheduled for baseline = 3da7902 and contender = 9565887. 9565887 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
* Remove multilayer chain Ordering comparison for sort parallelize rule * Update tree code * Simplify if condition * Update test * Simplify sort insertion utility to avoid clones --------- Co-authored-by: Mehmet Ozan Kabak <[email protected]>
Which issue does this PR close?
Closes #.
Rationale for this change
As suggested by @mingmwang in the discussion. Comparing ordering between schemas when there are multiple executors in between them may produce incorrect results. Although this situation may not arise in practice with the current ordering of the optimization rules. We have changed the implementation in case one encounters it (Nobody knows).
What changes are included in this PR?
With this change parallelize sort rule compares the immediate table for converting
Coalesce + Executors that do not require SingleDistribution + Sortcascades toExecutors that do not require SingleDistribution + Sort + SortPreservingMergecascade.We also added a check that the final Sort in the cascade rule is actually a Global sort.
Are these changes tested?
We added a unit test to verify that rule only ends when Sort is a global sort. Also, the existing test checks the sort comparison is done between immediate executors.
Are there any user-facing changes?