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: introduce an individual physical plan for indexNestedLoopHashJoin #12139

Merged
merged 8 commits into from
Sep 24, 2019

Conversation

XuHuaiyu
Copy link
Contributor

@XuHuaiyu XuHuaiyu commented Sep 10, 2019

I'll split this PR into small ones:

What problem does this PR solve?

Introduce an individual physical plan for IndexNestedLoopHashJoin.

What is changed and how it works?

PhysicalIndexHashJoin will be constructed when len(prop.Item) == 0 is true.

Check List

Tests

Exists tests.

Code changes

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

Side effects

  • Increased code complexity

Related changes

N/A

Release note

N/A

@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12139   +/-   ##
===========================================
  Coverage   81.3278%   81.3278%           
===========================================
  Files           454        454           
  Lines        100449     100449           
===========================================
  Hits          81693      81693           
  Misses        12941      12941           
  Partials       5815       5815

@XuHuaiyu XuHuaiyu force-pushed the index_hashjoin_outer_order branch 2 times, most recently from 7441627 to 558eaa3 Compare September 17, 2019 09:22
executor/adapter.go Outdated Show resolved Hide resolved
executor/joiner.go Show resolved Hide resolved
planner/core/exhaust_physical_plans.go Outdated Show resolved Hide resolved
path *accessPath,
compareFilters *ColWithCmpFuncManager,
) []PhysicalPlan {
// TODO(xuhuaiyu): support keep outer order for indexHashJoin.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need it? I thought in this case index merge join would always performs better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some cases need to keep order but index merge join can't make it. For example:

We need to join tables t1 and t2 on t1.c = t2.c, t1 is the outer table. But it requires to keep order with t1.a, t1.b. The index merge join can only make sure of keeping t1.c order.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the reason why we also need index lookup join now.

path *accessPath,
compareFilters *ColWithCmpFuncManager,
) []PhysicalPlan {
// TODO(xuhuaiyu): support keep outer order for indexHashJoin.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some cases need to keep order but index merge join can't make it. For example:

We need to join tables t1 and t2 on t1.c = t2.c, t1 is the outer table. But it requires to keep order with t1.a, t1.b. The index merge join can only make sure of keeping t1.c order.

path *accessPath,
compareFilters *ColWithCmpFuncManager,
) []PhysicalPlan {
// TODO(xuhuaiyu): support keep outer order for indexHashJoin.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's the reason why we also need index lookup join now.

planner/core/task.go Outdated Show resolved Hide resolved
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

executor/builder.go Outdated Show resolved Hide resolved
@XuHuaiyu
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM.

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

sre-bot commented Sep 24, 2019

Your auto merge job has been accepted, waiting for 12239

@lzmhhh123 lzmhhh123 added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 24, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 24, 2019

/run-all-tests

@sre-bot sre-bot merged commit 1fe9773 into pingcap:master Sep 24, 2019
@XuHuaiyu XuHuaiyu deleted the index_hashjoin_outer_order branch September 25, 2019 01:55
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/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants