Fixes up the overflowed fixed-point round on nullable column #10316
Conversation
Signed-off-by: sperlingxx <[email protected]>
sperlingxx
added
bug
Codecov Report
@@ Coverage Diff @@
## branch-22.04 #10316 +/- ##
================================================
- Coverage 10.67% 10.63% -0.05%
================================================
Files 122 122
Lines 20874 20953 +79
================================================
Hits 2228 2228
- Misses 18646 18725 +79
Continue to review full report at Codecov.
|
cpp/src/round/round.cu
Outdated
| stream); | ||
| } else { | ||
| detail::copy_range(thrust::make_constant_iterator(static_cast<Type>(0)), | ||
| thrust::make_constant_iterator(false), |
There was a problem hiding this comment.
Wait, if the input does not have null, the output will be a column of all nulls?
There was a problem hiding this comment.
Oh, you are right. It is unnecessary to set the null mask if the input doesn't have one.
There was a problem hiding this comment.
If so, then we can just use thrust::uninitialize_fill to fill the zero value for the output.
There was a problem hiding this comment.
Done. While, I am quite curious why we use unititialize_fill instead of thrust::flll here. Is it because thrust::fill will have extra cost on initializing the data before assigning the value? @ttnghia
There was a problem hiding this comment.
That's very interesting. I believe that for plain types they are the same. Otherwise, unititialize_fill calls copy constructor while thrust::fill call assignment operator.
There was a problem hiding this comment.
In addition, it would be nice if you can add a unit test for this case 😃
There was a problem hiding this comment.
That's interesting!
The unit test for validity was added into one of the existed test blocks.
|
@gpucibot merge |
Signed-off-by: sperlingxx <[email protected]> Closes #3793 Pushes cuDF-related decimal utilities down to cuDF. This PR is relied on cuDF changes: rapidsai/cudf#9809, rapidsai/cudf#9907 and rapidsai/cudf#10316.
Signed-off-by: sperlingxx [email protected]
Fixes up the overflow round on
nullablefixed-point columns. In previous implementation, we built the zero replacement column viacudf::detail::fill_in_placewith a zero scalar. However, this API overwrites the null mask of original column with the validity of the zero scalar, which is unexpected.