Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support Segmented Min/Max Reduction on String Type #10447
Support Segmented Min/Max Reduction on String Type #10447
Changes from 16 commits
068542a
66bdb75
9bdcc65
3ddad32
889c253
e78ee18
5dcd6a1
eef7f21
a8dc8ea
ed5eaf9
24c8f91
fe5da2d
ab801f1
5e7ca89
680e0fe
19e5df8
d826cf9
2d3bd6b
d4ad909
10af494
4a3a541
6910c16
1d0b716
424f540
8f2a687
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
A super minor nit: we can determine if
segmented_null_mask
is a temporary memory based onresult->null_count()
and choose to use default resource v.s. supplied resource here.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.
Feel free to ignore or punt to next one @bdice.
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.
We are already using
mr
as efficiently (minimally) as possible. Ifresult->null_count() == 0
, we return themr
-allocated bitmask. Else, we do aninplace_bitmask_and
that alters themr
-allocated bitmask before returning it.The use of
mr
ininplace_bitmask_and
, which callsinplace_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 theinplace
functions to removemr
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the gather result contains null, its
null_mask
is allocated withmr
. Theinplace_bitmask_and
uses this as the target null mask, in this situation, thesegmented_null_mask
doesn't need to use the suppliedmr
because this null mask is acting only as an operand in theinplace_nullmask_and
, not as the result.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.
Okay, that might be the case. The
segmented_null_mask_reduction
could useresult->null_count() == 0 ? mr : rmm::mr::get_current_device_resource()
. I'm not sure if I would recommend using a conditionalmr
for the sake of clarity, but it seems that it could be done.