-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add hash_join_single_partition_threshold_rows config #8720
Conversation
9c2af7b
to
d9c9d34
Compare
@korowa may also be interested in this |
Change looks good to me in general, could you also update the tests @maruschin so we can check the new behavior? |
ece73a9
to
42028a9
Compare
Hi @Dandandan, I fixed the tests and refactored variables, making the names more explicit. In general, the tests have not changed. |
@Dandandan as the author of #8405 would you possibly have time to review this PR again? If not I will find time to do so |
To me the change looks good, I think it makes sense to run the (TPC-H) benchmarks once more to catch regressions. |
I will do so |
I merged this branch with |
Here is the result of the benchmarks when I ran them. My conclusion is that there is no measurable difference in performance due to this branch so it should be good to go
|
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.
Thanks agian @maruschin
Thanks for checking it @alamb and for the PR @maruschin |
Which issue does this PR close?
Closes #8405.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?