-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix empty mask behavior of apply_deletion_mask
#23857
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 all commits
2d05c16
d8f7458
fcb7cf7
ee39148
eb5a39e
18cb4d0
6981b52
03f3157
8276804
137f908
ea5be0b
9bf410a
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 |
|---|---|---|
|
|
@@ -67,15 +67,18 @@ std::unique_ptr<table> apply_mask(table_view const& input, | |
| cuda::stream_ref stream, | ||
| rmm::device_async_resource_ref mr) | ||
| { | ||
| if (boolean_mask.is_empty()) { return empty_like(input); } | ||
| auto const is_retention = (mask_kind == mask_type::RETENTION); | ||
|
|
||
| if (boolean_mask.is_empty()) { | ||
| return is_retention ? empty_like(input) : std::make_unique<table>(input, stream, mr); | ||
|
Contributor
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. [Optional] This is the right behavior if you want to special-case the empty mask for deletion, but I'm also wondering if there's a need to special-case it in the first place. Since we're changing behavior here anyway, and there's no backward compatibility with
Contributor
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. I think this new behavior makes sense. Empty mask should just mean nothing retained or deleted or we should just strongly enforce
Contributor
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.
I think
Contributor
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. I was only talking about the deletion branch. Retention would have to keep Since this was optional, we can just leave it at that, but just note that the lists and the table versions still disagree on how they treat an empty mask. |
||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| CUDF_EXPECTS(boolean_mask.type().id() == type_id::BOOL8, "Mask must be Boolean type"); | ||
| CUDF_EXPECTS(input.num_rows() == 0 || input.num_rows() == boolean_mask.size(), | ||
| "Column size mismatch"); | ||
|
|
||
| auto device_boolean_mask = cudf::column_device_view::create(boolean_mask, stream); | ||
|
|
||
| auto const is_retention = (mask_kind == mask_type::RETENTION); | ||
| if (boolean_mask.has_nulls()) { | ||
| if (is_retention) { | ||
| return detail::copy_if(input, retention_mask_filter<true>{*device_boolean_mask}, stream, mr); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.