Add decimal division functionality with scale preservation by simoneves · Pull Request #22261 · rapidsai/cudf

simoneves · 2026-04-22T23:48:10Z

Resurrection of #19861, rebased on main for testing with Velox

====

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 #17448

(cherry picked from commit f8b1ff34ef9490b486ab76f2071b86c6d4d3423c)

Description

Checklist

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

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 rapidsai#17448 Co-authored-by: Akihiro Hirota <akihiro-hirota@gmobiz.com> (cherry picked from commit f8b1ff34ef9490b486ab76f2071b86c6d4d3423c)

GregoryKimball · 2026-04-23T03:51:22Z

Thank you @a-hirota for your excellent contribution in #19861! We are looking to refresh and rework your implementation here. 💚 😄

mhaseeb123 · 2026-05-18T16:31:34Z

+/*
+ * Copyright (c) 2025, NVIDIA CORPORATION.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */


Suggested change

/*

* Copyright (c) 2025, NVIDIA CORPORATION.

*

* Licensed under the Apache License, Version 2.0 (the "License");

* you may not use this file except in compliance with the License.

* You may obtain a copy of the License at

*

* http://www.apache.org/licenses/LICENSE-2.0

*

* Unless required by applicable law or agreed to in writing, software

* distributed under the License is distributed on an "AS IS" BASIS,

* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

* See the License for the specific language governing permissions and

* limitations under the License.

*/

/*

* SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION.

* SPDX-License-Identifier: Apache-2.0

*/

mhaseeb123 · 2026-05-18T16:32:17Z

