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.

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

planner: some cleanup #11164

Merged
merged 5 commits into from
Jul 12, 2019
Merged

planner: some cleanup #11164

merged 5 commits into from
Jul 12, 2019

Conversation

winoros
Copy link
Member

@winoros winoros commented Jul 10, 2019

What problem does this PR solve?

Three cleanup.

What is changed and how it works?

  1. Use NewPlanBuilder to create new builder.
  2. Memory table don't need union scan operator.
  3. code cleanup in partition pruning.

Check List

Tests

  • Existing Unit test

@winoros winoros added the sig/planner SIG: Planner label Jul 10, 2019
@codecov
Copy link

codecov bot commented Jul 10, 2019

Codecov Report

Merging #11164 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master    #11164   +/-   ##
=========================================
  Coverage   81.463%   81.463%           
=========================================
  Files          423       423           
  Lines        91002     91002           
=========================================
  Hits         74133     74133           
  Misses       11523     11523           
  Partials      5346      5346

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

@@ -60,16 +59,16 @@ func (s *partitionProcessor) rewriteDataSource(lp LogicalPlan) (LogicalPlan, err
// Union->(UnionScan->DataSource1), (UnionScan->DataSource2)
children := make([]LogicalPlan, 0, len(ua.Children()))
for _, child := range ua.Children() {
us := LogicalUnionScan{}.Init(ua.ctx)
us := LogicalUnionScan{conditions: p.conditions}.Init(ua.ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line a bugfix? can we figure out a test case to cover it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, another separate PR #11187 is working on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

merged master branch

@eurekaka eurekaka added the type/enhancement The issue or PR belongs to an enhancement. label Jul 11, 2019
@winoros winoros requested a review from eurekaka July 11, 2019 03:17
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

@eurekaka eurekaka added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 12, 2019
@winoros winoros merged commit 6cd0027 into pingcap:master Jul 12, 2019
@winoros winoros deleted the planner-clean-up branch July 12, 2019 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner 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.

3 participants