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

executor: control Chunk size for Joiners #9614

Merged
merged 16 commits into from
Mar 19, 2019
Merged

executor: control Chunk size for Joiners #9614

merged 16 commits into from
Mar 19, 2019

Conversation

qw4990
Copy link
Contributor

@qw4990 qw4990 commented Mar 8, 2019 •
edited
Loading

What problem does this PR solve?

Control the number of rows in chunks returned by joiner and join-like executors.
Following up #9452, this PR is a subtask of #9166.

What is changed and how it works?

For MergeJoin, HashJoin and IndexLookupJoin, support requiredRows from parents and push requiredRows down to its outer child if it is outer join.

Check List

Tests

  • Unit test

@qw4990 qw4990 requested review from lysu, zz-jason, XuHuaiyu, alivxxx and winoros and removed request for lysu and zz-jason March 8, 2019 05:50
@qw4990 qw4990 self-assigned this Mar 8, 2019
@qw4990 qw4990 added the sig/execution SIG execution label Mar 8, 2019
@codecov-io
Copy link

codecov-io commented Mar 8, 2019 •
edited by codecov bot
Loading

Codecov Report

Merging #9614 into master will increase coverage by 0.0294%.
The diff coverage is 87.5%.

@@               Coverage Diff                @@
##             master      #9614        +/-   ##
================================================
+ Coverage   67.2176%   67.2471%   +0.0294%     
================================================
  Files           381        381                
  Lines         79851      79874        +23     
================================================
+ Hits          53674      53713        +39     
+ Misses        21389      21380         -9     
+ Partials       4788       4781         -7

@qw4990 qw4990 changed the title Chunk size control joins executor: control Chunk size for Joiners Mar 11, 2019
alivxxx
alivxxx reviewed Mar 11, 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 the status/LGT1 Indicates that a PR has LGTM 1. label Mar 11, 2019
@qw4990 qw4990 requested review from lysu, zz-jason, XuHuaiyu and winoros and removed request for lysu, XuHuaiyu, zz-jason and winoros March 11, 2019 12:08
@qw4990 qw4990 mentioned this pull request Mar 11, 2019
Merged
@qw4990 qw4990 requested a review from eurekaka March 13, 2019 02:40
@qw4990
Copy link
Contributor Author

qw4990 commented Mar 13, 2019

@lysu @zz-jason PTAL~

lysu
lysu reviewed Mar 15, 2019
View reviewed changes
Copy link
Contributor

@lysu lysu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

rest LGTM and @zz-jason PTAL

executor/merge_join.go Show resolved Hide resolved
zz-jason
zz-jason reviewed Mar 19, 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.

hash join also need to consider the "is outer join" property.

executor/joiner.go Outdated Show resolved Hide resolved
executor/index_lookup_join.go Outdated Show resolved Hide resolved
executor/merge_join.go Outdated Show resolved Hide resolved
lzmhhh123
lzmhhh123 reviewed Mar 19, 2019
View reviewed changes
executor/index_lookup_join.go Outdated Show resolved Hide resolved
executor/index_lookup_join.go Show resolved Hide resolved
@qw4990
Copy link
Contributor Author

qw4990 commented Mar 19, 2019

All comments have been addressed, PTAL @zz-jason @lzmhhh123

zz-jason
zz-jason reviewed Mar 19, 2019
View reviewed changes
executor/joiner.go Outdated Show resolved Hide resolved
lzmhhh123
lzmhhh123 reviewed Mar 19, 2019
View reviewed changes
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/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 19, 2019
zz-jason
zz-jason approved these changes Mar 19, 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

@qw4990
Copy link
Contributor Author

qw4990 commented Mar 19, 2019

/run-all-tests

@qw4990 qw4990 merged commit 821af9e into pingcap:master Mar 19, 2019
kolbe pushed a commit to kolbe/tidb that referenced this pull request Mar 20, 2019
@qw4990 @kolbe
executor: control Chunk size for Joiners (pingcap#9614)
49735ab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzmhhh123 lzmhhh123 lzmhhh123 left review comments

@alivxxx alivxxx alivxxx left review comments

@lysu lysu lysu left review comments

@zz-jason zz-jason zz-jason approved these changes

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

@winoros winoros Awaiting requested review from winoros

@eurekaka eurekaka Awaiting requested review from eurekaka

Assignees

@qw4990 qw4990

Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@qw4990 @codecov-io @lysu @zz-jason @alivxxx @lzmhhh123