Add decimal division functionality with scale preservation by a-hirota · Pull Request #19861 · NVIDIA/cudf

a-hirota · 2025-09-01T14:09:49Z

Description

This PR addresses issue #17448 by implementing decimal division operations that preserve the scale of the dividend, similar to Java's BigDecimal.divide() behavior.

Key changes:

Add C++ decimal division implementation with rounding modes (HALF_UP, HALF_EVEN)
Implement Python bindings through pylibcudf
Add divide_decimal method to DecimalBaseColumn

The implementation supports:

Column-column division
Column-scalar division
Scalar-column division
Scale preservation (maintains dividend's scale)
Rounding modes for precision control

Future Work:

Current implementation uses progressive type promotion (32→64→128 bit) which may impact performance
Future optimization: Pre-calculate required precision to minimize overflow checks

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

copy-pr-bot · 2025-09-01T14:09:53Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Copilot

Pull Request Overview

This PR implements decimal division functionality with scale preservation, addressing issue #17448. The implementation follows Java's BigDecimal.divide() behavior by maintaining the dividend's scale rather than using standard division which would change the scale.

Key changes:

Adds C++ decimal division implementation with HALF_UP and HALF_EVEN rounding modes
Implements Python bindings through pylibcudf with proper type checking and error handling
Extends DecimalBaseColumn with divide_decimal method supporting column-column, column-scalar, and scalar-column operations

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
cpp/include/cudf/decimal/decimal_ops.hpp	New header defining public API for decimal division functions
cpp/include/cudf/fixed_point/fixed_point.hpp	Adds divide_decimal function with rounding modes and scale preservation logic
cpp/src/binaryop/decimal_ops.cu	Core C++ implementation handling different decimal types and null values
python/pylibcudf/pylibcudf/libcudf/decimal/decimal_ops.pxd	Cython declarations for C++ decimal operations
python/pylibcudf/pylibcudf/decimal_division.pyx	Python bindings with type validation and rounding mode conversion
python/cudf/cudf/core/column/decimal.py	High-level divide_decimal method for DecimalBaseColumn
python/cudf/cudf/tests/test_decimal_division.py	Comprehensive test suite covering various scenarios and edge cases

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-01T14:54:54Z

+/**
+ * @brief Helper function to perform division with scale preservation
+ *
+ * NOTE: This function was added for SQL-compatible division but is currently unused.
+ * It preserves the dividend's scale (lhs.scale()) rather than using standard behavior.
+ * Kept for potential future use or removal.
+ *
+ * @tparam Rep1 Representation type of input operands
+ * @tparam Rad1 Radix (base) type
+ * @tparam ReturnRep Representation type for return value (defaults to Rep1)
+ * @param lhs Left hand side operand
+ * @param rhs Right hand side operand
+ * @return Division result with dividend's scale preserved
+ */
+template <typename Rep1, Radix Rad1, typename ReturnRep = Rep1>
+CUDF_HOST_DEVICE inline fixed_point<ReturnRep, Rad1> perform_decimal_division(
+  fixed_point<Rep1, Rad1> const& lhs, fixed_point<Rep1, Rad1> const& rhs)
+{
+#if defined(__CUDACC_DEBUG__)
+  assert(!division_overflow<Rep1>(lhs.value(), rhs.value()) && "fixed_point overflow");
+#endif
+
+  // Calculate adjustment factor based on right operand's scale
+  auto const scale_factor = detail::ipow<Rep1, Rad1>(static_cast<int32_t>(rhs.scale()));
+
+  // Check for potential overflow when scaling left operand
+  bool overflow = multiplication_overflow<Rep1>(lhs.value(), scale_factor);
+
+  // Normal processing (no overflow)
+  if (!overflow) {
+    ReturnRep result_value = detail::left_shift<ReturnRep, Rad1>(
+                               lhs.value(), scale_type{static_cast<int32_t>(rhs.scale())}) /
+                             rhs.value();
+
+#if defined(__CUDACC_DEBUG__)
+    printf("No overflow in division calculation.\n");
+#endif
+


This function is marked as unused and "kept for potential future use or removal." Since it's not being used in this implementation, consider removing it to avoid code bloat and confusion about which division function to use.

Suggested change

/**

* @brief Helper function to perform division with scale preservation

*

* NOTE: This function was added for SQL-compatible division but is currently unused.

* It preserves the dividend's scale (lhs.scale()) rather than using standard behavior.

* Kept for potential future use or removal.

*

* @tparam Rep1 Representation type of input operands

* @tparam Rad1 Radix (base) type

* @tparam ReturnRep Representation type for return value (defaults to Rep1)

* @param lhs Left hand side operand

* @param rhs Right hand side operand

* @return Division result with dividend's scale preserved

*/

template <typename Rep1, Radix Rad1, typename ReturnRep = Rep1>

CUDF_HOST_DEVICE inline fixed_point<ReturnRep, Rad1> perform_decimal_division(

fixed_point<Rep1, Rad1> const& lhs, fixed_point<Rep1, Rad1> const& rhs)

{

#if defined(__CUDACC_DEBUG__)

assert(!division_overflow<Rep1>(lhs.value(), rhs.value()) && "fixed_point overflow");

#endif

// Calculate adjustment factor based on right operand's scale

auto const scale_factor = detail::ipow<Rep1, Rad1>(static_cast<int32_t>(rhs.scale()));

// Check for potential overflow when scaling left operand

bool overflow = multiplication_overflow<Rep1>(lhs.value(), scale_factor);

// Normal processing (no overflow)

if (!overflow) {

ReturnRep result_value = detail::left_shift<ReturnRep, Rad1>(

lhs.value(), scale_type{static_cast<int32_t>(rhs.scale())}) /

rhs.value();

#if defined(__CUDACC_DEBUG__)

printf("No overflow in division calculation.\n");

#endif

Copilot · 2025-09-01T14:54:54Z

+        cpp_mode = <cpp_decimal_rounding_mode>0  # HALF_UP = 0
+    elif rounding_mode == DecimalRoundingMode.HALF_EVEN:
+        cpp_mode = <cpp_decimal_rounding_mode>1  # HALF_EVEN = 1


The magic numbers 0 and 1 are hardcoded here and repeated in multiple functions. Consider using the enum values directly (e.g., cpp_decimal_rounding_mode.HALF_UP) to avoid magic numbers and improve maintainability.

Suggested change

cpp_mode = <cpp_decimal_rounding_mode>0 # HALF_UP = 0

elif rounding_mode == DecimalRoundingMode.HALF_EVEN:

cpp_mode = <cpp_decimal_rounding_mode>1 # HALF_EVEN = 1

cpp_mode = cpp_decimal_rounding_mode.HALF_UP

elif rounding_mode == DecimalRoundingMode.HALF_EVEN:

cpp_mode = cpp_decimal_rounding_mode.HALF_EVEN

Implements divide_decimal for fixed-point decimal columns that preserves the dividend's scale, similar to Java BigDecimal.divide() with rounding mode. - Add divide_decimal C++ implementation with HALF_UP and HALF_EVEN rounding - Add Python bindings via pylibcudf - Add divide_decimal method to DecimalBaseColumn - Add comprehensive tests for various scales and rounding modes - Fix critical bug in C++ implementation for different scales - Add Doxygen documentation for new functions Addresses issue NVIDIA#17448 Co-authored-by: Akihiro Hirota <akihiro-hirota@gmobiz.com>

a-hirota · 2025-09-02T23:59:03Z

@davidwendt @bdice
Sorry for the force pushes after the review request. Reverted back to 969285a (the commit when review was requested). Ready for review.

vyasr · 2025-09-03T23:50:37Z

Hi @a-hirota thanks for the PR! JFYI @bdice is out until next week. Since he's spent the most time with decimals recently I'm going to defer to him to review when he's back, so it will be a few days. We appreciate your patience!

bdice · 2025-09-08T22:35:45Z

Hi! Acknowledging the request for review. Optimistically I will try to review tomorrow but I might need a couple days before I can give this a deep dive.

bdice

Thanks for the PR! Overall, this seems like it's on the right track. I have some high level guidance on naming, where code should live, what APIs should be public (or not), test cases that we should cover, and so forth. I think a few iterations will get this into the right place!

bdice · 2025-09-17T14:54:06Z

We should probably rename "decimal" to "fixed point" for consistency with the rest of libcudf. The code paths we have don't force the base to be 10 (could be binary).

That would put this in cudf/fixed_point/fixed_point_operators.hpp or similar.

bdice · 2025-09-17T14:55:48Z

+ * @param rounding_mode The rounding mode to use (default: HALF_UP)
+ * @return A fixed-point number with the same scale as the dividend
+ */
+template <typename Rep1, Radix Rad1>


If this is specialized for decimals, we might need to require that the radix is 10. But if this logic is generic, we should call it "fixed point" rather than "decimal."

bdice · 2025-09-17T14:58:29Z

+      }
+    } else if (rounding_mode == decimal_rounding_mode::HALF_EVEN) {
+      // Banker's rounding
+      // Avoid abs() ambiguity for __int128 by using conditional


Let's copy this pattern. Maybe write it into a helper function if it's repeated.

https://github.com/rapidsai/cudf/blob/7362ce261962c62a406402d7c7b965d3bcbaf7a5/cpp/include/cudf/fixed_point/detail/floating_conversion.hpp#L1104-L1109

bdice · 2025-09-17T15:04:23Z

+  WidestRep remainder       = scaled_dividend % wide_divisor;
+
+  // Apply rounding
+  if (rounding_mode == decimal_rounding_mode::HALF_UP) {


Can we pull out some of this logic into a function templated on the rep? It looks like it's repeated several times.

bdice · 2025-09-17T15:06:37Z

+                                            rmm::cuda_stream_view stream,
+                                            rmm::device_async_resource_ref mr)
+{
+  using namespace numeric;


I'd like to avoid this using namespace if possible. It is easier to trace when namespaces are explicitly written.

bdice · 2025-09-17T15:17:40Z

+        s2 = cudf.Series([Decimal("2.0"), Decimal("3.0"), Decimal("4.0")])
+
+        # divide_decimal (scale preserved)
+        decimal_result = s1._column.divide_decimal(s2._column)


Continuing on some comments from above: users should not need to invoke private APIs for this functionality. It should be done by converting cuDF Series to pylibcudf columns (with public APIs) and calling a free function from pylibcudf.

bdice · 2025-09-17T15:18:29Z

+    >>> lhs = pylibcudf.column_from_decimal_values([1.23, 4.56], scale=-2)
+    >>> rhs = pylibcudf.column_from_decimal_values([2.0, 3.0], scale=-1)
+    >>> # Divide preserving lhs scale
+    >>> result = divide_decimal(lhs, rhs, DecimalRoundingMode.HALF_UP)


Yes, this is the public API we want to recommend for users.

bdice · 2025-09-17T15:19:32Z

+    else:
+        raise ValueError(f"Invalid rounding mode: {rounding_mode}")
+
+    # Check that both columns are decimal types


Is this check being performed at the C++ layer? I think we might want to defer this and let C++ handle the type validation.

bdice · 2025-09-17T15:20:58Z

+    return Column.from_libcudf(move(result))
+
+
+cpdef Column divide_decimal_column_scalar(


There may be some Cython fused-type magic we need to invoke here to make overloads work properly. Something like this: https://github.com/rapidsai/cudf/blob/7362ce261962c62a406402d7c7b965d3bcbaf7a5/python/pylibcudf/pylibcudf/lists.pxd#L16-L18

Please give that a try.

bdice · 2025-09-17T15:23:19Z

+ * Specifies how to round the result when performing decimal division
+ * with scale preservation.
+ */
+enum class decimal_rounding_mode : int32_t {


Could we reuse this enum?

https://github.com/rapidsai/cudf/blob/764b63b2765f181f9c15b663aaa076108a6b9c36/cpp/include/cudf/round.hpp#L33-L40

revans2

I am not sure what the requester of this feature really wants, but this does not fit with what we want for Spark. Spark deals with decimal divide in inconsistent and configurable ways. The adjustments to the precision and scale changed in 3.3.0. The divide in an average is treated differently from regular divide. There is also the config https://github.com/apache/spark/blob/e08c15b62712303942284cb90d6a1b004f69652c/sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala#L4115-L4124 which can change how the precision and scale change when the precision would need to go above 38 to get the final result. To make this work in the same way as Spark we have multiple different implementations of DecimalDivide. In general we cast each LHS and RHS decimal value to an intermediate type that would give us enough output to match the desired output precision+1 and scale+1 so that we can do the HALF_UP rounding being done here. Then we do the rounding by casting the result to the final precision and scale. But that only works if the true intermediate (output precision +1 and scale +1) would fit in a DECIMAL_128. If not, then we have to call into a custom kernel that we wrote which does a 256 by 256 bit decimal divide. We also have to check to see if the result is too large to actually fit in the specified output and handle those overflow cases. If anyone wants details about all of this I am happy to provide them.

bdice · 2025-09-17T19:22:52Z

@a-hirota It might be good to do some comparisons across libraries and enumerate all of the engines that you expect to match this set of conventions, and list them in the comments.

@revans2 Could you point to the decimal division test cases you use in Spark? It would be good to expand the tests for this feature to include all of those cases. I am guessing you've run across far more edge cases than I can think of.

vyasr · 2025-09-24T17:49:21Z

I'm afraid I haven't had the chance to review this yet myself, apologies. Given the discussions above, though, it seems like we still need to have higher-level discussions, so I've bumped this to 25.12.

vyasr · 2026-02-27T19:18:53Z

@a-hirota can you comment on the concerns raised by @revans2 above? What conventions are you hoping to implement, and which engines match/don't match?

revans2 · 2026-03-02T14:49:01Z

Sorry I missed the comment from @bdice about test cases. Most of our test cases are a direct comparison to Spark.

https://github.com/NVIDIA/spark-rapids/blob/53c9c8ce698b59ea2686eb85d050263480162f6a/integration_tests/src/main/python/arithmetic_ops_test.py#L264-L306

Tests general division with ANSI disabled so we don't get exceptions, we get nulls on overflow. We focus on some large ranges of scale and precision explicitly as a part of this, and mixing different scales and precisions along with int like inputs.

vyasr · 2026-03-10T15:25:32Z

Thanks! @a-hirota hopefully that pointer gives you some idea of the different behaviors we need to support here.

GregoryKimball · 2026-04-21T04:45:03Z

@simoneves I understand you were examining this implementation to see if it helps with Velox as well. Does this method for division meet your needs?

simoneves · 2026-04-22T23:29:17Z

@a-hirota there is renewed interest here at NVIDIA in this PR. Please let us know if you would be interested in collaborating on completing it, or if you are OK with us taking it over from you. I tried emailing you at the address associated with your GitHub account, but it bounced. If you prefer you can reply to me at seves@nvidia.com

vyasr · 2026-06-26T18:12:52Z

Closing as replaced by #22261. @a-hirota thank you for your hard work on this! Please feel free to coordinate with @simoneves on his PR as much as you would like, hopefully we can develop something that meets everyone's needs there.

github-actions Bot assigned a-hirota Sep 1, 2025

a-hirota force-pushed the feature/decimal-division-operations branch from ab27895 to 60fce03 Compare September 1, 2025 14:21

a-hirota marked this pull request as ready for review September 1, 2025 14:53

Copilot AI review requested due to automatic review settings September 1, 2025 14:53

a-hirota requested review from a team as code owners September 1, 2025 14:53

a-hirota requested review from mhaseeb123, rjzamora and vyasr September 1, 2025 14:53

a-hirota mentioned this pull request Sep 1, 2025

[BUG] Decimal division loses precision when dividing by decimals with fractional parts #17448

Open

Copilot AI reviewed Sep 1, 2025

View reviewed changes

a-hirota force-pushed the feature/decimal-division-operations branch from 60fce03 to fd3a8a4 Compare September 1, 2025 22:28

a-hirota force-pushed the feature/decimal-division-operations branch from fd3a8a4 to 969285a Compare September 2, 2025 09:59

davidwendt requested a review from bdice September 2, 2025 11:30

a-hirota force-pushed the feature/decimal-division-operations branch 2 times, most recently from 19e60bc to 969285a Compare September 2, 2025 22:34

Merge branch 'branch-25.10' into feature/decimal-division-operations

7983977

github-actions Bot added libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. CMake CMake build issue pylibcudf Issues specific to the pylibcudf package labels Sep 17, 2025

github-actions Bot assigned bdice Sep 17, 2025

github-project-automation Bot added this to cuDF Python Sep 17, 2025

GPUtester moved this to In Progress in cuDF Python Sep 17, 2025

bdice requested changes Sep 17, 2025

View reviewed changes

revans2 reviewed Sep 17, 2025

View reviewed changes

vyasr changed the base branch from branch-25.10 to branch-25.12 September 24, 2025 17:46

GregoryKimball mentioned this pull request Apr 15, 2026

[FEA] libcudf implementation of "intuitive" decimal division #22157

Open

GregoryKimball requested a review from simoneves April 21, 2026 04:45

GregoryKimball assigned simoneves and unassigned bdice Apr 21, 2026

GregoryKimball added this to libcudf Apr 21, 2026

GregoryKimball moved this to Burndown in libcudf Apr 21, 2026

simoneves mentioned this pull request Apr 22, 2026

Add decimal division functionality with scale preservation #22261

Open

3 tasks

GregoryKimball removed this from libcudf May 4, 2026

vyasr closed this Jun 26, 2026

github-project-automation Bot moved this from In Progress to Done in cuDF Python Jun 26, 2026

		return Column.from_libcudf(move(result))


		cpdef Column divide_decimal_column_scalar(

Uh oh!

Conversation

a-hirota commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

copy-pr-bot Bot commented Sep 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

a-hirota commented Sep 2, 2025

Uh oh!

vyasr commented Sep 3, 2025

Uh oh!

bdice commented Sep 8, 2025

Uh oh!

bdice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

revans2 left a comment

Choose a reason for hiding this comment

Uh oh!

bdice commented Sep 17, 2025

Uh oh!

vyasr commented Sep 24, 2025

Uh oh!

vyasr commented Feb 27, 2026

Uh oh!

revans2 commented Mar 2, 2026

Uh oh!

vyasr commented Mar 10, 2026

Uh oh!

GregoryKimball commented Apr 21, 2026

Uh oh!

simoneves commented Apr 22, 2026

Uh oh!

vyasr commented Jun 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

a-hirota commented Sep 1, 2025 •

edited

Loading