Repository navigation
Opt-in overflow tracking for decimal fixed-point arithmetic #22356
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 14 commits
38de09b
cb1e7c9
927fe61
6a33a77
281dce6
063f1d7
f874b5e
d738fe8
736169f
e378183
0e0d29c
ad57bd8
2cb1ace
69095e1
eee3690
73b68f8
72b132a
a585cfa
670af6c
98dda38
9e5d015
c237180
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 |
|---|---|---|
|
|
@@ -10,7 +10,9 @@ | |
| #include <cudf/utilities/export.hpp> | ||
| #include <cudf/utilities/memory_resource.hpp> | ||
|
|
||
| #include <cstdint> | ||
| #include <memory> | ||
| #include <utility> | ||
|
|
||
| namespace CUDF_EXPORT cudf { | ||
|
|
||
|
|
@@ -221,6 +223,46 @@ std::unique_ptr<column> binary_operation( | |
| rmm::cuda_stream_view stream = cudf::get_default_stream(), | ||
| rmm::device_async_resource_ref mr = cudf::get_current_device_resource_ref()); | ||
|
|
||
| /** | ||
| * @brief Decimal fixed-point binary operation with a per-row overflow column. | ||
| * | ||
| * Like `binary_operation` for two base-10 decimal operands, but additionally returns a | ||
| * `BOOL8` column of the same length as the result. Element `i` of that column is `true` | ||
| * iff row `i` is an **active** (non-null on both sides) row whose arithmetic or rescale | ||
| * to @p output_type overflowed. Rows that are null on either side, or rows whose | ||
| * arithmetic completed without overflow, hold `false`. The overflow column has no null | ||
| * mask. | ||
| * | ||
| * Supported operators: ADD, SUB, MUL, DIV, MOD, PMOD, PYMOD. | ||
| * | ||
| * @return A pair `{result, overflow}` where `result` is the arithmetic result column | ||
| * (same semantics as `binary_operation`) and `overflow` is the per-row BOOL8 | ||
| * overflow column described above. | ||
| */ | ||
| std::pair<std::unique_ptr<column>, std::unique_ptr<column>> binary_operation_safe( | ||
|
Member
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. Another minor difference I wanted to mention: in the It would be good to align with the existing pattern, or at least converge on a consistent one. That said, it’s not immediately obvious how to adjust the Changing the output here from two columns to a single struct column seems relatively straightforward, but I’m not sure how important this consistency is in practice, or whether it’s worth enforcing it across the board. |
||
| scalar const& lhs, | ||
| column_view const& rhs, | ||
| binary_operator op, | ||
| data_type output_type, | ||
| rmm::cuda_stream_view stream = cudf::get_default_stream(), | ||
| rmm::device_async_resource_ref mr = cudf::get_current_device_resource_ref()); | ||
|
|
||
| std::pair<std::unique_ptr<column>, std::unique_ptr<column>> binary_operation_safe( | ||
|
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. can you make this behaviour part of binary_operation instead?
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. What behavior are you referring to?
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. the error checking. rather than having
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 disagree with this suggestion. |
||
| column_view const& lhs, | ||
| scalar const& rhs, | ||
| binary_operator op, | ||
| data_type output_type, | ||
| rmm::cuda_stream_view stream = cudf::get_default_stream(), | ||
| rmm::device_async_resource_ref mr = cudf::get_current_device_resource_ref()); | ||
|
|
||
| std::pair<std::unique_ptr<column>, std::unique_ptr<column>> binary_operation_safe( | ||
| column_view const& lhs, | ||
| column_view const& rhs, | ||
| binary_operator op, | ||
| data_type output_type, | ||
| rmm::cuda_stream_view stream = cudf::get_default_stream(), | ||
| rmm::device_async_resource_ref mr = cudf::get_current_device_resource_ref()); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * @brief Performs a binary operation between two columns using a | ||
| * user-defined PTX function. | ||
|
|
||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,273 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <cudf/fixed_point/detail/floating_conversion.hpp> | ||
| #include <cudf/fixed_point/fixed_point.hpp> | ||
| #include <cudf/utilities/export.hpp> | ||
| #include <cudf/utilities/traits.hpp> | ||
|
|
||
| #include <cuda/std/type_traits> | ||
| #include <cuda/std/utility> | ||
|
|
||
| /** | ||
| * @file safe_arithmetic.hpp | ||
| * @brief Overflow-aware free functions for `numeric::fixed_point`. | ||
| * | ||
| * These free functions intentionally live outside of `fixed_point` itself | ||
| * (mirroring how `floating_conversion.hpp` lives outside `fixed_point` for the | ||
| * float<->decimal conversion path). They take regular, non-tracking | ||
| * `fixed_point` operands and return a `safe_result` that pairs the computed | ||
| * value with an `overflow` flag describing whether the operation -- including | ||
| * any internal rescale -- would have wrapped the underlying integer storage. | ||
| * | ||
| * The intent is that overflow-aware code paths (the `binary_operation_safe` | ||
| * kernel today; specialized reduce/groupby aggregations in the future) compose | ||
| * these primitives explicitly, instead of relying on a sticky bit baked into | ||
| * the value type. | ||
| */ | ||
|
|
||
| namespace CUDF_EXPORT numeric { | ||
| namespace detail { | ||
|
|
||
| /** | ||
| * @brief Result of an overflow-checked `fixed_point` operation. | ||
| * | ||
| * @tparam Rep Storage type of the wrapped `fixed_point` value | ||
| * @tparam Rad Radix of the wrapped `fixed_point` value | ||
| */ | ||
| template <typename Rep, Radix Rad> | ||
| struct safe_result { | ||
| fixed_point<Rep, Rad> value; ///< Computed `fixed_point` value | ||
| bool overflow; ///< Whether the producing operation overflowed | ||
| }; | ||
|
Comment on lines
+48
to
+52
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. can you replace this and all occurrences with |
||
|
|
||
| /** | ||
| * @brief Whether `shift<Rep, Rad>(val, scale)` would incur signed-integer overflow | ||
| * | ||
| * Mirrors the overflow conditions of `multiplication_overflow` / | ||
| * `division_overflow` on the intermediate scale factor. | ||
| * | ||
| * @tparam Rep Representation type | ||
| * @tparam Rad Radix | ||
| * @tparam T Type of the value being shifted (typically `Rep`) | ||
| * @param val The value being shifted | ||
| * @param scale The amount to shift the value by | ||
| * @return true if the shift would overflow `Rep`, false otherwise | ||
| */ | ||
| template <typename Rep, Radix Rad, typename T> | ||
| CUDF_HOST_DEVICE inline constexpr bool shift_overflows(T const& val, scale_type const& scale) | ||
| { | ||
| auto const v = static_cast<Rep>(val); | ||
| if (scale == 0) { return false; } | ||
| if (scale > 0) { | ||
| Rep const divisor = ipow<Rep, Rad>(static_cast<int32_t>(scale)); | ||
| return division_overflow<Rep>(v, divisor); | ||
| } | ||
| Rep const multiplier = ipow<Rep, Rad>(static_cast<int32_t>(-scale)); | ||
| return multiplication_overflow<Rep>(v, multiplier); | ||
| } | ||
|
|
||
| /** | ||
| * @brief Rescale a `fixed_point` value, reporting whether the underlying shift overflows | ||
| * | ||
| * Equivalent to `x.rescaled(new_scale)` but surfaces the shift overflow flag | ||
| * instead of dropping it. | ||
| * | ||
| * @tparam Rep Storage type | ||
| * @tparam Rad Radix | ||
| * @param x The value to rescale | ||
| * @param new_scale The target scale | ||
| * @return `{rescaled_value, overflow}` | ||
| */ | ||
| template <typename Rep, Radix Rad> | ||
| CUDF_HOST_DEVICE inline safe_result<Rep, Rad> safe_rescaled(fixed_point<Rep, Rad> x, | ||
| scale_type new_scale) | ||
| { | ||
| if (new_scale == x.scale()) { return safe_result<Rep, Rad>{x, false}; } | ||
| auto const scale_delta = scale_type{new_scale - x.scale()}; | ||
| bool const overflow = shift_overflows<Rep, Rad>(x.value(), scale_delta); | ||
| Rep const value = shift<Rep, Rad>(x.value(), scale_delta); | ||
| return safe_result<Rep, Rad>{fixed_point<Rep, Rad>{scaled_integer<Rep>{value, new_scale}}, | ||
| overflow}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked addition of two `fixed_point` values | ||
| * | ||
| * Rescales both operands to the smaller of their two scales (matching | ||
| * `operator+`), then performs the add. The returned `overflow` flag is the | ||
| * disjunction of any rescale overflow and the integer add overflow. | ||
| */ | ||
| template <typename Rep, Radix Rad> | ||
| CUDF_HOST_DEVICE inline safe_result<Rep, Rad> safe_add(fixed_point<Rep, Rad> lhs, | ||
|
PointKernel marked this conversation as resolved.
|
||
| fixed_point<Rep, Rad> rhs) | ||
| { | ||
| auto const common_scale = cuda::std::min(lhs.scale(), rhs.scale()); | ||
| auto const lhs_r = safe_rescaled(lhs, common_scale); | ||
| auto const rhs_r = safe_rescaled(rhs, common_scale); | ||
| Rep const lv = lhs_r.value.value(); | ||
| Rep const rv = rhs_r.value.value(); | ||
| bool const op_overflow = addition_overflow<Rep>(lv, rv); | ||
| Rep const sum = lv + rv; | ||
| return safe_result<Rep, Rad>{ | ||
| fixed_point<Rep, Rad>{scaled_integer<Rep>{sum, common_scale}}, | ||
| op_overflow || lhs_r.overflow || rhs_r.overflow}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked subtraction of two `fixed_point` values | ||
| */ | ||
| template <typename Rep, Radix Rad> | ||
| CUDF_HOST_DEVICE inline safe_result<Rep, Rad> safe_sub(fixed_point<Rep, Rad> lhs, | ||
| fixed_point<Rep, Rad> rhs) | ||
| { | ||
| auto const common_scale = cuda::std::min(lhs.scale(), rhs.scale()); | ||
| auto const lhs_r = safe_rescaled(lhs, common_scale); | ||
| auto const rhs_r = safe_rescaled(rhs, common_scale); | ||
| Rep const lv = lhs_r.value.value(); | ||
| Rep const rv = rhs_r.value.value(); | ||
| bool const op_overflow = subtraction_overflow<Rep>(lv, rv); | ||
| Rep const diff = lv - rv; | ||
| return safe_result<Rep, Rad>{ | ||
| fixed_point<Rep, Rad>{scaled_integer<Rep>{diff, common_scale}}, | ||
| op_overflow || lhs_r.overflow || rhs_r.overflow}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked multiplication of two `fixed_point` values | ||
| * | ||
| * No rescale is needed -- the result scale is `lhs.scale() + rhs.scale()`. | ||
| */ | ||
| template <typename Rep, Radix Rad> | ||
| CUDF_HOST_DEVICE inline safe_result<Rep, Rad> safe_mul(fixed_point<Rep, Rad> lhs, | ||
| fixed_point<Rep, Rad> rhs) | ||
| { | ||
| Rep const lv = lhs.value(); | ||
| Rep const rv = rhs.value(); | ||
| bool const op_overflow = multiplication_overflow<Rep>(lv, rv); | ||
| Rep const prod = lv * rv; | ||
| scale_type const out_scale{lhs.scale() + rhs.scale()}; | ||
| return safe_result<Rep, Rad>{fixed_point<Rep, Rad>{scaled_integer<Rep>{prod, out_scale}}, | ||
| op_overflow}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked division of two `fixed_point` values | ||
| * | ||
| * Only `INT_MIN / -1` is a signed-integer division overflow; that's exactly what | ||
| * `division_overflow` catches. | ||
| */ | ||
| template <typename Rep, Radix Rad> | ||
| CUDF_HOST_DEVICE inline safe_result<Rep, Rad> safe_div(fixed_point<Rep, Rad> lhs, | ||
| fixed_point<Rep, Rad> rhs) | ||
| { | ||
| Rep const lv = lhs.value(); | ||
| Rep const rv = rhs.value(); | ||
| bool const op_overflow = division_overflow<Rep>(lv, rv); | ||
| Rep const quot = lv / rv; | ||
| scale_type const out_scale{lhs.scale() - rhs.scale()}; | ||
| return safe_result<Rep, Rad>{fixed_point<Rep, Rad>{scaled_integer<Rep>{quot, out_scale}}, | ||
| op_overflow}; | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked modulo of two `fixed_point` values | ||
| * | ||
| * Integer `%` itself cannot overflow when both operands are representable, so | ||
| * the only overflow source is the rescale to the common scale. | ||
| */ | ||
| template <typename Rep, Radix Rad> | ||
| CUDF_HOST_DEVICE inline safe_result<Rep, Rad> safe_mod(fixed_point<Rep, Rad> lhs, | ||
| fixed_point<Rep, Rad> rhs) | ||
| { | ||
| auto const common_scale = cuda::std::min(lhs.scale(), rhs.scale()); | ||
| auto const lhs_r = safe_rescaled(lhs, common_scale); | ||
| auto const rhs_r = safe_rescaled(rhs, common_scale); | ||
| Rep const remainder = lhs_r.value.value() % rhs_r.value.value(); | ||
| return safe_result<Rep, Rad>{ | ||
| fixed_point<Rep, Rad>{scaled_integer<Rep>{remainder, common_scale}}, | ||
| lhs_r.overflow || rhs_r.overflow}; | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked positive modulo, matching `ops::PMod` semantics for decimals. | ||
| * | ||
| * Implements `rem = x % y; (rem < 0) ? (rem + y) % y : rem`. The intermediate | ||
| * `rem + y` can overflow even though `%` itself cannot, so this surfaces the | ||
| * add overflow in addition to the rescale overflows reported by `safe_mod`. | ||
| */ | ||
| template <typename Rep, Radix Rad> | ||
| CUDF_HOST_DEVICE inline safe_result<Rep, Rad> safe_pmod(fixed_point<Rep, Rad> lhs, | ||
| fixed_point<Rep, Rad> rhs) | ||
| { | ||
| auto const m = safe_mod(lhs, rhs); | ||
| if (!(m.value.value() < Rep{0})) { return m; } | ||
|
|
||
| auto const rhs_r = safe_rescaled(rhs, m.value.scale()); | ||
| Rep const mv = m.value.value(); | ||
| Rep const rv = rhs_r.value.value(); | ||
| bool const add_overflow = addition_overflow<Rep>(mv, rv); | ||
| Rep const corrected = (mv + rv) % rv; | ||
| return safe_result<Rep, Rad>{ | ||
| fixed_point<Rep, Rad>{scaled_integer<Rep>{corrected, m.value.scale()}}, | ||
| m.overflow || rhs_r.overflow || add_overflow}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked Python-style modulo: `((x % y) + y) % y`. | ||
| * | ||
| * The intermediate add can overflow; the final `%` cannot. | ||
| */ | ||
| template <typename Rep, Radix Rad> | ||
| CUDF_HOST_DEVICE inline safe_result<Rep, Rad> safe_pymod(fixed_point<Rep, Rad> lhs, | ||
| fixed_point<Rep, Rad> rhs) | ||
| { | ||
| auto const m = safe_mod(lhs, rhs); | ||
|
|
||
| auto const rhs_r = safe_rescaled(rhs, m.value.scale()); | ||
| Rep const mv = m.value.value(); | ||
| Rep const rv = rhs_r.value.value(); | ||
| bool const add_overflow = addition_overflow<Rep>(mv, rv); | ||
| Rep const corrected = (mv + rv) % rv; | ||
| return safe_result<Rep, Rad>{ | ||
| fixed_point<Rep, Rad>{scaled_integer<Rep>{corrected, m.value.scale()}}, | ||
| m.overflow || rhs_r.overflow || add_overflow}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked floating-point -> `fixed_point` conversion | ||
| * | ||
| * Uses `convert_floating_to_integral<Rep, true>` (in | ||
| * `floating_conversion.hpp`) for base-10 decimals, which saturates and reports | ||
| * overflow. For base-2 radixes there is no checked path today, so `overflow` | ||
| * is always `false`. | ||
| * | ||
| * @tparam Fixed Target `fixed_point` instantiation | ||
| * @tparam Floating Source floating-point type | ||
| * @param floating The floating-point value to convert | ||
| * @param scale The desired scale of the result | ||
| * @return `{fixed_point_value, overflow}` | ||
| */ | ||
| template <typename Fixed, | ||
| typename Floating, | ||
| CUDF_ENABLE_IF(cuda::std::is_floating_point_v<Floating> && | ||
| cudf::is_fixed_point<Fixed>())> | ||
| CUDF_HOST_DEVICE inline safe_result<typename Fixed::rep, Fixed::rad> | ||
| safe_convert_floating_to_fixed(Floating floating, scale_type scale) | ||
| { | ||
| using Rep = typename Fixed::rep; | ||
| if constexpr (Fixed::rad == Radix::BASE_10) { | ||
| auto const [value, overflow] = convert_floating_to_integral<Rep, true>(floating, scale); | ||
| return safe_result<Rep, Fixed::rad>{Fixed{scaled_integer<Rep>{value, scale}}, overflow}; | ||
| } else { | ||
| Rep const value = static_cast<Rep>(shift<Rep, Fixed::rad>(floating, scale)); | ||
| return safe_result<Rep, Fixed::rad>{Fixed{scaled_integer<Rep>{value, scale}}, false}; | ||
| } | ||
| } | ||
|
|
||
| } // namespace detail | ||
| } // namespace CUDF_EXPORT numeric | ||
|
Comment on lines
+43
to
+299
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. can you please confirm each of these operators hasn't been implemented in
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. please also merge these into
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.
However, it doesn't detect rescale-induced overflow in The main reason for this implementation is that, by default, Velox GPU/Velox-cuDF silently wraps the result on decimal arithmetic overflow instead of throwing an error.
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'm not convinced scale-induced overflow/underflow needs to be checked in the device code, whilst it is useful to have.
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. It might be worth doing benchmark before trying this. A single kernel to do both seems like it would be faster than 2 separate kernels but a benchmark would certainly show the impact.
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. The scale is the same for all the rows of a column. So it is trivial to compute and check before launching the binary operation (2 scalars). AST has a similar setup. It's what Spark RAPIDS does.
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. Thanks @lamarrr. Two separate things here: Scale/type validity -> agreed, I'll add a pre-kernel check. Scales are column metadata (same for all rows), so I can add a host-side CUDF_EXPECTS guard before launch that rejects invalid scale/output-type combos and fails fast without launching the kernel. Rescale overflow -> has to stay per-row. Whether a rescale overflows depends on each row's value, not just the scale: Same scale metadata, different outcome-> so a scalar pre-check can't produce the per-row BOOL8 this API exposes . So the metadata check is additive. A cheap early guard but doesn't remove the kernel's per-row checks.
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. can you instead implement overflow checks into "checked_arithmetic.cuh" header, using #22836 as base? |
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared toansi, I do like thesafenaming though it's not perfect.Actually,
safeis probably not a great choice either as we already have existing uses ofunsafeto denote an operation is not thread safe, e.g. https://github.com/rapidsai/cudf/blob/4162d61633c332be91851126df388fb6489aac9c/cpp/include/cudf/utilities/bit.hpp#L73AFAIK, we currently have three different naming schemes in libcudf related to overflow checking:
_WITH_OVERFLOWsuffix to denote it has overflow check https://github.com/rapidsai/cudf/blob/aa3cdee199d636d0075b6ae165d8ed09aff0b92a/cpp/include/cudf/aggregation.hpp#L81ansi, e.g. https://github.com/rapidsai/cudf/blob/6f8c429d0a532bdfd5d5e4057643addec6f5f486/cpp/include/cudf/detail/operators/ansi_arithmetic.cuh#L32safeprefix/suffixWe should really converge on a consistent naming scheme going forward to avoid further fragmentation and branching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we've agreed on using an "_overflow" suffix