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 PushTopNDownUnionAll #14078

Closed
wants to merge 17 commits into from
Closed

planner/cascades: add transformation rule PushTopNDownUnionAll #14078

wants to merge 17 commits into from

Conversation

Reminiscent
Copy link
Contributor

@Reminiscent Reminiscent commented Dec 16, 2019 •
edited
Loading

What problem does this PR solve?

This PR adds Transformation rule PushTopNDownUnionAll, which 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/LogicalUnion.pushDownTopN.

Check List

Tests

  • Unit test

@Reminiscent Reminiscent requested a review from a team as a code owner December 16, 2019 10:43
@ghost ghost requested review from eurekaka and lzmhhh123 and removed request for a team December 16, 2019 10:43
@codecov
Copy link

codecov bot commented Dec 16, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@cc635db). Click here to learn what that means.
The diff coverage is 32%.

@@             Coverage Diff             @@
##             master     #14078   +/-   ##
===========================================
  Coverage          ?   80.1546%           
===========================================
  Files             ?        483           
  Lines             ?     121540           
  Branches          ?          0           
===========================================
  Hits              ?      97420           
  Misses            ?      16362           
  Partials          ?       7758

lzmhhh123
lzmhhh123 reviewed Dec 16, 2019
View reviewed changes
planner/cascades/transformation_rules.go Outdated Show resolved Hide resolved
planner/cascades/transformation_rules.go Outdated

newTopN.ByItems = make([]*plannercore.ByItems, 0, len(topN.ByItems))
for _, by := range topN.ByItems {
newTopN.ByItems = append(newTopN.ByItems, &plannercore.ByItems{by.Expr, by.Desc})
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you ever test TopN by items from different union children? I guess you should divide the TopN by each child's schema.

Copy link
Member

Choose a reason for hiding this comment

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

PlanBuilder ensures that all of the UnionAll's children have the same schema, by adding Projections below the UnionAll. So we have no need to deal with the schema here. It will be done by PushTopNDownProjection.

@lzmhhh123 lzmhhh123 added sig/planner SIG: Planner type/enhancement The issue or PR belongs to an enhancement. labels Dec 16, 2019
francis0407
francis0407 reviewed Dec 17, 2019
View reviewed changes
planner/cascades/testdata/transformation_rules_suite_out.json Outdated Show resolved Hide resolved
planner/cascades/transformation_rules.go Outdated Show resolved Hide resolved
planner/cascades/transformation_rules.go Outdated

newTopN.ByItems = make([]*plannercore.ByItems, 0, len(topN.ByItems))
for _, by := range topN.ByItems {
newTopN.ByItems = append(newTopN.ByItems, &plannercore.ByItems{by.Expr, by.Desc})
Copy link
Member

Choose a reason for hiding this comment

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

PlanBuilder ensures that all of the UnionAll's children have the same schema, by adding Projections below the UnionAll. So we have no need to deal with the schema here. It will be done by PushTopNDownProjection.

francis0407
francis0407 reviewed Dec 17, 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.

Please add a test which topN has both offset and count.

@winoros winoros mentioned this pull request Dec 17, 2019
Open
48 tasks
@francis0407 francis0407 mentioned this pull request Dec 17, 2019
Merged
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
@Reminiscent Reminiscent deleted the TopNnionAll branch December 24, 2019 12:14
@Reminiscent Reminiscent restored the TopNnionAll branch December 24, 2019 12:17
@Reminiscent Reminiscent reopened this Dec 24, 2019
@Reminiscent
Copy link
Contributor Author

Reminiscent commented Dec 25, 2019 •
edited
Loading

To #14214
Sorry for my wrong operation, I can only re-create a new pr to complete this task.

@francis0407 francis0407 added the duplicate Issues or pull requests already exists. label Dec 25, 2019
@Reminiscent Reminiscent deleted the TopNnionAll branch August 5, 2021 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@francis0407 francis0407 francis0407 left review comments

@eurekaka eurekaka Awaiting requested review from eurekaka

@alivxxx alivxxx Awaiting requested review from alivxxx

@lzmhhh123 lzmhhh123 Awaiting requested review from lzmhhh123

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. duplicate Issues or pull requests already exists. sig/planner SIG: Planner type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Reminiscent @alivxxx @francis0407 @lzmhhh123 @sre-bot @sticnarf @pingyu