Skip to content

planner: support basic Show in cascades planner#12185

Merged
sre-bot merged 11 commits intopingcap:masterfrom
zz-jason:cascades/support-basic-show
Sep 17, 2019
Merged

planner: support basic Show in cascades planner#12185
sre-bot merged 11 commits intopingcap:masterfrom
zz-jason:cascades/support-basic-show

Conversation

@zz-jason
Copy link
Member

Signed-off-by: Zhang Jian zjsariel@gmail.com

What problem does this PR solve?

  1. Make it possible to build a physical plan for LogicalShow in the cascades planner
  2. Refactor some code comments

What is changed and how it works?

  1. Add a new operand for LogicalShow.
  2. Add an implementation rule for the newly added operand which implements LogicalShow to PhysicalShow.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

Signed-off-by: Zhang Jian <zjsariel@gmail.com>
@zz-jason zz-jason added type/enhancement The issue or PR belongs to an enhancement. sig/planner SIG: Planner labels Sep 14, 2019
@codecov
Copy link

codecov bot commented Sep 17, 2019

Codecov Report

Merging #12185 into master will decrease coverage by 0.0284%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #12185        +/-   ##
================================================
- Coverage   81.1538%   81.1254%   -0.0285%     
================================================
  Files           454        454                
  Lines         98450      98450                
================================================
- Hits          79896      79868        -28     
- Misses        12815      12831        +16     
- Partials       5739       5751        +12

Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

Any test?

@zz-jason
Copy link
Member Author

Any test?

Added. PTAL again, thx.

Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

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

logicProp := expr.Group.Prop
show := expr.ExprNode.(*plannercore.LogicalShow)

// TODO(zz-jason): unifying LogicalShow and PhysicalShow to a single
Copy link
Member

Choose a reason for hiding this comment

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

Here also comes up with a question, for operators that only has one physical operator. Do we need to split it to logical one and physical one?

@winoros winoros added the status/can-merge Indicates a PR has been approved by a committer. label Sep 17, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 17, 2019

/run-all-tests

@zz-jason zz-jason added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 17, 2019
@sre-bot sre-bot merged commit e685ce4 into pingcap:master Sep 17, 2019
@zz-jason zz-jason deleted the cascades/support-basic-show branch March 17, 2022 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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. type/enhancement The issue or PR belongs to an enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants