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

plan: new plan support index look up join. #3281

Merged
merged 13 commits into from May 21, 2017
Merged

Conversation

hanfei1991
Copy link
Member

@hanfei1991 hanfei1991 commented May 17, 2017

I give up the old index look up join which uses apply and create a new indexJoin Plan.

@shenli @zimulala @winoros PTAL

@hanfei1991 hanfei1991 added Priority/P1 Features that will be implemented in the latest or next major/minor version non-release-blocker labels May 17, 2017
@@ -113,6 +113,168 @@ func (p *Projection) convert2NewPhysicalPlan(prop *requiredProp) (taskProfile, e
return task, p.storeTaskProfile(prop, task)
}

func (p *LogicalJoin) getIndexJoinKey(inner Plan) []*expression.Column {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this function since it is not called by others.

@@ -133,7 +295,7 @@ func (p *LogicalJoin) convert2NewPhysicalPlan(prop *requiredProp) (taskProfile,
default:
if p.preferUseMergeJoin() {
task, err = p.convert2MergeJoin(prop)
} else {
} else if task, err = p.tryToGetIndexJoin(prop); task == nil && err == nil {
// TODO: We will consider index look up join in the future.
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this comment

plan/plan.go Outdated
@@ -113,6 +113,15 @@ type requiredProp struct {
taskTp taskType
}

func (p *requiredProp) checkMatchSchema(schema *expression.Schema) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is not called by others.

return matchOffsets
}

func (p *LogicalJoin) convertToIndexJoin(prop *requiredProp, outerIdx int) (taskProfile, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment for outerIdx.

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

Rest LGTM.


// convertToIndexJoin will generate index join by required properties and outerIndex. OuterIdx points out the outer child,
// because we will swap the children of join when the right child is outer child.
// First of all, we will extract th join keys for p's equal conditions. If the join keys can match some of the indices or pk
Copy link
Contributor

Choose a reason for hiding this comment

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

s/th/the

return task, nil
}

func (p *LogicalJoin) tryToGetIndexJoin(prop *requiredProp) (taskProfile, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment for this function.

@hanfei1991 hanfei1991 added the status/LGT1 Indicates that a PR has LGTM 1. label May 18, 2017
@XuHuaiyu
Copy link
Contributor

LGTM

sql: "select /*+ TIDB_INLJ(t1, t2) */ * from t t1 left outer join t t2 on t1.a = t2.a and t2.b < 1",
best: "IndexJoin{TableReader(Table(t))->TableReader(Table(t)->Sel([lt(t2.b, 1)]))}(t1.a,t2.a)",
},
// Test Index Join failed.
Copy link
Member

Choose a reason for hiding this comment

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

Join failed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fail to use index join

@@ -133,7 +295,7 @@ func (p *LogicalJoin) convert2NewPhysicalPlan(prop *requiredProp) (taskProfile,
default:
if p.preferUseMergeJoin() {
task, err = p.convert2MergeJoin(prop)
} else {
} else if task, err = p.tryToGetIndexJoin(prop); task == nil && err == nil {
// TODO: We will consider index look up join in the future.
Copy link
Member

Choose a reason for hiding this comment

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

Should we update this comment?

return nil
}
for i, col := range p.tableInfo.Columns {
if mysql.HasPriKeyFlag(col.Flag) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider composed primary key?

Copy link
Member Author

Choose a reason for hiding this comment

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

composed primary key is index

if !found {
return nil
}
if i+1 == len(keys) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not "keys match all columns in index." but "all keys match columns in index."

@hanfei1991
Copy link
Member Author

@shenli PTAL

Copy link
Member

@shenli shenli left a comment

Choose a reason for hiding this comment

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

LGTM

@hanfei1991 hanfei1991 merged commit f57f99a into master May 21, 2017
@hanfei1991 hanfei1991 deleted the hanfei/inlj-plan branch May 21, 2017 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority/P1 Features that will be implemented in the latest or next major/minor version status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants