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, executor: index join enhancement #8471

Merged
merged 42 commits into from
Apr 29, 2019

Conversation

winoros
Copy link
Member

@winoros winoros commented Nov 27, 2018

What problem does this PR solve?

  1. Originally, we use constant 1 to build fake eq condition when try to build index join. It worked before, but now there're some cases it cannot satisfy. fixes Fail to choose index join in when index has time column. #8244
  2. We want that the case like select * from t, tt where t.a=tt.a and tt.b >= t.b and tt.b < t.b+20 can use tt.b >= t.b and tt.b < t.b+20 to build index join and access the inner table.

What is changed and how it works?

We analyze the join key and filters as the following procedures:

  1. find the join key with index' column. To see that whether it can be used.
  2. find the filters that is in form column eq constant or column in (constant list).
  3. find the filters that is in form of column from inner table compareOp expression only contain outer table's column and not a constant value.
    3.1 union first two part, construct the template ranges that will be used in execution phase.
  4. if 3 find nothing. We call ranger to build range for the column.
    4.1 union this three part, construct the template ranges that will be used in execution phase.

When in execution phase, if there's re expressions found in 3. We will calculate it, build range for it and append it to the tail of the template range.

Check List

Tests

  • Unit test // test in planner is added.
  • Integration test
  • Manual test

Code changes

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

Side effects

  • Increased code complexity

Related changes

  • Need to update the documentation
  • Need to be included in the release note

This change is Reviewable

@winoros winoros added sig/planner SIG: Planner sig/execution SIG execution labels Nov 27, 2018
@winoros winoros force-pushed the indexjoin-enhancement branch from 6fd3dbf to 5bdb46f Compare November 27, 2018 09:53
@winoros winoros force-pushed the indexjoin-enhancement branch from 7e053bf to 029c6fd Compare November 27, 2018 11:14
@winoros
Copy link
Member Author

winoros commented Nov 27, 2018

/run-common-test
/run-sqllogic-test

@winoros winoros force-pushed the indexjoin-enhancement branch from a2685c7 to 3f4c9b9 Compare November 28, 2018 06:01
}
if len(nextColCmpFilterManager.OpType) == 0 {
Copy link
Member Author

@winoros winoros Nov 28, 2018

Choose a reason for hiding this comment

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

We can let the manager hold the constant range too.
But this will lead to this situation: If there's no filter need to be computed at runtime. We can actually set the range information in planner. But if we use the manager hold the constant range, the constant range will be set during execution phase.

But this can save some codes.

Which one do you prefer?

planner/core/exhaust_physical_plans.go Show resolved Hide resolved
access, eqConds, remained, keyOff2IdxOff := p.buildFakeEqCondsForIndexJoin(innerJoinKeys, idxCols, colLengths, innerPlan.pushedDownConds)
// ColWithCompareOps is used in index join to handle the column with compare functions(>=, >, <, <=).
// It stores the compare functions and build ranges in execution phase.
type ColWithCompareOps struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we have a better name for this struct.

executor/builder.go Outdated Show resolved Hide resolved
indexRanges []*ranger.Range
keyOff2IdxOff []int
indexRanges []*ranger.Range
nextColCompareFilters *plannercore.ColWithCompareOps
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we have a better name for this variable.

@winoros
Copy link
Member Author

winoros commented Nov 28, 2018

We need to modify the explain result of index join.
The main change now is okay to review.
Unit-test will be added later

@winoros
Copy link
Member Author

winoros commented Dec 11, 2018

When adding unit-test, found some incorrect result when index have prefix-string column.
It needs some time to solve it.

@winoros winoros force-pushed the indexjoin-enhancement branch from 4e082a3 to d9d51d8 Compare December 13, 2018 11:31
@winoros winoros changed the title [WIP] planner, executor: index join enhancement planner, executor: index join enhancement Dec 13, 2018
@winoros
Copy link
Member Author

winoros commented Dec 13, 2018

/run-all-tests

@winoros
Copy link
Member Author

winoros commented Dec 17, 2018

UPD: The failed case is caused by the incorrect behavior of MutRow, not the pr itself.

@winoros
Copy link
Member Author

winoros commented Dec 18, 2018

The failed case is fixed by #8725

@winoros winoros force-pushed the indexjoin-enhancement branch from b13e136 to 7631437 Compare April 16, 2019 07:13
@winoros winoros requested review from zz-jason and qw4990 and removed request for zz-jason April 16, 2019 07:32
@qw4990 qw4990 requested review from qw4990 and removed request for qw4990 April 16, 2019 09:07
@winoros winoros force-pushed the indexjoin-enhancement branch from 6ff3cc1 to 19a06f3 Compare April 22, 2019 03:40
@winoros winoros added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Apr 28, 2019
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 28, 2019
@alivxxx
Copy link
Contributor

alivxxx commented Apr 28, 2019

@winoros Please fix the conflicts.

// If the filter contains index column covered by join keys, it will be useless since we always use join key to build range for that index column..
for _, innerFilter := range innerPlan.pushedDownConds {
affectedCols := expression.ExtractColumns(innerFilter)
if expression.ColumnSliceIsIntersect(affectedCols, ijHelper.curPossibleUsedKeys) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that curPossibleUsedKeys is always empty here because we build them in removeUselessEqAndInFunc, which is after this.

Copy link
Member Author

@winoros winoros Apr 28, 2019

Choose a reason for hiding this comment

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

Removed this for loop.

@winoros
Copy link
Member Author

winoros commented Apr 28, 2019

./run-all-tests

@winoros winoros requested a review from alivxxx April 28, 2019 13:55
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 29, 2019
@alivxxx alivxxx merged commit 9d74d64 into pingcap:master Apr 29, 2019
@winoros winoros deleted the indexjoin-enhancement branch May 7, 2019 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/release-blocker This issue blocks a release. Please solve it ASAP. sig/execution SIG execution sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail to choose index join in when index has time column.
5 participants