Fix incorrect SortExec removal before AggregateExec (option 2)#20247
Fix incorrect SortExec removal before AggregateExec (option 2)#20247akurmustafa merged 3 commits intoapache:mainfrom
SortExec removal before AggregateExec (option 2)#20247Conversation
github-actions
bot
added
optimizer
alamb
changed the title
SortExec removal before AggregateExec (option 2)
alamb
force-pushed
the
alamb/fix_monotonic_sort_bug_option2
branch
from
33cc3f2 to
3d6b8cd
Compare
| } | ||
| } | ||
| } else if let Some(aggregate_exec) = plan.as_any().downcast_ref::<AggregateExec>() { | ||
| handle_aggregate_pushdown(aggregate_exec, parent_required) |
There was a problem hiding this comment.
This comment explains why this is needed: #19287 (comment)
Basically the generic version is not correct for AggregateExec, but this was masked due to some limitations that were lifted in #19287
|
@ozankabak @mustafasrepo / @akurmustafa or @berkaysynnada I wonder if you might have time to review this PR (it is a bug fix for a long existing bug) |
There was a problem hiding this comment.
Thank you, @alamb for this PR. I added a new test case and resolved the merge conflicts. This PR looks good to me.
| EXPLAIN SELECT x, SUM(v) | ||
| FROM agg_expr_parquet | ||
| GROUP BY x | ||
| ORDER BY x + 1 DESC; |
There was a problem hiding this comment.
It is not important but, maybe we can change this test such that desired ordering is ORDER BY x + 1 ASC. Because input ordering is already x ASC and in theory this version can be eliminated, not the current version in test as far as I can tell.
There was a problem hiding this comment.
No longer applicable, I added a new testcase with ORDER BY 2*x ASC.
There was a problem hiding this comment.
Thank you -- sorry for the delay -- I was out last week
akurmustafa
left a comment
akurmustafa
left a comment
There was a problem hiding this comment.
I added an additional testcase and resolved the merge conflict. Thank you @alamb for this PR.
| 2 150 | ||
| 1 60 | ||
|
|
||
| # Test 3.12: Aggregate ORDER BY non-column expression (unsatisfied) keeps SortExec |
There was a problem hiding this comment.
I added this test case to showcase in theory final SortExec can be eliminated however currently cannot. I tried to use ORDER BY x +1 ASC. However, in that case there was no SortExec at the end. Hence I added this example.
…on 2) (apache#20247)" This reverts commit 132b043.
…on 2) (apache#20247)" This reverts commit 132b043.
|
Thank you very much @akurmustafa for your help with this issue |
…pache#20247) - Fixes apache#20244 This is an alternatative to - apache#20245 Wrong answers bug was exposed by apache#19287 in 52. See apache#20244 and backstory here - apache#19287 (comment) Fix the bug by properly implemnting Yes, a new test is added A bug is fixed --------- Co-authored-by: Mustafa Akur <[email protected]>
…pache#20247) - Fixes apache#20244 This is an alternatative to - apache#20245 Wrong answers bug was exposed by apache#19287 in 52. See apache#20244 and backstory here - apache#19287 (comment) Fix the bug by properly implemnting Yes, a new test is added A bug is fixed --------- Co-authored-by: Mustafa Akur <[email protected]>
|
I made a backport PR for this one: |
…20247) (#20507) ## Which issue does this PR close? - part of #20287 - Fixes #20287 on branch-52 ## Rationale for this change See issues ## What changes are included in this PR? - backports #20247 ## Are these changes tested? By CI ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> --------- Co-authored-by: Mustafa Akur <[email protected]>
3 participants
Which issue does this PR close?
This is an alternatative to
SortExecremoval beforeAggregateExec#20245Rationale for this change
Wrong answers bug was exposed by #19287 in 52. See #20244 and backstory here
What changes are included in this PR?
Fix the bug by properly implemnting
Are these changes tested?
Yes, a new test is added
Are there any user-facing changes?
A bug is fixed