-
Notifications
You must be signed in to change notification settings - Fork 411
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
Some code refine in join #6680
Some code refine in join #6680
Conversation
Signed-off-by: gengliqi <[email protected]>
Signed-off-by: gengliqi <[email protected]>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
dbms/src/Interpreters/Join.h
Outdated
@@ -326,8 +326,6 @@ class Join | |||
BlocksList blocks; | |||
/// mutex to protect concurrent insert to blocks | |||
std::mutex blocks_lock; | |||
/// keep original block for concurrent build | |||
Blocks original_blocks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will soonly be used when join support spill, I think we can keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use BlocksList blocks
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the block in blocks
is changed during insertFromBlock
, the key columns are removed or moved to the start of the block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Maybe we can change this behavior in the future. If so, we can also delete the copy of the right join key columns.
Signed-off-by: gengliqi <[email protected]>
/run-all-tests |
Signed-off-by: gengliqi <[email protected]>
Signed-off-by: gengliqi <[email protected]>
/run-all-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
Can you fix a build fail issue in this pr?
void cut(Block & block, size_t rows, size_t limit, size_t pos) ==> void cut(Block & block, size_t rows [[maybe_unused]], size_t limit, size_t pos) :) |
Signed-off-by: gengliqi <[email protected]>
/merge |
@gengliqi: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
This pull request has been accepted and is ready to merge. Commit hash: 7d59600
|
/run-integration-test |
This reverts commit 0b1ffce.
ref pingcap#5900 Signed-off-by: ywqzzy <[email protected]>
Signed-off-by: gengliqi [email protected]
What problem does this PR solve?
Issue Number: ref #5900
Problem Summary:
SavedHash
forserialized
to speed up thekeyEquals
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note