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

plan: push down constant filters over join properly #9848

Merged
merged 3 commits into from
Apr 4, 2019
Merged

plan: push down constant filters over join properly #9848

merged 3 commits into from
Apr 4, 2019

Conversation

eurekaka
Copy link
Contributor

@eurekaka eurekaka commented Mar 21, 2019 •
edited
Loading

What problem does this PR solve?

Fix #9843

What is changed and how it works?

For filter expression which does not contain columns:

  • if the condition is filter condition, push it down to both children of join, whatever the join type is;
  • if the condition is join condition, push it down to inner child of join if the join is outer join, or both children if the join is not outer join;

Check List

Tests

  • Unit test
  • Integration test

Code changes

N/A

Side effects

N/A

Related changes

  • Need to cherry-pick to the release branch

@eurekaka eurekaka added type/bugfix This PR fixes a bug. sig/planner SIG: Planner labels Mar 21, 2019
@ngaut
Copy link
Member

ngaut commented Mar 22, 2019

/run-all-tests

@eurekaka eurekaka changed the title plan: keep constant filters in OtherConditions of join plan: push down constant filters over join properly Mar 25, 2019
@codecov
Copy link

codecov bot commented Mar 25, 2019 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master      #9848   +/-   ##
===========================================
  Coverage          ?   77.2275%           
===========================================
  Files             ?        405           
  Lines             ?      81682           
  Branches          ?          0           
===========================================
  Hits              ?      63081           
  Misses            ?      13926           
  Partials          ?       4675

@eurekaka
Copy link
Contributor Author

/run-all-tests

@eurekaka
Copy link
Contributor Author

/run-all-tests

@eurekaka eurekaka marked this pull request as ready for review March 25, 2019 13:32
@eurekaka
Copy link
Contributor Author

/rebuild

@eurekaka
Copy link
Contributor Author

/run-all-tests

eurekaka
eurekaka commented Mar 27, 2019
View reviewed changes
cmd/explaintest/r/explain_easy.result Outdated
│ └─Selection_40 2.40 cop eq(3, test.t.a)
│ └─IndexScan_39 3.00 cop table:s, index:b, range:[3,3], keep order:false
└─TableReader_48 4.00 root data:Selection_47
└─Selection_47 4.00 cop eq(3, test.t.a)
Copy link
Contributor Author

@eurekaka eurekaka Mar 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

For reviewers: the row count of Selection_47 and TableReader_48 is wrong, but this should be a separate issue and I would fix it in another PR later.

planner/core/logical_plan_test.go
@@ -906,7 +906,7 @@ func (s *testPlanSuite) TestJoinReOrder(c *C) {
},
{
sql: "select * from t o where o.b in (select t3.c from t t1, t t2, t t3 where t1.a = t3.a and t2.a = t3.a and t2.a = o.a and t1.a = 1)",
best: "Apply{DataScan(o)->Join{Join{DataScan(t3)->DataScan(t1)}->DataScan(t2)}->Projection}->Projection",
best: "Apply{DataScan(o)->Join{Join{DataScan(t1)->DataScan(t2)}->DataScan(t3)}->Projection}->Projection",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@winoros Please confirm this join order change does not effect correctness.

Copy link
Member

@winoros winoros Mar 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

THe join order changed since some filter pushed before but now is not.

winoros
winoros reviewed Mar 27, 2019
View reviewed changes
planner/core/logical_plan_builder.go
@@ -180,6 +180,33 @@ func (b *PlanBuilder) buildResultSetNode(node ast.ResultSetNode) (p LogicalPlan,
}
}

// pushDownConstExpr checks if the condition is from filter condition, if true, push it down to both
Copy link
Member

Choose a reason for hiding this comment

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

What does filter here mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To differentiate the where conditions from join conditions, I use filter condition here. I can change it to another name if it is misleading.

Copy link
Member

Choose a reason for hiding this comment

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

So it should be the conditions exclude the join key conditions? If so, can deriveLeft || deriveRight match its meaning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

extractOnCondition is called by PredicatePushDown and atttachOnConds, in PredicatePushDown, the input conditions are treated as filter conditions, and at least deriveLeft or deriveRight is true; in attachOnConds, the input conditions are join conditions, and deriveLeft and deriveRight are both false, so it should be correct?

@eurekaka eurekaka requested review from winoros and alivxxx March 27, 2019 09:46
zz-jason
zz-jason reviewed Apr 4, 2019
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 4, 2019
alivxxx
alivxxx previously approved these changes Apr 4, 2019
View reviewed changes
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

@alivxxx alivxxx added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 4, 2019
@eurekaka eurekaka merged commit 8800ec1 into pingcap:master Apr 4, 2019
@eurekaka eurekaka deleted the extract_on branch April 4, 2019 07:50
eurekaka added a commit to eurekaka/tidb that referenced this pull request Apr 4, 2019
@eurekaka
plan: push down constant filters over join properly (pingcap#9848)
b1f17d1
@eurekaka eurekaka mentioned this pull request Apr 4, 2019
Merged
eurekaka added a commit to eurekaka/tidb that referenced this pull request Apr 4, 2019
@eurekaka
plan: push down constant filters over join properly (pingcap#9848)
33ce21e
@eurekaka eurekaka mentioned this pull request Apr 4, 2019
Merged
ngaut pushed a commit that referenced this pull request Apr 8, 2019
@eurekaka @ngaut
plan: push down constant filters over join properly (#9848) (#10047)
6641576
zz-jason pushed a commit that referenced this pull request Apr 8, 2019
@eurekaka @zz-jason
plan: push down constant filters over join properly (#9848) (#10049)
0f97e0b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zz-jason zz-jason zz-jason left review comments

@winoros winoros winoros approved these changes

@alivxxx alivxxx alivxxx left review comments

Assignees
No one assigned
Labels
sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eurekaka @ngaut @zz-jason @winoros @alivxxx