-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 all 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 |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2019-2025, NVIDIA CORPORATION. | ||
| * SPDX-FileCopyrightText: Copyright (c) 2019-2026, NVIDIA CORPORATION. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
|
|
@@ -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,130 @@ 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 between a scalar and a column, | ||
| * with a per-row overflow column. | ||
| * | ||
| * The result column contains `op(lhs, rhs[i])` for all `0 <= i < rhs.size()`, | ||
| * matching the semantics of `binary_operation` for the seven supported decimal | ||
| * arithmetic operators (ADD, SUB, MUL, DIV, MOD, PMOD, PYMOD). | ||
| * | ||
| * Additionally returns a `BOOL8` column of the same length as the result. | ||
| * Element `i` is `true` iff row `i` is an active (non-null on both sides) row | ||
| * whose arithmetic or rescale to @p output_type overflowed (or, for DIV / MOD | ||
| * / PMOD / PYMOD, divided by zero). Null rows and clean rows hold `false`. | ||
| * The overflow column has no null mask. | ||
| * | ||
| * @p lhs, @p rhs, and @p output_type must share the same decimal storage type | ||
| * (e.g. all `DECIMAL64`); mixing decimal widths or pairing decimal with a | ||
| * non-decimal operand is not supported on this path. | ||
| * | ||
| * @param lhs The left operand decimal scalar | ||
| * @param rhs The right operand decimal column | ||
| * @param op The binary operator | ||
| * @param output_type The desired data type of the result column (must be a base-10 decimal) | ||
| * @param stream CUDA stream used for device memory operations and kernel launches | ||
| * @param mr Device memory resource used to allocate the returned columns' device memory | ||
| * @return A pair `{result, overflow}` where `result` is the arithmetic result column and | ||
| * `overflow` is the per-row `BOOL8` overflow column described above. | ||
| * @throw cudf::logic_error if @p lhs or @p rhs is not a fixed-point type | ||
| * @throw cudf::logic_error if @p op is not one of ADD, SUB, MUL, DIV, MOD, PMOD, PYMOD | ||
| * @throw cudf::logic_error if @p lhs, @p rhs, and @p output_type do not share the same | ||
| * decimal storage type | ||
| * @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( | ||
|
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()); | ||
|
|
||
| /** | ||
| * @brief Decimal fixed-point binary operation between a column and a scalar, | ||
| * with a per-row overflow column. | ||
| * | ||
| * The result column contains `op(lhs[i], rhs)` for all `0 <= i < lhs.size()`, | ||
| * matching the semantics of `binary_operation` for the seven supported decimal | ||
| * arithmetic operators (ADD, SUB, MUL, DIV, MOD, PMOD, PYMOD). | ||
| * | ||
| * Additionally returns a `BOOL8` column of the same length as the result. | ||
| * Element `i` is `true` iff row `i` is an active (non-null on both sides) row | ||
| * whose arithmetic or rescale to @p output_type overflowed (or, for DIV / MOD | ||
| * / PMOD / PYMOD, divided by zero). Null rows and clean rows hold `false`. | ||
| * The overflow column has no null mask. | ||
| * | ||
| * @p lhs, @p rhs, and @p output_type must share the same decimal storage type | ||
| * (e.g. all `DECIMAL64`); mixing decimal widths or pairing decimal with a | ||
| * non-decimal operand is not supported on this path. | ||
| * | ||
| * @param lhs The left operand decimal column | ||
| * @param rhs The right operand decimal scalar | ||
| * @param op The binary operator | ||
| * @param output_type The desired data type of the result column (must be a base-10 decimal) | ||
| * @param stream CUDA stream used for device memory operations and kernel launches | ||
| * @param mr Device memory resource used to allocate the returned columns' device memory | ||
| * @return A pair `{result, overflow}` where `result` is the arithmetic result column and | ||
| * `overflow` is the per-row `BOOL8` overflow column described above. | ||
| * @throw cudf::logic_error if @p lhs or @p rhs is not a fixed-point type | ||
| * @throw cudf::logic_error if @p op is not one of ADD, SUB, MUL, DIV, MOD, PMOD, PYMOD | ||
| * @throw cudf::logic_error if @p lhs, @p rhs, and @p output_type do not share the same | ||
| * decimal storage type | ||
| * @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( | ||
|
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()); | ||
|
|
||
| /** | ||
| * @brief Decimal fixed-point binary operation between two columns, | ||
| * with a per-row overflow column. | ||
| * | ||
| * The result column contains `op(lhs[i], rhs[i])` for all `0 <= i < lhs.size()`, | ||
| * matching the semantics of `binary_operation` for the seven supported decimal | ||
| * arithmetic operators (ADD, SUB, MUL, DIV, MOD, PMOD, PYMOD). | ||
| * | ||
| * Additionally returns a `BOOL8` column of the same length as the result. | ||
| * Element `i` is `true` iff row `i` is an active (non-null on both sides) row | ||
| * whose arithmetic or rescale to @p output_type overflowed (or, for DIV / MOD | ||
| * / PMOD / PYMOD, divided by zero). Null rows and clean rows hold `false`. | ||
| * The overflow column has no null mask. | ||
| * | ||
| * @p lhs, @p rhs, and @p output_type must share the same decimal storage type | ||
| * (e.g. all `DECIMAL64`); mixing decimal widths or pairing decimal with a | ||
| * non-decimal operand is not supported on this path. | ||
| * | ||
| * @param lhs The left operand decimal column | ||
| * @param rhs The right operand decimal column | ||
| * @param op The binary operator | ||
| * @param output_type The desired data type of the result column (must be a base-10 decimal) | ||
| * @param stream CUDA stream used for device memory operations and kernel launches | ||
| * @param mr Device memory resource used to allocate the returned columns' device memory | ||
| * @return A pair `{result, overflow}` where `result` is the arithmetic result column and | ||
| * `overflow` is the per-row `BOOL8` overflow column described above. | ||
| * @throw cudf::logic_error if @p lhs and @p rhs are different sizes | ||
| * @throw cudf::logic_error if @p lhs or @p rhs is not a fixed-point type | ||
| * @throw cudf::logic_error if @p op is not one of ADD, SUB, MUL, DIV, MOD, PMOD, PYMOD | ||
| * @throw cudf::logic_error if @p lhs, @p rhs, and @p output_type do not share the same | ||
| * decimal storage type | ||
| * @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( | ||
| 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. | ||
|
|
||
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