+namespace CUDF_EXPORT cudf {
+
+/**
+ * @addtogroup transformation_decimalops


I think this doxygen group does not already exist and we need to add it. Might need to rename it to match existing patterns if any

mhaseeb123 · 2026-05-18T16:33:23Z

+ * @param lhs         The left operand column
+ * @param rhs         The right operand column
+ * @param rounding_mode The rounding mode to use
+ * @param stream CUDA stream used for device memory operations and kernel launches
+ * @param mr          Device memory resource used to allocate the returned column's device memory
+ * @return            Output column containing the result of the decimal division
+ * @throw cudf::logic_error if @p lhs and @p rhs are different sizes
+ * @throw cudf::logic_error if @p lhs and @p rhs are not decimal types


clang-format likely needed here

mhaseeb123 · 2026-05-18T16:35:02Z

+/**
+ * @brief Performs decimal division between two columns with scale preservation.
+ *
+ * The output contains the result of `divide_decimal(lhs[i], rhs[i])` for all `0 <= i < lhs.size()`


The output contains the result of divide_decimal(lhs[i], rhs[i]) for all 0 <= i < lhs.size()

Result of divide_decimal(lhs[i], rhs[i]) isn't yet defined as this is the first instance of this API overload

mhaseeb123 · 2026-05-18T16:36:50Z

@@ -11,6 +11,7 @@
 import numpy as np


Please separate out python bindings and pytests into a separate follow up PR if possible to limit the scope of this PR

mhaseeb123 · 2026-05-18T16:41:27Z

+ * @tparam Rad1 The radix of the fixed-point numbers
+ * @param lhs The dividend (left-hand side of division)
+ * @param rhs The divisor (right-hand side of division)
+ * @param rounding_mode The rounding mode to use (default: HALF_UP)


No need to add default value here as clear from the param below. Also less future maintenance

Suggested change

* @param rounding_mode The rounding mode to use (default: HALF_UP)

* @param rounding_mode The rounding mode to use

mhaseeb123 · 2026-05-18T16:49:26Z

+  // We need to compensate for the scale difference to preserve the dividend's scale
+  // Standard division would give us scale = lhs.scale() - rhs.scale()
+  // To preserve lhs.scale(), we need to scale up by 10^(-rhs.scale())
+  auto const scale_factor = detail::ipow<Rep1, Rad1>(-static_cast<int>(rhs.scale()));


We should look into reusing utils from src/round/round.cu here if possible to avoid code duplication

mhaseeb123 · 2026-05-18T16:50:40Z

+ * @return A fixed-point number with the same scale as the dividend
+ */
+template <typename Rep1, Radix Rad1>
+CUDF_HOST_DEVICE inline fixed_point<Rep1, Rad1> divide_decimal(


Is the CUDF_HOST_DEVICE needed or could this just be a device only util since i only see it being called from a device function below

mhaseeb123 · 2026-05-18T16:51:41Z

+namespace detail {
+
+template <typename DecimalType>
+struct divide_decimal_functor {


Looks like unused functor?

mhaseeb123 · 2026-05-18T17:02:03Z

+  }
+};
+
+std::unique_ptr<column> divide_decimal_impl(column_view const& lhs,


Would recommend restructuring the code in this file as follows:

namespace CUDF_EXPORT cudf { namespace detail { // Provided two fixed point iterators here and computes decimal_divide from `fixed_point.hpp` using a transform on them. template <LHS, RHS> std::unique_ptr<column> divide_decimal(LHS lhs, RHS rhs, size_type size ...) {} // Detail APIs for decimal_divide(col, col), decimal_divide(col, scalar), decimal_divide(scalar, col) overloads that call the above one with combination of regular (columns) or `cuda::constant_iterator` (scalars) } // namespace detail // public APIs here that just do this std::unique_ptr<column> divide_decimal(..) { CUDF_FUNC_RANGE(); return detail::divide_decimal(..) } } // namespace cudf

mhaseeb123

First-pass human review. Please update the PR description to be inline with cudf template and standards (clear, concise, direct, human-written preferred, no git or commit details unless needed for context)

mhaseeb123 · 2026-05-18T17:16:20Z

@@ -0,0 +1,103 @@
+/*


Nit: Reconsider renaming the file. Here decimal/decimal_ops.hpp the word decimal seems to be repeating, also adds a new folder. Perhaps we could move this to cudf/fixed_point/math_ops.hpp.

Open to suggestions

simoneves requested review from a team as code owners April 22, 2026 23:48

simoneves requested review from galipremsagar, karthikeyann, mroeschke and ttnghia April 22, 2026 23:48

github-actions Bot assigned simoneves Apr 22, 2026

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 Apr 22, 2026

github-project-automation Bot added this to cuDF Python Apr 22, 2026

GPUtester moved this to In Progress in cuDF Python Apr 22, 2026

simoneves force-pushed the simoneves/a-hirota/decimal-division-operations-rebased branch from aefd0bd to e3bbe5b Compare April 23, 2026 00:53

wjxiz1992 mentioned this pull request Apr 23, 2026

Fix cudaErrorIllegalAddress in concatenate_list_elements when inner list has 0 rows #22147

Merged

3 tasks

simoneves added 3 commits April 23, 2026 14:14

DEBUG (may not compile)

4c461fa

Debug that compiles?

02d52af

Debug that compiles NOW?

41c2ab1

GregoryKimball added this to libcudf May 4, 2026

GregoryKimball moved this to Burndown in libcudf May 4, 2026

mhaseeb123 changed the title ~~feat(cudf): Add decimal division functionality with scale preservation~~ Add decimal division functionality with scale preservation May 7, 2026

mhaseeb123 requested changes May 18, 2026

View reviewed changes

mhaseeb123 reviewed May 18, 2026

View reviewed changes

mhaseeb123 mentioned this pull request May 18, 2026

Opt-in overflow tracking for decimal fixed-point arithmetic #22356

Draft

	* @param rounding_mode The rounding mode to use (default: HALF_UP)
	* @param rounding_mode The rounding mode to use

Conversation

simoneves commented Apr 22, 2026

Description

Checklist

Uh oh!

GregoryKimball commented Apr 23, 2026

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!

mhaseeb123 May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhaseeb123 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mhaseeb123 May 18, 2026 •

edited

Loading

mhaseeb123 left a comment •

edited

Loading