Support min and max operations for structs in rolling window
#10332
Conversation
ttnghia
added
feature request
This comment was marked as off-topic.
This comment was marked as off-topic.
ttnghia
added
3 - Ready for Review
cpp/src/rolling/rolling_detail.cuh
Outdated
| @@ -166,31 +178,62 @@ struct DeviceRollingArgMinMax { | |||
| size_type end_index, | |||
| size_type current_index) | |||
| { | |||
| using AggOp = typename corresponding_operator<op>::type; | |||
| AggOp agg_op; | |||
| if constexpr (std::is_same_v<InputType, cudf::string_view>) { | |||
There was a problem hiding this comment.
The operator() function implementation for struct_view and string_view appear to be completely different as shown by this if constexpr expression. Also, the string_view path does not require the member variable agg_op. Since it's operator() and member variables do not align, perhaps they should not be defined in the same functor? Or maybe some other refactoring is possible?
There was a problem hiding this comment.
I would like to share as much common code as possible for the two types, so we can reduce maintenance effort afterward. Yes, their operator() interface seem to be different. I'm totally fine with refactoring them to have different functors.
There was a problem hiding this comment.
Update: I have separated the previous functor into 2 separate ones for string and struct.
cpp/src/rolling/rolling_detail.cuh
Outdated
| [&input](size_type idx) { return input.is_valid_nocheck(idx); }) | ||
| : end_index - start_index; | ||
|
|
||
| // Set -1 will help identify null elements while gathering for Min and Max. |
There was a problem hiding this comment.
I believe the argmin/argmax ops define a sentinel to avoid needing to use a magic number here.
There was a problem hiding this comment.
Yes, so it may be neglected in the original code. I just fixed both (old and new).
|
@gpucibot merge |
This PR adds support for
minandmaxoperations in rolling window for STRUCT type. It also does some minor modifications to the existing code, such as renaming some variables and refining some comments.Partially addresses #8974.