Support Segmented Min/Max Reduction on String Type #10447
Conversation
isVoid
added
feature request
isVoid
changed the title
Co-authored-by: Bradley Dice <[email protected]>
…fea/seg_reduction_dec_type
… removed unused functions in `reduction.cuh`. Co-authored-by: Bradley Dice <[email protected]>
isVoid
changed the title
isVoid
changed the title
Co-authored-by: Bradley Dice <[email protected]>
Co-authored-by: Bradley Dice <[email protected]>
bdice
added
3 - Ready for Review
|
@vuule All these are great review comments! I wouldn't consider them as optional :) |
Co-authored-by: David Wendt <[email protected]>
|
rerun tests |
bdice
added
5 - Ready to Merge
| offsets.begin() + 1, | ||
| null_handling, | ||
| stream, | ||
| mr); |
There was a problem hiding this comment.
A super minor nit: we can determine if segmented_null_mask is a temporary memory based on result->null_count() and choose to use default resource v.s. supplied resource here.
There was a problem hiding this comment.
Feel free to ignore or punt to next one @bdice.
There was a problem hiding this comment.
We are already using mr as efficiently (minimally) as possible. If result->null_count() == 0, we return the mr-allocated bitmask. Else, we do an inplace_bitmask_and that alters the mr-allocated bitmask before returning it.
The use of mr in inplace_bitmask_and, which calls inplace_bitmask_binop, is a little misleading. The allocations there are always temporary and are not part of the return value. I think it may be possible to refactor the inplace functions to remove mr as an argument, and require the use of the default memory resource to avoid an implication that it is used for the returned data (it is not, since it acts in-place on bitmasks passed in that were from some other allocator).
There was a problem hiding this comment.
When the gather result contains null, its null_mask is allocated with mr. The inplace_bitmask_and uses this as the target null mask, in this situation, the segmented_null_mask doesn't need to use the supplied mr because this null mask is acting only as an operand in the inplace_nullmask_and, not as the result.
There was a problem hiding this comment.
Okay, that might be the case. The segmented_null_mask_reduction could use result->null_count() == 0 ? mr : rmm::mr::get_current_device_resource(). I'm not sure if I would recommend using a conditional mr for the sake of clarity, but it seems that it could be done.
|
@gpucibot merge |
This PR adds support to min/max segmented reduction to fixed point type. Together with #10447, this PR closes #10417 Besides, this PR refactors `segmented_reduce` to accept output iterators instead of allocating the result column from within. Authors: - Michael Wang (https://github.com/isVoid) - Bradley Dice (https://github.com/bdice) Approvers: - Bradley Dice (https://github.com/bdice) - Ram (Ramakrishna Prabhu) (https://github.com/rgsl888prabhu) - David Wendt (https://github.com/davidwendt) URL: #10794
This PR adds
min/maxsegmented reduction to string type.Part of #10417