Fix issue in slice() where columns with a positive offset were computing null counts incorrectly. #8738
Conversation
…ing a null count based on the wrong range of validity bits.
nvdbaranec
added
bug
cpp/src/copying/slice.cpp
Outdated
| std::transform(indices.begin(), | ||
| indices.end(), | ||
| std::back_inserter(shifted_indices), | ||
| [offset = input.offset()](size_type index) { return index + offset; }); |
There was a problem hiding this comment.
slice() is supposed to be a relatively free operation (ignoring the bit counting), so materializing the shifted indices is a bummer.
I think it would be fairly easy to make segmented_count_unset_bits take an iterator for the segment offsets so you could instead pass a transform iterator in that adds the offset.
There was a problem hiding this comment.
How about just invalidating the null count so the slice function will never do such counting? We don't know if the null count will be used later or not. If it is used later, it will be recomputed.
There was a problem hiding this comment.
We used to do it that way, but we found that it was a better idea to compute all the null counts at once. Otherwise if you have many slices where you need the null count, you end up launching many kernels. This came up in the context of contiguous_split.
There was a problem hiding this comment.
Note: slice.cpp will have to become slice.cu to do this.
There was a problem hiding this comment.
Done. Couple things of note: A number of functions/kernels got moved from null_mask.cu to null_mask.cuh. Both segmented_count_unset_bits and segmented_count_set_bits have been refactored to take iterators, since unset is implemented through set. I had to change a device lambda in segmented_count_unset_bits into a functor (word_num_set_bits_functor) due to extended device lambda restrictions causing the compiler to complain about the iterators now being passed in.
Codecov Report
@@ Coverage Diff @@
## branch-21.08 #8738 +/- ##
===============================================
Coverage ? 10.67%
===============================================
Files ? 109
Lines ? 18670
Branches ? 0
===============================================
Hits ? 1993
Misses ? 16677
Partials ? 0 Continue to review full report at Codecov.
|
…egmented_count_set_bits() to take iterators for range indices.
| @@ -141,6 +142,285 @@ void inplace_bitmask_binop( | |||
| stream.synchronize(); | |||
| } | |||
|
|
|||
| namespace { | |||
There was a problem hiding this comment.
Don't use anonymous namespaces in headers: https://wiki.sei.cmu.edu/confluence/display/cplusplus/DCL59-CPP.+Do+not+define+an+unnamed+namespace+in+a+header+file
There was a problem hiding this comment.
@jrhemstad @nvdbaranec JFYI this PR looks like it got merged without addressing this. It's not a big deal but may be worth fixing in a follow-up to avoid the possibility of unexpected UB somewhere down the line.
|
@gpucibot merge |
slice()was passing the raw split indices tosegmented_count_unset_bits(), resulting in the wrong ranges of validity bits being counted.