Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

planner: fix wrong selection push down when having above agg #27021

Merged
merged 6 commits into from
Aug 16, 2021
Merged

planner: fix wrong selection push down when having above agg #27021

merged 6 commits into from
Aug 16, 2021

Conversation

lzmhhh123
Copy link
Contributor

@lzmhhh123 lzmhhh123 commented Aug 9, 2021 •
edited
Loading

Signed-off-by: lzmhhh123 lzmhhh123@gmail.com

What problem does this PR solve?

Issue Number: close #26496

What is changed and how it works?

How it Works:

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression: Consumes more CPU

Release note 

fix wrong selection push down when having above agg

@lzmhhh123
planner: fix wrong selection push down when having above agg
1680959 
Signed-off-by: lzmhhh123 <lzmhhh123@gmail.com>
@lzmhhh123
fix test
d91433f 
Signed-off-by: lzmhhh123 <lzmhhh123@gmail.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 9, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • guo-shaoge
  • wshwsh12

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 9, 2021
@github-actions github-actions bot added the sig/execution SIG execution label Aug 9, 2021
@ichn-hu ichn-hu mentioned this pull request Aug 9, 2021
Closed
wshwsh12
wshwsh12 reviewed Aug 15, 2021
View reviewed changes
planner/core/testdata/ordered_result_mode_suite_out.json
" └─TableReader_17 8000.00 root data:HashAgg_12",
" └─HashAgg_12 8000.00 cop[tikv] group by:test.t1.d, funcs:min(test.t1.a)->Column#11, funcs:max(test.t1.b)->Column#12, funcs:sum(test.t1.c)->Column#13",
" └─TableFullScan_15 10000.00 cop[tikv] table:t1 keep order:false, stats:pseudo"
"Selection_8 6400.00 root lt(Column#6, 20)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the selection_8 can't push down to sort?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a tradeoff. For the correct result. Actually, it can be pushed down to sort.

@wshwsh12 wshwsh12 self-requested a review August 15, 2021 15:17
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 16, 2021
wshwsh12
wshwsh12 approved these changes Aug 16, 2021
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Maybe we need create another issue to track the plan regression.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 16, 2021
@lzmhhh123
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: d91433f

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 16, 2021
@ti-chi-bot ti-chi-bot merged commit 09ebd0d into pingcap:master Aug 16, 2021
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 failed

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 16, 2021
@lzmhhh123 @ti-srebot
cherry pick pingcap#27021 to release-5.1
27243c1 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 16, 2021
Merged
3 tasks
@ti-srebot
Copy link
Contributor

cherry pick to release-5.1 in PR #27258

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 16, 2021
@lzmhhh123 @ti-srebot
cherry pick pingcap#27021 to release-5.2
9b24110 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 16, 2021
Merged
3 tasks
@ti-srebot
Copy link
Contributor

cherry pick to release-5.2 in PR #27259

@lzmhhh123 lzmhhh123 deleted the bug-fix/fix_having_agg_eliminate branch August 16, 2021 16:36
ti-chi-bot pushed a commit that referenced this pull request Aug 17, 2021
@ti-srebot
planner: fix wrong selection push down when having above agg (#27021) (…
a179a5f 
…#27259)
@XuHuaiyu XuHuaiyu added sig/planner SIG: Planner and removed sig/execution SIG execution labels Aug 19, 2021
@zhouqiang-cl
Copy link
Contributor

/run-cherry-picker

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Sep 1, 2021
@lzmhhh123 @ti-srebot
cherry pick pingcap#27021 to release-4.0
fe832b3 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Sep 1, 2021
Merged
3 tasks
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #27741

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Sep 1, 2021
@lzmhhh123 @ti-srebot
cherry pick pingcap#27021 to release-5.0
c4ad71a 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Sep 1, 2021
Merged
3 tasks
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #27742

ti-chi-bot pushed a commit that referenced this pull request Sep 3, 2021
@ti-srebot
planner: fix wrong selection push down when having above agg (#27021) (…
171d556 
…#27742)
ti-chi-bot pushed a commit that referenced this pull request Sep 13, 2021
@ti-srebot
planner: fix wrong selection push down when having above agg (#27021) (…
fc92ef9 
…#27741)
ti-chi-bot pushed a commit that referenced this pull request Sep 15, 2021
@ti-srebot
planner: fix wrong selection push down when having above agg (#27021) (…
a894e34 
…#27258)
@time-and-fate time-and-fate mentioned this pull request Nov 24, 2021
Closed
This was referenced Mar 16, 2022
Closed
Merged
ti-chi-bot pushed a commit that referenced this pull request Mar 17, 2022
@wjhuang2016
*: revert #27021 to fix a bug that selection can not be pushed down w…
47e4b5b 
…hen having clause above aggregation (#33168)

close #33166
This was referenced Mar 17, 2022
Closed
Closed
Closed
Closed
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wshwsh12 wshwsh12 wshwsh12 approved these changes

@guo-shaoge guo-shaoge guo-shaoge approved these changes

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

@winoros winoros Awaiting requested review from winoros

Assignees
No one assigned
Labels
needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 needs-cherry-pick-release-5.2 release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/planner SIG: Planner size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

having clause does not work correctly
7 participants
@lzmhhh123 @ti-chi-bot @ti-srebot @zhouqiang-cl @guo-shaoge @wshwsh12 @XuHuaiyu