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: tiny code refine for contructIndexJoin #12254

Merged
merged 6 commits into from
Sep 23, 2019

Conversation

XuHuaiyu
Copy link
Contributor

What problem does this PR solve?

Tiny code refine for contructIndexJoin

What is changed and how it works?

Extract 2 functions:
buildIndexJoinInner2TableScan
buildIndexJoinInner2IndexScan

Check List

Tests

Exists tests.

Code changes
N/A

Side effects
N/A

Related changes
N/A

Release note

N/A

@XuHuaiyu XuHuaiyu added the sig/planner SIG: Planner label Sep 18, 2019
@XuHuaiyu
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented Sep 18, 2019

Codecov Report

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

@@            Coverage Diff            @@
##            master    #12254   +/-   ##
=========================================
  Coverage   81.191%   81.191%           
=========================================
  Files          454       454           
  Lines        99176     99176           
=========================================
  Hits         80522     80522           
  Misses       12883     12883           
  Partials      5771      5771

return rightJoins, true
}

if leftJoins != nil && lhsCardinality < rhsCardinality {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this heuristic rule actually? Previously, cost of index join computed only depends on row count of outer child, then this rule can hold, but now we have changed the cost computation of index join to consider inner row count as well, so it would not always be better if outer row count is smaller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we make this change in an individual PR?
This PR only refines the code structure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure.

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/LGT1 Indicates that a PR has LGTM 1. label Sep 23, 2019
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 status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 23, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 23, 2019

/run-all-tests

@sre-bot sre-bot merged commit 3eaa4d3 into pingcap:master Sep 23, 2019
@XuHuaiyu XuHuaiyu deleted the refine_code_4_indexjoin branch September 23, 2019 04:21
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants