[REVIEW] Remove null_count() and has_nulls() from column_device_view. #4799
[REVIEW] Remove null_count() and has_nulls() from column_device_view. #4799trevorsm7 merged 8 commits intorapidsai:branch-0.14from
Conversation
… several places that were using it to use the source column_view instead.
|
Can one of the admins verify this patch? |
Codecov Report
@@ Coverage Diff @@
## branch-0.14 #4799 +/- ##
===============================================
+ Coverage 88.51% 88.58% +0.06%
===============================================
Files 51 50 -1
Lines 9869 9797 -72
===============================================
- Hits 8736 8679 -57
+ Misses 1133 1118 -15
Continue to review full report at Codecov.
|
jrhemstad
left a comment
There was a problem hiding this comment.
I think it's worth documenting somewhere on the column_device_view why it doesn't have a null_count member when both column and column_view do. Say something about how this information isn't needed in device code (where column_device_view is intended to be used), and is otherwise available from the column_view.
…unt() or has_nulls(). Updated copyright dates on a number of files.
harrism
left a comment
There was a problem hiding this comment.
Looks good. Just need to change some c-style casts to c++-style.
…sts to static_cast.
cpp/src/column/column_device_view.cu
Outdated
| : detail::column_device_view_base{source.type(), source.size(), | ||
| source.head(), source.null_mask(), | ||
| source.null_count(), source.offset()}, | ||
| source.offset()}, |
There was a problem hiding this comment.
@nvdbaranec I think you incorrectly resolved conflicts in this file when you merged branch-0.14. It seems @davidwendt's PR #4783 moved the contents of this constructor into a helper function and moved this constructor later in the file. I just tried to resolve conflicts on this PR (4799) and found it very confusing because of those changes. Please go back and fix up the merge.
There was a problem hiding this comment.
This wasn't a bad local merge. That PR (4783) just got commited at an unfortunate time (after the last update to this got pushed). Updated.
Adds the `null_count()` member function back to the `cudf::column_device_view` class. This member function was removed in #4799 because the null count was unreliable since it could be set to `UNKNOWN_NULL_COUNT`. The `UNKNOWN_NULL_COUNT` was removed in #13372 so this should no longer be an issue. Although the value is now more reliable it is just a copy at the time the `cudf::column_device_view` was created so it is not without caveats though these are easily explained and hopefully understood. The value is much less reliable on `cudf::mutable_column_device_view` where the null-mask is more likely modified rendering the count invalid. Therefore, the `null_count()` is added only to the immutable `cudf::column_device_view`. The growing number of changes to move template parameters like `has_nulls` to runtime parameters reduces unneeded kernel compilations. Here is one example where having this member function would've helped to reduce the function parameters and more clearly identify the `has_nulls` parameter directly correlates to the `input` parameter state: https://github.com/rapidsai/cudf/blob/609b08e2c8075cad12347cede69d195fa5b186f1/cpp/src/rolling/detail/rolling_operators.cuh#L399-L416 Note the `has_nulls` parameter would not be needed since the `input` could contain this information and more clearly show what the parameter is based on. Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Tianyu Liu (https://github.com/kingcrimsontianyu) - Nghia Truong (https://github.com/ttnghia) - Muhammad Haseeb (https://github.com/mhaseeb123) URL: #21430
Closes #4368
The main noteworthy thing in this PR is the removal of an exception in the constructor of
value_accessor. This does add some mild danger, but after internal discussions, it was decided to be reasonable.