-
Notifications
You must be signed in to change notification settings - Fork 915
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
Fix has_atomic_support check in can_use_hash_groupby() #10588
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-22.06 #10588 +/- ##
================================================
- Coverage 86.31% 86.31% -0.01%
================================================
Files 140 140
Lines 22300 22255 -45
================================================
- Hits 19249 19210 -39
+ Misses 3051 3045 -6
Continue to review full report at Codecov.
|
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.
Nice, I hadn't even thought of this as a solution. Very clean.
@gpucibot merge |
Thanks @jrhemstad, @PointKernel, and @ttnghia for the reviews, and @jlowe for help debugging this problem. |
Closes #10583.
Change the has_atomic_support check in can_use_hash_groupby() to check the target type for the aggregation instead of the source type.
See discussion in #10583.
I have verified that this fixes the performance regression in our customer queries, and all unit tests still pass.