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/cascades: add transformation rule PushTopNDownProjection #13855

Merged
merged 9 commits into from
Dec 9, 2019
Merged

planner/cascades: add transformation rule PushTopNDownProjection #13855

merged 9 commits into from
Dec 9, 2019

Conversation

hsqlu
Copy link
Contributor

@hsqlu hsqlu commented Dec 2, 2019

What problem does this PR solve?

This PR adds Transformation rule PushTopNDownProjectionwhich is a part of TopN push down in cascades planner.

What is changed and how it works?

The logic is the same with planner/core/rule_topn_push_down.go/LogicalProjection.pushDownTopN.

Check List

Tests

  • Unit test

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 2, 2019
@codecov
Copy link

codecov bot commented Dec 2, 2019 •
edited
Loading

Codecov Report

Merging #13855 into master will increase coverage by 0.2618%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #13855        +/-   ##
================================================
+ Coverage   80.1669%   80.4288%   +0.2618%     
================================================
  Files           481        480         -1     
  Lines        120496     121255       +759     
================================================
+ Hits          96598      97524       +926     
+ Misses        16185      16035       -150     
+ Partials       7713       7696        -17

@francis0407 francis0407 added the sig/planner SIG: Planner label Dec 2, 2019
francis0407
francis0407 reviewed Dec 2, 2019
View reviewed changes
Copy link
Member

@francis0407 francis0407 left a comment

Choose a reason for hiding this comment

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

Thanks for you contribution. Please address these comment.

planner/cascades/transformation_rules.go Outdated Show resolved Hide resolved
planner/cascades/transformation_rules.go Outdated Show resolved Hide resolved
@hsqlu hsqlu force-pushed the cascades-optimize-topn branch from 1b6ad73 to 5077c3d Compare December 4, 2019 08:30
@hsqlu hsqlu requested a review from a team as a code owner December 4, 2019 08:30
@ghost ghost removed their request for review December 4, 2019 08:30
@hsqlu hsqlu force-pushed the cascades-optimize-topn branch from 42752d3 to 9d19259 Compare December 4, 2019 10:15
francis0407
francis0407 reviewed Dec 4, 2019
View reviewed changes
Copy link
Member

@francis0407 francis0407 left a comment

Choose a reason for hiding this comment

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

LGTM

@francis0407 francis0407 added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 4, 2019
winoros
winoros reviewed Dec 4, 2019
View reviewed changes
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

rest codes lgtm

planner/cascades/transformation_rules.go Show resolved Hide resolved
@winoros winoros mentioned this pull request Dec 4, 2019
Open
48 tasks
@hsqlu hsqlu force-pushed the cascades-optimize-topn branch from 7670917 to 3b20d1e Compare December 5, 2019 11:00
@hsqlu
Copy link
Contributor Author

hsqlu commented Dec 5, 2019

Conflicts resolved. PTAL @winoros @francis0407 Thanks

@hsqlu hsqlu force-pushed the cascades-optimize-topn branch from 22a10fe to 05b8aa1 Compare December 5, 2019 11:48
@hsqlu hsqlu force-pushed the cascades-optimize-topn branch from 9cdd2a2 to 224f39a Compare December 6, 2019 08:25
@hsqlu hsqlu force-pushed the cascades-optimize-topn branch from 5ce2a4e to a9d34d9 Compare December 6, 2019 09:25
@hsqlu hsqlu force-pushed the cascades-optimize-topn branch 2 times, most recently from 2c90bb3 to 741aa3d Compare December 6, 2019 10:04
@hsqlu hsqlu force-pushed the cascades-optimize-topn branch from 741aa3d to eb4a3eb Compare December 6, 2019 11:24
@hsqlu
Copy link
Contributor Author

hsqlu commented Dec 6, 2019

PTAL. @winoros @francis0407 @lamxTyler Thanks

winoros
winoros previously approved these changes Dec 6, 2019
View reviewed changes
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@hsqlu hsqlu force-pushed the cascades-optimize-topn branch from e36f4ed to 386e92a Compare December 9, 2019 05:19
@hsqlu hsqlu force-pushed the cascades-optimize-topn branch from 386e92a to fa47301 Compare December 9, 2019 05:24
@hsqlu
Copy link
Contributor Author

hsqlu commented Dec 9, 2019

Hi, what the causes of this failed unit-test?

@francis0407
Copy link
Member

/rebuild

@hsqlu hsqlu force-pushed the cascades-optimize-topn branch from fa47301 to ce46dd6 Compare December 9, 2019 07:04
francis0407
francis0407 approved these changes Dec 9, 2019
View reviewed changes
Copy link
Member

@francis0407 francis0407 left a comment

Choose a reason for hiding this comment

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

LGTM

@francis0407 francis0407 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Dec 9, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

/run-all-tests

@sre-bot sre-bot merged commit dd622a9 into pingcap:master Dec 9, 2019
@hsqlu hsqlu deleted the cascades-optimize-topn branch December 9, 2019 10:53
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@hsqlu @XiaTianliang
planner/cascades: add transformation rule PushTopNDownProjection (pin…
03694af 
…gcap#13855)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alivxxx alivxxx alivxxx left review comments

@francis0407 francis0407 francis0407 approved these changes

@winoros winoros winoros left review comments

@eurekaka eurekaka Awaiting requested review from eurekaka

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. sig/planner SIG: Planner 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.
5 participants
@hsqlu @francis0407 @sre-bot @winoros @alivxxx