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: Check if the query is killed when joining rows in HashJoin #20270

Merged
merged 13 commits into from
Oct 28, 2020
Merged

executor: Check if the query is killed when joining rows in HashJoin #20270

merged 13 commits into from
Oct 28, 2020

Conversation

TszKitLo40
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #20245

Problem Summary:

What is changed and how it works?

What's Changed:
Check if the query is killed when joining rows in HashJoin
How it Works:
Check if the query is killed in HashJoinExec.join2Chunk and HashJoinExec.JoinMatchedProbeSideRow2Chunk

Check List

Tests

  • Unit test

Release note

  • Check if the query is killed when joining rows in HashJoin

@TszKitLo40 TszKitLo40 requested a review from a team as a code owner September 28, 2020 06:22
@TszKitLo40 TszKitLo40 requested review from SunRunAway and removed request for a team September 28, 2020 06:22
@ti-srebot ti-srebot added the contribution This PR is from a community contributor. label Sep 28, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Sep 28, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@TszKitLo40 TszKitLo40 changed the title Check if the query is killed when joining rows in HashJoin executor: Check if the query is killed when joining rows in HashJoin Sep 28, 2020
@qw4990 qw4990 self-requested a review September 28, 2020 06:24
@github-actions github-actions bot added the sig/execution SIG execution label Sep 28, 2020
qw4990
qw4990 reviewed Oct 12, 2020
View reviewed changes
executor/join.go Outdated Show resolved Hide resolved
qw4990
qw4990 reviewed Oct 13, 2020
View reviewed changes
executor/join_test.go Outdated Show resolved Hide resolved
qw4990
qw4990 reviewed Oct 13, 2020
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 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.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 13, 2020
@qw4990 qw4990 requested a review from wshwsh12 October 16, 2020 02:21
SunRunAway
SunRunAway reviewed Oct 16, 2020
View reviewed changes
executor/join.go Outdated Show resolved Hide resolved
@SunRunAway
Copy link
Contributor

@TszKitLo40 PTAL if free.

@SunRunAway
Copy link
Contributor

Could you add some test cases?

@TszKitLo40
Copy link
Contributor Author

TszKitLo40 commented Oct 21, 2020 •
edited
Loading

Could you add some test cases?

I have add a test in TestIssue20270. Do you mean it is not enough?

Yisaer
Yisaer reviewed Oct 28, 2020
View reviewed changes
Copy link
Contributor

@Yisaer Yisaer 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

executor/join_test.go Outdated Show resolved Hide resolved
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Oct 28, 2020
@qw4990
Copy link
Contributor

qw4990 commented Oct 28, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 28, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@TszKitLo40 merge failed.

@qw4990
Copy link
Contributor

qw4990 commented Oct 28, 2020

/run-unit-test

@qw4990
Copy link
Contributor

qw4990 commented Oct 28, 2020

/reward 600

@ti-challenge-bot
Copy link

This PR's linked issue is not picked.

@TszKitLo40
Copy link
Contributor Author

This PR's linked issue is not picked.

I have picked up this issue.

@qw4990
Copy link
Contributor

qw4990 commented Oct 28, 2020

/reward 600

@ti-challenge-bot
Copy link

Reward success.

@qw4990
Copy link
Contributor

qw4990 commented Oct 28, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@TszKitLo40 merge failed.

@qw4990
Copy link
Contributor

qw4990 commented Oct 28, 2020

/run-common-test
/run-unit-test

@qw4990
Copy link
Contributor

qw4990 commented Oct 28, 2020

/run-unit-test

3 similar comments
@qw4990
Copy link
Contributor

qw4990 commented Oct 28, 2020

/run-unit-test

@qw4990
Copy link
Contributor

qw4990 commented Oct 28, 2020

/run-unit-test

@qw4990
Copy link
Contributor

qw4990 commented Oct 28, 2020

/run-unit-test

@qw4990 qw4990 merged commit 2e4390b into pingcap:master Oct 28, 2020
@ti-challenge-bot
Copy link

@TszKitLo40, Congratulations, you get 600 in this PR, and your total score is 4400 in high-performance challenge program.

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Oct 28, 2020
@TszKitLo40 @ti-srebot
cherry pick pingcap#20270 to release-4.0
0cf6e16 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Oct 28, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #20702

qw4990 pushed a commit that referenced this pull request Oct 29, 2020
@ti-srebot
cherry pick #20270 to release-4.0 (#20702)
5fd5527
@ou-bing ou-bing mentioned this pull request Nov 4, 2020
Merged
@tiancaiamao tiancaiamao mentioned this pull request Nov 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yisaer Yisaer Yisaer left review comments

@qw4990 qw4990 qw4990 left review comments

@SunRunAway SunRunAway SunRunAway left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

@wshwsh12 wshwsh12 Awaiting requested review from wshwsh12

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. 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.

Check if the query is killed when joining rows in HashJoin
6 participants
@TszKitLo40 @sre-bot @SunRunAway @qw4990 @ti-srebot @Yisaer