Use column_device_view instead of raw pointers in Decimal Division Expression Functors#11
Open
simoneves wants to merge 1 commit into
Conversation
…e divide functors and moved null handling into the kernel. * GPU (`DecimalExpressionKernelsGpu.cu`) - Functors now take `cudf::column_device_view` / `cudf::mutable_column_device_view` instead of raw pointers. - Device views are created on the host via `::create()` before launch (same pattern as `DecimalAggregationDevice.cu`). - In-kernel null rules: - **Column–column**: null if `lhs` or `rhs` is null, or `rhs == 0`. - **Column–scalar**: null if `lhs` is null, or scalar divisor is `0`. - **Scalar–column**: null if `rhs` is null, or `rhs == 0`. - Valid rows use `element<T>(idx)`; null rows use `out.set_null(idx)`. - Removed the `denom == 0` early return from `decimalDivideImpl` — callers skip zero divisors before calling it. * Host (`DecimalExpressionKernels.cpp`) - Removed host-side null-mask prep (`bitmask_and`, `copy_bitmask`) and `scatterNullsAtZeroDivisor`. - Output columns are created with `mask_state::ALL_VALID` so the kernel can set null bits. - Added `finalizeDivideOutputNullCount()` to compute `null_count` via `cudf::null_count()` after the kernel. * Header cleanup - Removed the public `scatterNullsAtZeroDivisor` API. - Updated docs to describe in-kernel null propagation. This drops the extra `binary_operation` + `copy_if_else` pass for zero divisors. Scalar divide-by-zero now also produces null (previously only column–column did, via the scatter step).
Selective Build Plan
Selective build plan |
column_device_view / mutable_column_device_view in th…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implemented
column_device_view/mutable_column_device_viewin the divide functors and moved null handling into the kernel.DecimalExpressionKernelsGpu.cu)cudf::column_device_view/cudf::mutable_column_device_viewinstead of raw pointers.::create()before launch (same pattern asDecimalAggregationDevice.cu).lhsorrhsis null, orrhs == 0.lhsis null, or scalar divisor is0.rhsis null, orrhs == 0.element<T>(idx); null rows useout.set_null(idx).denom == 0early return fromdecimalDivideImpl— callers skip zero divisors before calling it.DecimalExpressionKernels.cpp)bitmask_and,copy_bitmask) andscatterNullsAtZeroDivisor.mask_state::ALL_VALIDso the kernel can set null bits.finalizeDivideOutputNullCount()to computenull_countviacudf::null_count()after the kernel.scatterNullsAtZeroDivisorAPI.This drops the extra
binary_operation+copy_if_elsepass for zero divisors. Scalar divide-by-zero now also produces null (previously only column–column did, via the scatter step).Does this change ensure that garbage values (for null rows) will never reach decimalDivideImpl and therefore never cause markDecimalOverflow to be called incorrectly?
Yes — for rows that should produce null output, garbage payload values no longer reach
decimalDivideImpl, so they cannot spuriously setoverflowFlag.Each functor returns before calling
decimalDivideImplwhen:DivideFunctorlhs.is_null(idx),rhs.is_null(idx), orrhs == 0DivideRhsScalarFunctorlhs.is_null(idx)orrhsValue == 0DivideLhsScalarFunctorrhs.is_null(idx)orrhs == 0Because of short-circuit evaluation,
element()is not called on null inputs. For example inDivideFunctor:lhs→ return before readingrhs.elementrhs→ return before readingrhs.elementrhswith value0→rhs.elementis read intentionally (zero-divisor case)So garbage stored in null slots is never fed into
decimalDivideImpl.Compared to the old approach
Previously, the host pre-merged null masks and the kernel still ran on every row via raw pointers (
lhs[idx],rhs[idx]). Null rows could contain arbitrary payloads, and those values were passed intodecimalDivideImpl, which could callmarkDecimalOverflowon garbage. The new in-kernel null guards fix that.Caveats worth knowing
overflowFlag— that's intended.set_null(idx)is called without writingout.element<OutT>(idx).overflowFlagis global — one real overflow on any valid row fails the whole divide, but null rows no longer contribute false positives.Bottom line: null-input and zero-divisor rows are filtered before
decimalDivideImpl, so garbage in null slots cannot cause incorrect overflow reporting.