-
Notifications
You must be signed in to change notification settings - Fork 920
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 scan min/max support for chrono types to libcudf reduction-scan (not groupby scan) #9518
Add scan min/max support for chrono types to libcudf reduction-scan (not groupby scan) #9518
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-21.12 #9518 +/- ##
================================================
- Coverage 10.79% 10.66% -0.13%
================================================
Files 116 117 +1
Lines 18869 19733 +864
================================================
+ Hits 2036 2104 +68
- Misses 16833 17629 +796
Continue to review full report at Codecov.
|
Looks like we should be able to support the following for scan (inclusive):
All other types (DICTIONARY32, LIST, STRUCT) are not supported. The I could include support for DURATION/SUM in this PR (including the gtests). I did not realize we were just missing one case. But I'm inclined to adding into a follow up PR instead to keep this one focused on min/max support for which the target issue is requesting. |
I'm fine with that. I would really like to see this code moved away from using a handrolled type/operator support matrix to using something like |
This one is particularly offensive because the |
So this is only reduction scan, right? And for groupby scan it is not yet supported? If so then we should clarify that in the PR title/description. |
@gpucibot merge |
Depends on #9518 Reference #9518 (comment) Add support for inclusive scan of duration types with SUM aggregation. Also refactor the `scan_tests.cpp` to put rank tests into `rank_tests.cpp`, common utilities into `scan_tests.hpp`, and move the actual unit test logic out of `cudf::test` namespace. Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Jake Hemstad (https://github.com/jrhemstad) - Nghia Truong (https://github.com/ttnghia) URL: #9536
Reference #8709
Enable chrono types (timestamp, duration) support in
cudf::scan
with inclusive scan-type. The only requirement for min/max scan is for the type to be relationally-comparable. The code already supported string type for min/max so the change here only required generalizing that approach.New gtests were added to
scan_tests.cpp
for all chrono types. Also, theto_strings
specialization in the column test utilities was enhanced to show timestamp-type-specific details which is helpful when usingcudf::test::print
on timestamp types.Note that this does not include groupby scan support but only reduction's scan.