Opt-in overflow tracking for decimal fixed-point arithmetic - #22356
Opt-in overflow tracking for decimal fixed-point arithmetic#22356Avinash-Raj wants to merge 22 commits into
Conversation
github-actions
Bot
added
libcudf
Avinash-Raj
changed the title
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Avinash-Raj
changed the title
|
I would favor adding new operators/functions rather than a compile-time flag — we want to be able to ship one build of cuDF that works universally. |
|
@bdice I'd imagine this is not limited to scalar functions like binaryops and such. We'd also need to detect overflow in aggregations like groupby and reduce. And those would require overloading the existing +,* operators. What's your opinion on adding a new type maybe called safe_decimal. Spark should also have a concept of arithmetic exception for overflows. How do they support this with libcudf decimal? |
Inline with this thinking, we could extend cudf's and use |
|
This is related to ANSI support that I've recently filed: #21676 |
|
Ok, replaced the CMake |
|
You might want to put overflow tracking into the floating <--> decimal conversion code as well (include/cudf/fixed_point/detail/floating_conversion.hpp). E.g. there are the guarded_left_shift() and guarded_right_shift() functions, and there are potential overflows in convert_floating_to_integral_shifting(), shift_to_decimal_pospow(), and shift_to_decimal_negpow(). All potential overflow sites are mentioned explicitly in the code comments in these functions. |
Avinash-Raj
changed the title
|
Could this be done without modifying the fixed-point class? The operator overloads are not really required for this to work since the dispatcher logic creates specialized code paths automatically. And somehow the overflow-awareness is communicated through to the appropriate operation/function and would generally require a special code path either way. It seems the operators could be just free functions and not member functions or member operators. The return type would include an overflow flag along with the result. Similar to how the https://github.com/rapidsai/cudf/blob/main/cpp/include/cudf/fixed_point/detail/floating_conversion.hpp is a separate file handling specific operations for fixed-point. The new free functions would be called as part of the specialized overflow-aware aggregation types for reduce, groupby, etc. |
|
@davidwendt I have addressed your comment by adding free functions that return the operation result and has_overflow_occurred as a tuple, without making or reverting any changes to the fixed_point class. |
davidwendt
left a comment
davidwendt
left a comment
There was a problem hiding this comment.
Seems the gtests only execute on decimal64.
| * @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) |
There was a problem hiding this comment.
Can this function be moved to safe_arithmetic.hpp? Or does it need to be public?
Perhaps move it or create a new header file.
There was a problem hiding this comment.
Yes, we could move the shift_overflows function to safe_arithmetic.hpp since that is the only place where it is called.
There was a problem hiding this comment.
Done! the relevant function gets moved to safe_arithmetic.hpp and also added safe arithmetic tests to deal with decimal64 overflows.
|
Is this PR and #22261 related or overlapping? |
| return guarded_left_shift(static_cast<UnsignedRep>(shifting_rep), pow2); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
duplicating the functionality by adding new high level functions for all of this code is brutal; we shouldn't have to duplicate all of this code logic. It's too easy for one function to get updated but not the other, etc. I would instead add a template parameter bool for whether to check or not. Then that bool can be passed to e.g. checked_narrow_cast() (potentially renaming) and if it's constexpr-false we just do the straight up cast.
There was a problem hiding this comment.
In addition I feel the overflow bool should be returned and not passed in.
There was a problem hiding this comment.
Like you suggested, instead of duplicating the high-level functions, I used compile-time branching inside each function based on the CheckOverflow template parameter. For example,
safe_arithmetic.hpp
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};
}
}floating_conversion.hpp
/**
* @brief Perform floating-point -> integer decimal conversion
*
* @tparam Rep The type of integer we are converting to, to store the decimal value
* @tparam CheckOverflow Whether to return overflow detection with the converted value
* @tparam FloatingType The type of floating-point object we are converting from
* @param floating The floating point value to convert
* @param scale The desired base-10 scale factor: decimal value = returned value * 10^scale
* @return Integer representation of the floating-point value, or `{value, overflow}` when
* `CheckOverflow` is true
*/
template <typename Rep,
bool CheckOverflow = false,
typename FloatingType,
CUDF_ENABLE_IF(cuda::std::is_floating_point_v<FloatingType>)>
CUDF_HOST_DEVICE inline auto convert_floating_to_integral(FloatingType const& floating,
scale_type const& scale)
{
// Extract components of the floating point number
using converter = floating_converter<FloatingType>;
auto const integer_rep = converter::bit_cast_to_integer(floating);
if (converter::is_zero(integer_rep)) { return maybe_with_overflow<CheckOverflow>(Rep{0}, false); }
// Note that the significand here is an unsigned integer with sizeof(FloatingType)
auto const is_negative = converter::get_is_negative(integer_rep);
auto const [significand, floating_pow2] = converter::get_significand_and_pow2(integer_rep);
// Add half a bit if truncating to yield expected value, see function for discussion.
auto const pow10 = static_cast<int>(scale);
auto const [base2_value, pow2] =
add_half_if_truncates(floating, significand, floating_pow2, pow10);
// Apply the powers of 2 and 10 to convert to decimal.
auto const [magnitude_u, overflow] =
convert_floating_to_integral_shifting<Rep, FloatingType, CheckOverflow>(
base2_value, pow10, pow2);
if constexpr (!CheckOverflow) {
// Reapply the sign and return
// NOTE: Cast can overflow!
auto const signed_magnitude = static_cast<Rep>(magnitude_u);
return is_negative ? -signed_magnitude : signed_magnitude;
} else {
// Reapply sign with saturation on representational overflow.
using UnsignedRep = cuda::std::make_unsigned_t<Rep>;
auto const umax = static_cast<UnsignedRep>(cuda::std::numeric_limits<Rep>::max());
if (!is_negative) {
if (magnitude_u > umax) {
return cuda::std::pair<Rep, bool>{cuda::std::numeric_limits<Rep>::max(), true};
}
return cuda::std::pair<Rep, bool>{static_cast<Rep>(magnitude_u), overflow};
}
// Negative range has one extra representable value for two's complement.
// magnitude == max+1 maps to min.
auto const umin_mag = umax + UnsignedRep{1};
if (magnitude_u > umin_mag) {
return cuda::std::pair<Rep, bool>{cuda::std::numeric_limits<Rep>::min(), true};
}
if (magnitude_u == umin_mag) {
return cuda::std::pair<Rep, bool>{cuda::std::numeric_limits<Rep>::min(), overflow};
}
return cuda::std::pair<Rep, bool>{static_cast<Rep>(-static_cast<Rep>(magnitude_u)), overflow};
}
}I hope this is the right way, correct me if I'm wrong.
There was a problem hiding this comment.
My point is we should not have both shift_to_decimal_pospow_checked() and shift_to_decimal_pospow(). It should just be one function with template bool on whether to check or not. Same for all of the others. Duplicating all of this code will just cause maintenance nightmares.
There was a problem hiding this comment.
Agreed and it got fixed on floating_conversion.hpp.
There was a problem hiding this comment.
@davidwendt “In addition, I feel the overflow bool should be returned and not passed in.” If my understanding is correct, you don’t want to pass a sticky overflow flag, rmm::device_scalar<std::uint32_t>& overflow_flag, to the high-level safe functions (refer below). Instead, the corresponding overflow value (bool) should be returned as the second output parameter. Is that correct?
template <typename LhsType, typename RhsType>
std::unique_ptr<column> binary_operation_safe(LhsType const& lhs,
RhsType const& rhs,
binary_operator op,
data_type output_type,
rmm::device_scalar<std::uint32_t>& overflow_flag,
rmm::cuda_stream_view stream,
rmm::device_async_resource_ref mr)
{There was a problem hiding this comment.
Yes. Should this not return a bool per row?
I would've expected the result to be 2 columns. One for the fixed-point value and one of bool values.
There was a problem hiding this comment.
I have made changes as suggested in your comment: "Now the binary_operation_safe function should return two columns (result, overflow_per_row)."
…n w.r.t template param
davidwendt
added
feature request
|
@davidwendt Updated PR description. |
|
Great. You have some style issues to resolve: https://results.pre-commit.ci/run/github/90506918/1779899179.fLA-VsUFQTW7brv_xDSJaw |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds overflow-aware fixed-point arithmetic helpers and checked floating-to-fixed conversion primitives; declares and implements binary_operation_safe (returns result + per-row BOOL8 overflow), provides compiled/kernel implementations for DECIMAL32/64/128, and adds extensive unit tests and CMake updates. ChangesSafe decimal arithmetic and overflow detection
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
cpp/tests/fixed_point/safe_arithmetic_tests.cpp (1)
301-329: ⚡ Quick winMove test helper out of anonymous namespace to match test-style rule.
Please keep this helper/static_assert block in the global namespace to align with the project’s test namespace rule.
As per coding guidelines, "
cpp/**/*test*.{cu,cpp}: Test code must be in the global namespace, not in custom namespaces."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/fixed_point/safe_arithmetic_tests.cpp` around lines 301 - 329, The helper function safe_ops_preserve_rep and its three static_asserts must be moved out of the anonymous namespace into the global namespace; locate the block declaring template<typename Fixed> constexpr bool safe_ops_preserve_rep() (which references Rep, Rad, numeric::detail::safe_result and uses decltype(detail::safe_add/sub/mul/div)) and the following static_assert(safe_ops_preserve_rep<decimal32/64/128>()) and remove the surrounding anonymous namespace so the template and asserts live at global scope to comply with the test namespace rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/cudf/binaryop.hpp`:
- Around line 226-264: Add full Doxygen for each binary_operation_safe overload
(the scalar-column, column-scalar, and column-column signatures) mirroring the
top-level brief: include `@brief`, an explicit `@param` for lhs, rhs, op,
output_type, stream, and mr (document types: scalar vs column_view where
appropriate), an `@return` describing the pair {result, overflow} and the per-row
BOOL8 overflow semantics, and an `@throw` describing exceptions (e.g., when
operand storage types differ or when overflow/errors occur). Also explicitly
document the implementation constraint that both operands must have the same
underlying storage type (same-storage-type requirement) and note that the
overflow column has no null mask. Ensure the tags and wording are consistent
across all three overload declarations.
In `@cpp/include/cudf/fixed_point/detail/floating_conversion.hpp`:
- Around line 533-535: The unchecked branch of checked_left_shift forwards a
possibly-negative bit_shift to guarded_left_shift, allowing undefined behavior;
fix by adding a guard in the !CheckOverflow branch: if bit_shift < 0 return an
overflow result instead of calling guarded_left_shift (e.g., return {value,
true}), otherwise call guarded_left_shift(value, bit_shift); reference symbols:
checked_left_shift, CheckOverflow, bit_shift, guarded_left_shift.
In `@cpp/include/cudf/fixed_point/detail/safe_arithmetic.hpp`:
- Around line 187-193: safe_mod currently computes `Rep const remainder =
lhs_r.value.value() % rhs_r.value.value();` without guarding a zero divisor;
modify `safe_mod` (and the analogous `safe_pmod`/`safe_pymod` code paths) to
first check whether `rhs_r.value.value()` is zero and short-circuit to a safe
return instead of performing `%`; specifically, if the rescaled RHS is zero,
construct and return a `safe_result<Rep, Rad>` containing a `fixed_point<Rep,
Rad>{ scaled_integer<Rep>{ /* zero remainder */, common_scale } }` (or other
agreed sentinel) and set the overflow flag to `lhs_r.overflow || rhs_r.overflow
|| true` (i.e., mark it as an invalid/overflow case), otherwise proceed to
compute `remainder` with `%` as before; apply the same zero-check and
short-circuit logic in the blocks that reference `lhs_r`, `rhs_r`,
`safe_rescaled`, `safe_result`, `fixed_point`, and `scaled_integer` at the other
locations mentioned (lines around the `safe_pmod`/`safe_pymod` implementations).
- Around line 168-174: The division implementation in safe_div computes quot =
lv / rv even when rv == 0; move the integer division after guarding the zero
divisor: use division_overflow<Rep>(lv, rv) and/or an explicit check for rv == 0
to set op_overflow (or an appropriate error flag) and only perform lv / rv when
rv != 0, then construct fixed_point<Rep, Rad>{scaled_integer<Rep>{quot,
out_scale}} and return safe_result<Rep, Rad>{... , op_overflow}; ensure symbols
referenced are Rep, lv, rv, division_overflow<Rep>, quot, out_scale,
fixed_point, scaled_integer, and safe_result so the zero-divisor is
short-circuited before any division occurs.
In `@cpp/src/binaryop/compiled/binary_ops_safe.cu`:
- Around line 105-118: The code currently reads fixed-width decimal elements via
lhs.element<Rep>(li) and rhs.element<Rep>(ri) even when the row is inactive;
change the flow to short-circuit before any element loads: compute row_active =
lhs.is_valid(li) && rhs.is_valid(ri) first, then if row_active is true load
lhs.element<Rep>(li) and rhs.element<Rep>(ri), perform SafeOp{} and
numeric::detail::safe_rescaled on the result and write d_overflow_per_row[i] and
out.data<Rep>()[i]; if row_active is false avoid calling
lhs.element/rhs.element, set d_overflow_per_row[i] = false and set
out.data<Rep>()[i] to a defined neutral value (e.g., zero) to avoid reading null
payloads.
In `@cpp/tests/binaryop/binop-compiled-fixed_point-test.cpp`:
- Around line 894-940: Add additional unit tests for binary_operation_safe to
cover required edge cases: implement TESTs (e.g.,
BinaryOperationSafeDecimal.mulEmptyInput, .mulNullsKeepFalse, .mulSlicedColumns,
.mulMultiBlockSizes) that call cudf::binary_operation_safe with decimal inputs
and use cudf::binary_operation_fixed_point_output_type to build the output type;
verify for empty inputs that result size is zero and overflow column is empty,
for inputs with nulls that inactive/null rows produce overflow==false, for
sliced column inputs that slicing preserves correct results and overflow flags,
and for large/multi-block sized inputs that overflow flags are correctly set
per-row. Locate behaviour around binary_operation_safe and reuse the pattern
from existing tests (stream/mr creation, CUDF_TEST_EXPECT_COLUMNS_EQUAL /
EXPECT_EQ) when asserting results.
---
Nitpick comments:
In `@cpp/tests/fixed_point/safe_arithmetic_tests.cpp`:
- Around line 301-329: The helper function safe_ops_preserve_rep and its three
static_asserts must be moved out of the anonymous namespace into the global
namespace; locate the block declaring template<typename Fixed> constexpr bool
safe_ops_preserve_rep() (which references Rep, Rad, numeric::detail::safe_result
and uses decltype(detail::safe_add/sub/mul/div)) and the following
static_assert(safe_ops_preserve_rep<decimal32/64/128>()) and remove the
surrounding anonymous namespace so the template and asserts live at global scope
to comply with the test namespace rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 73057fd4-1323-41df-8a0e-dea63e4fcded
📒 Files selected for processing (13)
cpp/CMakeLists.txtcpp/include/cudf/binaryop.hppcpp/include/cudf/fixed_point/detail/floating_conversion.hppcpp/include/cudf/fixed_point/detail/safe_arithmetic.hppcpp/include/cudf/fixed_point/fixed_point.hppcpp/include/cudf/utilities/type_dispatcher.hppcpp/src/binaryop/binaryop.cppcpp/src/binaryop/compiled/binary_ops.cucpp/src/binaryop/compiled/binary_ops.hppcpp/src/binaryop/compiled/binary_ops_safe.cucpp/tests/CMakeLists.txtcpp/tests/binaryop/binop-compiled-fixed_point-test.cppcpp/tests/fixed_point/safe_arithmetic_tests.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/binaryop/compiled/binary_ops.cu (1)
386-417: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd Doxygen documentation for new public-facing functions.
The three new
binary_operation_safeoverloads lack Doxygen documentation. While there is a brief comment on lines 383-385, proper Doxygen-style documentation is required for C++/CUDA code. As per coding guidelines, C++/CUDA code must include proper Doxygen documentation.📝 Example Doxygen documentation structure
+/** + * `@brief` Performs safe binary operation between two columns with overflow detection. + * + * `@param` out Output mutable column view to store the result + * `@param` lhs Left-hand side column view + * `@param` rhs Right-hand side column view + * `@param` op Binary operator to apply + * `@param` d_overflow_per_row Device pointer to bool array for per-row overflow flags + * `@param` stream CUDA stream used for device memory operations and kernel launches + * `@param` mr Device memory resource used for temporary allocations + */ void binary_operation_safe(mutable_column_view& out, column_view const& lhs, column_view const& rhs, binary_operator op, bool* d_overflow_per_row, rmm::cuda_stream_view stream, rmm::device_async_resource_ref mr)Similar documentation should be added for the scalar/column and column/scalar overloads.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/binaryop/compiled/binary_ops.cu` around lines 386 - 417, Add Doxygen-style comments for the three public overloads of binary_operation_safe so they follow project guidelines: document purpose, parameters (out, lhs, rhs, op, d_overflow_per_row, stream, mr), return/behavior (how overflow is reported via d_overflow_per_row), and any special cases (scalar vs column overloads and ownership of temporary column_views from scalar_to_column_view). Place the Doxygen block immediately above each function signature (the column/column, scalar/column, and column/scalar overloads) and mirror the wording/format used by existing documented binary ops in the file for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpp/src/binaryop/compiled/binary_ops.cu`:
- Around line 386-417: Add Doxygen-style comments for the three public overloads
of binary_operation_safe so they follow project guidelines: document purpose,
parameters (out, lhs, rhs, op, d_overflow_per_row, stream, mr), return/behavior
(how overflow is reported via d_overflow_per_row), and any special cases (scalar
vs column overloads and ownership of temporary column_views from
scalar_to_column_view). Place the Doxygen block immediately above each function
signature (the column/column, scalar/column, and column/scalar overloads) and
mirror the wording/format used by existing documented binary ops in the file for
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 50a77704-f1cc-4824-a22e-3c63a2ee44a4
📒 Files selected for processing (8)
cpp/CMakeLists.txtcpp/include/cudf/fixed_point/detail/floating_conversion.hppcpp/include/cudf/fixed_point/fixed_point.hppcpp/include/cudf/utilities/type_dispatcher.hppcpp/src/binaryop/binaryop.cppcpp/src/binaryop/compiled/binary_ops.cucpp/src/binaryop/compiled/binary_ops.hppcpp/src/binaryop/compiled/binary_ops_safe.cu
✅ Files skipped from review due to trivial changes (2)
- cpp/include/cudf/utilities/type_dispatcher.hpp
- cpp/include/cudf/fixed_point/fixed_point.hpp
🚧 Files skipped from review as they are similar to previous changes (5)
- cpp/src/binaryop/compiled/binary_ops.hpp
- cpp/CMakeLists.txt
- cpp/src/binaryop/binaryop.cpp
- cpp/src/binaryop/compiled/binary_ops_safe.cu
- cpp/include/cudf/fixed_point/detail/floating_conversion.hpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tests/binaryop/binop-compiled-fixed_point-test.cpp (1)
945-1084: ⚡ Quick winEdge-case coverage now in place; consider adding a
decimal128overflow case.These new tests resolve the earlier edge-case gap (empty input, null rows kept
false, sliced columns, multi-block sizes, div-by-zero). Slice/sparse-overflow math all checks out.One coverage note: every
BinaryOperationSafeDecimaltest usesdecimal64, so the boundary value(int64::max()/2)+1only exercises theint64storage path. Thedecimal128(__int128) rescale/overflow path is untested. A single added case mirroringmulOverflowSetsGlobalFlagwith adecimal128near-__int128-max operand would cover it cheaply.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/binaryop/binop-compiled-fixed_point-test.cpp` around lines 945 - 1084, Add a decimal128 overflow test analogous to the existing decimal64 case: create a new TEST(BinaryOperationSafeDecimal, mulOverflowDecimal128) that uses Rep = cudf::device_storage_type_t<numeric::decimal128>, pick a "big" operand near the __int128 max (mirror how big is computed for decimal64), build lhs/rhs vectors to force multiplication overflow on selected rows (similar pattern to mulOverflowSetsGlobalFlag), call cudf::binary_operation_fixed_point_output_type and cudf::binary_operation_safe with cudf::binary_operator::MUL, and assert the result size and that the overflow column (from overflow->view()) matches the expected boolean mask marking overflows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/tests/binaryop/binop-compiled-fixed_point-test.cpp`:
- Around line 945-1084: Add a decimal128 overflow test analogous to the existing
decimal64 case: create a new TEST(BinaryOperationSafeDecimal,
mulOverflowDecimal128) that uses Rep =
cudf::device_storage_type_t<numeric::decimal128>, pick a "big" operand near the
__int128 max (mirror how big is computed for decimal64), build lhs/rhs vectors
to force multiplication overflow on selected rows (similar pattern to
mulOverflowSetsGlobalFlag), call cudf::binary_operation_fixed_point_output_type
and cudf::binary_operation_safe with cudf::binary_operator::MUL, and assert the
result size and that the overflow column (from overflow->view()) matches the
expected boolean mask marking overflows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ac07494e-0ef2-43e7-a932-2633d3b25af0
📒 Files selected for processing (5)
cpp/include/cudf/binaryop.hppcpp/include/cudf/fixed_point/detail/floating_conversion.hppcpp/include/cudf/fixed_point/detail/safe_arithmetic.hppcpp/src/binaryop/compiled/binary_ops_safe.cucpp/tests/binaryop/binop-compiled-fixed_point-test.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- cpp/include/cudf/binaryop.hpp
- cpp/src/binaryop/compiled/binary_ops_safe.cu
- cpp/include/cudf/fixed_point/detail/safe_arithmetic.hpp
- cpp/include/cudf/fixed_point/detail/floating_conversion.hpp
|
@pmattione-nvidia https://github.com/rapidsai/cudf/actions/runs/26745355782/job/78819432616?pr=22356 |
|
@davidwendt All the style issues have been resolved now. |
| * @throw cudf::data_type_error if the operation is not supported for the types of | ||
| * @p lhs and @p rhs | ||
| */ | ||
| std::pair<std::unique_ptr<column>, std::unique_ptr<column>> binary_operation_safe( |
There was a problem hiding this comment.
Compared to ansi, I do like the safe naming though it's not perfect.
Actually, safe is probably not a great choice either as we already have existing uses of unsafe to denote an operation is not thread safe, e.g. https://github.com/rapidsai/cudf/blob/4162d61633c332be91851126df388fb6489aac9c/cpp/include/cudf/utilities/bit.hpp#L73
AFAIK, we currently have three different naming schemes in libcudf related to overflow checking:
- For aggregation, we use
_WITH_OVERFLOWsuffix to denote it has overflow check https://github.com/rapidsai/cudf/blob/aa3cdee199d636d0075b6ae165d8ed09aff0b92a/cpp/include/cudf/aggregation.hpp#L81 - For JIT operators, we use the SQL term
ansi, e.g. https://github.com/rapidsai/cudf/blob/6f8c429d0a532bdfd5d5e4057643addec6f5f486/cpp/include/cudf/detail/operators/ansi_arithmetic.cuh#L32 - The current effort with
safeprefix/suffix
We should really converge on a consistent naming scheme going forward to avoid further fragmentation and branching.
There was a problem hiding this comment.
Yes, we've agreed on using an "_overflow" suffix
| * @throw cudf::data_type_error if the operation is not supported for the types of | ||
| * @p lhs and @p rhs | ||
| */ | ||
| std::pair<std::unique_ptr<column>, std::unique_ptr<column>> binary_operation_safe( |
There was a problem hiding this comment.
Another minor difference I wanted to mention: in the SUM_WITH_OVERFLOW aggregation, both reduce and groupby currently return a single STRUCT column, where the first field is the sum and the second is the overflow flag.
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 groupby or reduce code paths, since the input-output mapping assumes one input column projects to one output column, and there’s no straightforward way to change that.
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.
| * @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 | ||
| }; | ||
|
|
||
| /** | ||
| * @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}`; `rescaled_value` is zero on 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()}; | ||
| // Skip the shift when it would overflow: the rescaled value is meaningless | ||
| // and performing the multiply would be signed-integer-overflow UB. | ||
| if (shift_overflows<Rep, Rad>(x.value(), scale_delta)) { | ||
| return safe_result<Rep, Rad>{fixed_point<Rep, Rad>{scaled_integer<Rep>{Rep{0}, new_scale}}, | ||
| true}; | ||
| } | ||
| 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}}, false}; | ||
| } | ||
|
|
||
| /** | ||
| * @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. When | ||
| * overflow is detected the add is skipped and `value` is zero. | ||
| */ | ||
| template <typename Rep, Radix Rad> | ||
| CUDF_HOST_DEVICE inline safe_result<Rep, Rad> safe_add(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 overflow = lhs_r.overflow || rhs_r.overflow || addition_overflow<Rep>(lv, rv); | ||
| Rep const sum = overflow ? Rep{0} : lv + rv; | ||
| return safe_result<Rep, Rad>{fixed_point<Rep, Rad>{scaled_integer<Rep>{sum, common_scale}}, | ||
| overflow}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked subtraction of two `fixed_point` values | ||
| * | ||
| * When overflow is detected the subtract is skipped and `value` is zero. | ||
| */ | ||
| 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 overflow = lhs_r.overflow || rhs_r.overflow || subtraction_overflow<Rep>(lv, rv); | ||
| Rep const diff = overflow ? Rep{0} : lv - rv; | ||
| return safe_result<Rep, Rad>{fixed_point<Rep, Rad>{scaled_integer<Rep>{diff, common_scale}}, | ||
| overflow}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked multiplication of two `fixed_point` values | ||
| * | ||
| * No rescale is needed -- the result scale is `lhs.scale() + rhs.scale()`. | ||
| * When overflow is detected the multiply is skipped and `value` is zero. | ||
| */ | ||
| 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 overflow = multiplication_overflow<Rep>(lv, rv); | ||
| Rep const prod = overflow ? Rep{0} : 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}}, | ||
| overflow}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked division of two `fixed_point` values | ||
| * | ||
| * Two failure modes are reported as overflow: `INT_MIN / -1` (caught by | ||
| * `division_overflow`) and division by zero. In both cases the divide is | ||
| * skipped -- so we never invoke signed-integer divide overflow or | ||
| * divide-by-zero UB -- and a zero-valued result is returned. | ||
| */ | ||
| 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(); | ||
| scale_type const out_scale{lhs.scale() - rhs.scale()}; | ||
| // Short-circuit on a zero divisor before touching `division_overflow` (which | ||
| // would itself divide) or the divide below. | ||
| bool const overflow = (rv == Rep{0}) || division_overflow<Rep>(lv, rv); | ||
| Rep const quot = overflow ? Rep{0} : lv / rv; | ||
| return safe_result<Rep, Rad>{fixed_point<Rep, Rad>{scaled_integer<Rep>{quot, out_scale}}, | ||
| overflow}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked modulo of two `fixed_point` values | ||
| * | ||
| * The op-level failure modes are divide-by-zero and the `INT_MIN % -1` | ||
| * signed-overflow boundary; the rescale to the common scale can also overflow. | ||
| * All are OR'd into the returned flag, and whenever any of them is set the `%` | ||
| * is skipped and a zero-valued result is returned (so we never invoke | ||
| * `%`-by-zero or signed-overflow UB). | ||
| */ | ||
| 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 lv = lhs_r.value.value(); | ||
| Rep const rv = rhs_r.value.value(); | ||
| bool const overflow = | ||
| lhs_r.overflow || rhs_r.overflow || (rv == Rep{0}) || division_overflow<Rep>(lv, rv); | ||
| Rep const remainder = overflow ? Rep{0} : lv % rv; | ||
| return safe_result<Rep, Rad>{fixed_point<Rep, Rad>{scaled_integer<Rep>{remainder, common_scale}}, | ||
| overflow}; | ||
| } | ||
|
|
||
| /** | ||
| * @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. If the base modulo | ||
| * already overflowed (incl. divide-by-zero), or the correcting add overflows, | ||
| * the correction is skipped and a zero-valued result is returned. | ||
| */ | ||
| 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.overflow || !(m.value.value() < Rep{0})) { return m; } | ||
|
|
||
| // `m.overflow` is false here, so `safe_mod` saw a non-zero divisor: `rv != 0`. | ||
| 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 overflow = rhs_r.overflow || addition_overflow<Rep>(mv, rv); | ||
| Rep const corrected = overflow ? Rep{0} : (mv + rv) % rv; | ||
| return safe_result<Rep, Rad>{ | ||
| fixed_point<Rep, Rad>{scaled_integer<Rep>{corrected, m.value.scale()}}, overflow}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Overflow-checked Python-style modulo: `((x % y) + y) % y`. | ||
| * | ||
| * The intermediate add can overflow; the final `%` cannot. If the base modulo | ||
| * already overflowed (incl. divide-by-zero), or the correcting add overflows, | ||
| * the correction is skipped and a zero-valued result is returned, so we never | ||
| * invoke `%`-by-zero or signed-overflow UB. | ||
| */ | ||
| 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); | ||
| if (m.overflow) { return m; } | ||
|
|
||
| // `m.overflow` is false here, so `safe_mod` saw a non-zero divisor: `rv != 0`. | ||
| 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 overflow = rhs_r.overflow || addition_overflow<Rep>(mv, rv); | ||
| Rep const corrected = overflow ? Rep{0} : (mv + rv) % rv; | ||
| return safe_result<Rep, Rad>{ | ||
| fixed_point<Rep, Rad>{scaled_integer<Rep>{corrected, m.value.scale()}}, 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 |
There was a problem hiding this comment.
can you please confirm each of these operators hasn't been implemented in cudf/detail/operators/*?
There was a problem hiding this comment.
please also merge these into cudf/detail/operators/ansi_arithmetic.cuh or cudf/detail/operators/ansi_casts.cuh.
There was a problem hiding this comment.
ansi_arithmetic.cuh already provides overflow-checked add/sub/mul/div/mod (and abs/neg/precision_check), so it overlaps with safe_add/safe_sub/safe_mul/safe_div.
However, it doesn't detect rescale-induced overflow in add/sub
( Given the overlap, we should converge the two — e.g. have one delegate to the other ie, _safe kernel delegating to ansi_* (after closing ansi_*'s coverage gaps), not changing binary_operation itself).
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.
There was a problem hiding this comment.
I'm not convinced scale-induced overflow/underflow needs to be checked in the device code, whilst it is useful to have.
It is a metadata that should be checked before computing the operators.
Can you please move it outside of the operators and add it as a pre-kernel-launch error check instead.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
DECIMAL64, common rescale ×10^-3
row 0: rep = 5 → 5000 (Pass)
row 1: rep = 9.2e15 → overflows (Fail)
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.
There was a problem hiding this comment.
can you instead implement overflow checks into "checked_arithmetic.cuh" header, using #22836 as base?
Otherwise, reuse the equivalent functions in "checked_arithmetic.cuh". We'd like to reduce code duplication
| * @throw cudf::data_type_error if the operation is not supported for the types of | ||
| * @p lhs and @p rhs | ||
| */ | ||
| std::pair<std::unique_ptr<column>, std::unique_ptr<column>> binary_operation_safe( |
There was a problem hiding this comment.
can you make this behaviour part of binary_operation instead?
There was a problem hiding this comment.
What behavior are you referring to?
There was a problem hiding this comment.
the error checking. rather than having binary_operation and binary_operation_safe, we should have a single binary_operation with enums to specify whether to check for errors.
There was a problem hiding this comment.
I disagree with this suggestion.
| template <typename Rep, Radix Rad> | ||
| struct safe_result { | ||
| fixed_point<Rep, Rad> value; ///< Computed `fixed_point` value | ||
| bool overflow; ///< Whether the producing operation overflowed | ||
| }; |
There was a problem hiding this comment.
can you replace this and all occurrences with cuda::std::expected<decimal, errc>? to be consistent with CUDF's operator library
|
@Avinash-Raj are you planning to revisit this? |
|
@GregoryKimball No, because the pinned ANSI/checked arithmetic mode already provides what I need for Velox-cuDF, throwing an error on arithmetic overflow: facebookincubator/velox#17924 |
|
Closing this PR since it has been superseded by the ANSI mode PRs. |
11 participants
Description
Adds an opt-in path for detecting overflow in decimal fixed-point arithmetic. The default
binary_operation/fixed_pointcode paths are unchanged; callers that want overflow tracking get it through a new set of value-level free functions and a newbinary_operation_safepublic API, all from the same libcudf build (no CMake flag, no new type id).The change lands in three layers.
1. Value-level overflow-aware free functions
New header
cpp/include/cudf/fixed_point/detail/safe_arithmetic.hpp(namespacenumeric::detail) exposes:safe_result<Rep, Rad>— pair offixed_pointvalue +bool overflow.safe_add/safe_sub/safe_mul/safe_div/safe_mod/safe_pmod/safe_pymod— overflow-checked binary operators onfixed_point<Rep, Rad>. Rescale-induced and op-induced overflows are OR'd into the returned flag.safe_rescaled—x.rescaled(s)that surfaces the shift overflow.safe_convert_floating_to_fixed<Fixed>(floating, scale)— overflow-aware floating→decimal conversion built on the conversion primitives below.The
numeric::fixed_pointclass itself is not modified; these are free functions that take regularfixed_pointoperands and compose the per-call overflow flags explicitly. Downstream code paths (thebinary_operation_safekernel today, future overflow-aware reduce / groupby aggregations) consume these primitives directly.2. Column / binaryop level
New public API in
cpp/include/cudf/binaryop.hpp:Returns a
(result, overflow)pair where:resultis the decimal arithmetic result column (same semantics asbinary_operation).overflowis aBOOL8column of the same length. Elementiistrueiff rowiis an active (non-null on both sides) row whose arithmetic or rescale tooutput_typeoverflowed. Null rows and clean rows holdfalse. The column has no null mask.Supported operators:
ADD,SUB,MUL,DIV,MOD,PMOD,PYMOD. All three operand shapes (column,column/scalar,column/column,scalar) are supported. The kernel (cpp/src/binaryop/compiled/binary_ops_safe.cu) writes the per-row overflow bit with a plain store (no atomics), one byte per row, coalesced with the result write. Callers that only need a column-wide yes/no can reduce the overflow column withcudf::reduce(overflow, make_any_aggregation(), ...).3. Overflow-aware float → decimal conversion primitives
The conversion code in
cpp/include/cudf/fixed_point/detail/floating_conversion.hppgrew a singlebool CheckOverflow = falsetemplate parameter (rather than duplicating into*_checkedsiblings) on:checked_left_shift<CheckOverflow>(value, bit_shift)checked_narrow_cast<To, CheckOverflow>(value)multiply_power10_saturating<T, CheckOverflow>(value, pow10)(also made O(1) by precomputing the overflow threshold)shift_to_decimal_pospow<Rep, FloatingType, CheckOverflow>(...)shift_to_decimal_negpow<Rep, FloatingType, CheckOverflow>(...)convert_floating_to_integral_shifting<Rep, FloatingType, CheckOverflow>(...)convert_floating_to_integral<Rep, CheckOverflow, FloatingType>(...)(top-level entry consumed bysafe_convert_floating_to_fixed)When
CheckOverflow = falsethe generated code is unchanged frommain. Whentrue, every primitive returns{value, overflow}with saturating semantics on overflow. The sign-reapply path inconvert_floating_to_integralwas also fixed to handle the-INT_MINboundary without invoking signed UB on the unchecked code path.Motivation
decimal32/decimal64/decimal128silently wrap on overflow today. Downstream engines (velox-cudf, Spark RAPIDS, ANSI-mode pipelines) need a way to detect overflow without rolling their own checked arithmetic or shipping a forked libcudf. This PR:any) and "carry NULL / saturate per row".Tests
New test target
SAFE_ARITHMETIC_TEST(cpp/tests/fixed_point/safe_arithmetic_tests.cpp, 50+ tests):safe_*operators ondecimal32/decimal64/decimal128: happy path + boundary-overflow on each width (AddOverflowSetsFlag,Mul32OverflowSetsFlag,Mul128OverflowSetsFlag,Div{32,64,128}IntMinByNegativeOneOverflows, …).safe_rescaledno-op + shift-overflow.safe_convert_floating_to_fixedfor each width: float / double inputs, positive / negative scale,0.0/-0.0, positive- and negative-overflow branches, scale-induced overflow.static_assertchecks thatsafe_*preserve the operandRep(don't silently upcast).floating_conversion.hpp(newFloatingConversionOverflowTestfixture):checked_left_shift<true>,multiply_power10_saturating<true>,checked_narrow_cast<true>, and the entry pointconvert_floating_to_integral<Rep, true>— including theINT_MIN-exact boundary that the wrapper-level tests don't reach, and a regression check that the checked path's value matches the unchecked path on non-overflowing inputs.New
BinaryOperationSafeDecimaltests incpp/tests/binaryop/binop-compiled-fixed_point-test.cppcover the column-level API: a no-overflow case (asserts an all-falseBOOL8overflow column) and an overflow case (asserts the offending row's slot istrue).