-
Notifications
You must be signed in to change notification settings - Fork 80
Replace cudf::detail::valid_if with cudf::bools_to_mask
#4301
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
Changes from 5 commits
513eb5c
c3f2550
046ee39
7099240
717c80b
505b3b6
6452aa8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,11 +19,11 @@ | |
|
|
||
| #include <cudf/column/column_factories.hpp> | ||
| #include <cudf/detail/utilities/cuda_memcpy.hpp> | ||
| #include <cudf/detail/valid_if.cuh> | ||
| #include <cudf/io/detail/json.hpp> | ||
| #include <cudf/io/detail/tokenize_json.hpp> | ||
| #include <cudf/strings/detail/strings_children.cuh> | ||
| #include <cudf/strings/strings_column_view.hpp> | ||
| #include <cudf/transform.hpp> | ||
| #include <cudf/utilities/memory_resource.hpp> | ||
| #include <cudf/utilities/span.hpp> | ||
|
|
||
|
|
@@ -771,10 +771,17 @@ std::pair<rmm::device_buffer, cudf::size_type> create_null_mask( | |
| }); | ||
| } | ||
|
|
||
| auto const valid_it = should_be_nullified->view().begin<bool>(); | ||
| auto [null_mask, null_count] = cudf::detail::valid_if( | ||
| valid_it, valid_it + should_be_nullified->size(), thrust::logical_not<bool>{}, stream, mr); | ||
| return {null_count > 0 ? std::move(null_mask) : rmm::device_buffer{0, stream, mr}, null_count}; | ||
| rmm::device_uvector<bool> valids(should_be_nullified->size(), stream); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case we are no longer be able to use
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If cudf continue to bar downstream libraries/applications from using its detail utilities, this will be the trend that we are unavoidable to follow, unfortunately. There are many more things that would be copied very soon.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If it's going to come to that, then I might punt this change out of 26.04. I'm neither keen nor proud of having to replicate CUDF kernels here. But our hand might be forced. CUDF has made it quite clear that Making a custom copy might be best for efficiency, albeit at the cost of maintenance. |
||
| auto const nullify_it = should_be_nullified->view().begin<bool>(); | ||
| thrust::transform(rmm::exec_policy_nosync(stream), | ||
| nullify_it, | ||
| nullify_it + should_be_nullified->size(), | ||
| valids.begin(), | ||
| thrust::logical_not<bool>{}); | ||
| auto [null_mask, null_count] = | ||
| cudf::bools_to_mask(cudf::device_span<bool const>(valids), stream, mr); | ||
| return {null_count > 0 ? std::move(*null_mask.release()) : rmm::device_buffer{0, stream, mr}, | ||
| null_count}; | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
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.
Can we directly write to something bit-packed instead, to avoid the extra work from calling bools_to_mask?
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.
This is exactly what the
valid_ifkernel is doing. You are suggesting to reimplementing thevalid_ifkernel 😄