-
Notifications
You must be signed in to change notification settings - Fork 912
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 union operation in _is_supported()
#7959
Fix union operation in _is_supported()
#7959
Conversation
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 @charlesbluca ! Looks good.
I would normally ask you to add a test that includes a groupby test that is not supported by the optimized code path. However, it looks like any aggregation that is not supported by that path is broken in upstream dask anyway...
Thanks @rjzamora! Would a good compromise be to add some tests for |
Yeah - Although I'd rather not add a (possibly fragile) test for a private function, it's probably better than no test coverage at all. Maybe just test that including a completely made-up aggregation name will return |
Sure! I added a simple test for returning |
Also I think I need a category label for the rest of the checks to run |
I think gpuci is just currently down due to a network outage. |
rerun tests |
Is gpuCI up and running for cuDF again? Reran the tests yesterday and looks like they might have timed out |
rerun tests |
Codecov Report
@@ Coverage Diff @@
## branch-0.20 #7959 +/- ##
===============================================
+ Coverage 82.30% 82.49% +0.19%
===============================================
Files 101 103 +2
Lines 17053 17306 +253
===============================================
+ Hits 14035 14277 +242
- Misses 3018 3029 +11
Continue to review full report at Codecov.
|
@gpucibot merge |
Changes the
_global_set
union operation happening in_is_supported()
toSince
set.union()
doesn't actually modify the set in place. Before this PR, passing something like{"a": ["unsupported_agg"]}
into_is_supported()
would always returnTrue
.cc @rjzamora