-
Notifications
You must be signed in to change notification settings - Fork 908
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
Implement cudf::rolling
for decimal32
and decimal64
#7037
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-0.18 #7037 +/- ##
===============================================
+ Coverage 82.09% 82.11% +0.02%
===============================================
Files 97 97
Lines 16477 16480 +3
===============================================
+ Hits 13527 13533 +6
+ Misses 2950 2947 -3
Continue to review full report at Codecov.
|
return data_type{type_to_id<target_type_t<Source, k>>()}; | ||
auto const id = type_to_id<target_type_t<Source, k>>(); | ||
return id == type_id::DECIMAL32 || id == type_id::DECIMAL64 ? data_type{id, type.scale()} | ||
: data_type{id}; |
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 skip scale for non-decimal type? (It will have default value, anyway it won't be used).
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.
Ctor with scale only designed to be called for Decimal types.
explicit data_type(type_id id, int32_t scale) : _id{id}, _fixed_point_scale{scale}
{
assert(id == type_id::DECIMAL32 || id == type_id::DECIMAL64);
}
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.
Looks good, just a question.
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.
Surprisingly small change 👍
This PR resolves a part of #3556.
Aggregation ops supported:
MIN
MAX
COUNT
(bothnull_policy
-EX/INCLUDE
)LEAD
LAG
To Do List: