Uh oh!
There was an error while loading. Please reload this page.
GH-46901: [C++][Compute] Add remainder and modulo kernels - #48914
GH-46901: [C++][Compute] Add remainder and modulo kernels#48914fangchenli wants to merge 1 commit into
Conversation
tadeja
left a comment
There was a problem hiding this comment.
@fangchenli, thank you for your work and patience here. It would be great to continue with this effort. Would you consider the following adjustments?
- Rename
modandmod_checkedtomoduloandmodulo_checkedfor more clarity and to follow the terminology in divmod proposal (so reminder uses truncated C++ semantics with result of dividend sign, modulo uses floored Python/R semantics with result of divisor sign. For exampleremainder(-7, 3) == -1,modulo(-7,3) == 2) - Use a dedicated decimal output resolver instead of
ResolveDecimalAdditionOrSubtractionOutputsimilar to suggested changes. (Addition/subtraction need an extra precision digit for possible carry -const int32_t precision = std::max(p1 - s1, p2 - s2) + scale + 1;
but reminder/modulo don't need that extra digit.) Two failing examples:
TEST_F(TestBinaryArithmeticDecimal, RemainderMaximumPrecision) {
auto left = ScalarFromJSON(decimal128(38, 0), R"("7")");
auto right = ScalarFromJSON(decimal128(38, 0), R"("3")");
auto expected = ScalarFromJSON(decimal128(38, 0), R"("1")");
CheckScalarBinary("remainder", left, right, expected);
}Result -> Invalid: Decimal precision out of range [1, 38]: 39
TEST_F(TestBinaryArithmeticDecimal, ModuloMaximumPrecision) {
auto left = ScalarFromJSON(decimal256(76, 0), R"("-7")");
auto right = ScalarFromJSON(decimal256(76, 0), R"("3")");
auto expected = ScalarFromJSON(decimal256(76, 0), R"("2")");
CheckScalarBinary("mod", left, right, expected);
}Result -> Invalid: Decimal precision out of range [1, 76]: 77
- It would be good to add more tests to verify inputs of the same decimal type produce the same output type, mixed scales and precisions, Decimal128/Decimal256 promotion and maximum-precision decimals.
- Also rebase on main.
These can remain follow-ups: combined divmod #27909, floor division #39386 and duration support.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
570f24c to
67c05a2Compare67c05a2 to
6a4edb4CompareThere was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds new C++ compute arithmetic kernels for truncated remainder (remainder*) and floored modulo (modulo*), including overflow-checking variants, plus supporting decimal type resolution, documentation, and tests.
Changes:
- Register
remainder,remainder_checked,modulo, andmodulo_checkedscalar functions (including decimal output type resolution). - Add overflow-safe integer
%helper (ModuloWithOverflow*) and decimal256%operator support. - Extend C++ compute documentation and scalar arithmetic tests to cover semantics (truncated vs floored) across numeric types.
File summaries
| File | Description |
|---|---|
| docs/source/cpp/compute.rst | Documents new modulo* / remainder* kernels and their semantics. |
| cpp/src/arrow/util/int_util_overflow.h | Adds overflow-/trap-safe modulo helper for integers. |
| cpp/src/arrow/util/basic_decimal.h | Declares BasicDecimal256 operator%. |
| cpp/src/arrow/util/basic_decimal.cc | Implements BasicDecimal256 operator% via Divide remainder. |
| cpp/src/arrow/compute/kernels/scalar_arithmetic.cc | Adds decimal output resolver and registers new functions/docs. |
| cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc | Adds coverage for remainder/modulo semantics across types. |
| cpp/src/arrow/compute/kernels/base_arithmetic_internal.h | Implements functors for remainder/modulo and checked variants. |
| cpp/src/arrow/compute/api_scalar.h | Exposes compute::Remainder / compute::Modulo public APIs. |
| cpp/src/arrow/compute/api_scalar.cc | Wires public APIs to kernel names via SCALAR_ARITHMETIC_BINARY. |
Review details
Suppressed comments (3)
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:1201
remainder_checkedalso applies to floating-point and decimal inputs, but the one-line summary says "after integer division". Consider updating the summary to avoid implying it is integer-only.
"Compute the remainder after integer division (truncated)",
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:1214
- The
modulodoc only mentions integer division by zero, but the unchecked floating-point kernel yields NaN and the decimal kernel raises an error. Clarify divide-by-zero behavior across numeric types for accuracy.
"Integer division by zero returns an error."),
cpp/src/arrow/compute/api_scalar.h:694
- Same as above for
Modulo: decimals raise on a zero divisor, and floating-point divide-by-zero depends onArithmeticOptions::check_overflow. The current comment suggests this is integer-only.
/// Array values must be the same length. If either argument is null the result
/// will be null. For integer types, if there is a zero divisor, an error will be
/// raised.
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
6a4edb4 to
bfd778cCompareThere was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
cpp/src/arrow/compute/api_scalar.h:677
- This comment says divide-by-zero errors apply only to integer types, but
remainderalso raisesInvalidon zero divisor for decimal inputs (see decimal branch in kernels). Update the API doc to include decimals (and avoid implying floats/decimals behave like integers here).
/// Array values must be the same length. If either argument is null the result
/// will be null. For integer and decimal types, if there is a zero divisor, an
/// error will be raised. For floating-point types, a zero divisor yields NaN
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:1214
modulo_docsays divide-by-zero errors apply only to integer inputs, but the kernel also errors for decimal inputs and returns NaN for floating-point inputs in the unchecked variant. Please update the wording to reflect actual behavior.
const FunctionDoc modulo_doc{
"Compute the modulo of the arguments element-wise",
("The result has the same sign as the divisor (floored division).\n"
"This is equivalent to Python's '%' operator.\n"
"Integer and decimal division by zero returns an error, while\n"
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bfd778c to
baf89e2CompareUh oh!
There was an error while loading. Please reload this page.
Add the `remainder`/`remainder_checked` (truncated, sign follows the dividend) and `modulo`/`modulo_checked` (floored, sign follows the divisor) scalar arithmetic kernels for integer, floating-point and decimal inputs. Decimal arguments are promoted to a common scale like `add`, but the result type is resolved by a dedicated resolver: a remainder is always smaller in magnitude than the divisor, so no extra digit is needed for a carry and the result is `precision = max(p1, p2)`, `scale = s1`. This keeps maximum-precision inputs (decimal128(38, 0), decimal256(76, 0)) from overflowing the decimal precision range. Co-authored-by: tadeja <864005+tadeja@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NpB1vVJngVjyyWvWn4AUm7
baf89e2 to
2d1d850CompareThere was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
docs/source/cpp/compute.rst:588
- Note (5) is attached to both
remainderandremainder_checked, but the text currently says floating-point division by zero returns NaN. In the implementation,remainder_checkedreturns an error on floating-point division by zero, so the docs should distinguish the checked behavior.
* \(5) Computes the truncated remainder, where the result has the same sign as
the dividend. This is equivalent to C/C++'s ``%`` operator. Integer and
decimal division by zero returns an error, while floating-point division by
zero returns NaN. Decimal arguments are promoted to a common scale ``s``;
the result then has ``scale = s`` and ``precision = max(p1, p2)``.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
| * \(4) Computes the floored modulo, where the result has the same sign as the | ||
| divisor. This is equivalent to Python's ``%`` operator. Integer and decimal | ||
| division by zero returns an error, while floating-point division by zero | ||
| returns NaN. Decimal arguments are promoted to a common scale ``s``; the | ||
| result then has ``scale = s`` and ``precision = max(p1, p2)``. |
There was a problem hiding this comment.
Perhaps a change like:
Division by zero returns an error for integer and decimal inputs. For floating-point inputs it returns ``NaN`` in ``modulo`` and an error in ``modulo_checked``.
tadeja
left a comment
There was a problem hiding this comment.
Thanks, @fangchenli !
Just minor comments from my side, and let's ask for further reviews.
| * \(4) Computes the floored modulo, where the result has the same sign as the | ||
| divisor. This is equivalent to Python's ``%`` operator. Integer and decimal | ||
| division by zero returns an error, while floating-point division by zero | ||
| returns NaN. Decimal arguments are promoted to a common scale ``s``; the | ||
| result then has ``scale = s`` and ``precision = max(p1, p2)``. |
There was a problem hiding this comment.
Perhaps a change like:
Division by zero returns an error for integer and decimal inputs. For floating-point inputs it returns ``NaN`` in ``modulo`` and an error in ``modulo_checked``.| * \(5) Computes the truncated remainder, where the result has the same sign as | ||
| the dividend. This is equivalent to C/C++'s ``%`` operator. Integer and | ||
| decimal division by zero returns an error, while floating-point division by |
There was a problem hiding this comment.
... similarly:
For floating-point inputs, it returns ``NaN`` in ``remainder`` and an error in ``remainder_checked``.| // ============== MOD (Floored) Tests ============== |
| this->AssertBinop(Modulo, "[-7]", "[3]", "[2]"); | ||
| this->AssertBinop(Modulo, "[7]", "[-3]", "[-2]"); | ||
| this->AssertBinop(Modulo, "[-7]", "[-3]", "[-1]"); | ||
| // Edge case: -1 mod positive |
There was a problem hiding this comment.
another nitpick: modulo (or "negative dividend and positive divisor")
tadeja
commented
Sep 3, 2026
@fangchenli Could you also add the four new functions to the Python API autosummary to document the automatically exposed diff --git a/docs/source/python/api/compute.rst b/docs/source/python/api/compute.rst--- a/docs/source/python/api/compute.rst+++ b/docs/source/python/api/compute.rst@@ -96,12 +96,16 @@ throws an ``ArrowInvalid`` exception when overflow is detected.
exp
expm1
hypot
+ modulo+ modulo_checked
multiply
multiply_checked
negate
negate_checked
power
power_checked
+ remainder+ remainder_checked
sign
sqrt
sqrt_checked |
tadeja
commented
Sep 3, 2026
Ah! it would be beneficial to add a Python test, perhaps like this diff --git a/python/pyarrow/tests/test_compute.py b/python/pyarrow/tests/test_compute.py--- a/python/pyarrow/tests/test_compute.py+++ b/python/pyarrow/tests/test_compute.py@@ -1944,6 +1944,18 @@ def test_arithmetic_multiply():
assert result.equals(expected)
+def test_arithmetic_remainder_modulo():+ left = pa.array([7, -7, 7, -7, None])+ right = pa.array([3, 3, -3, -3, 3])+ expected_remainder = [1, -1, 1, -1, None]+ expected_modulo = [1, 2, -2, -1, None]++ assert pc.remainder(left, right).to_pylist() == expected_remainder+ assert pc.remainder_checked(left, right).to_pylist() == expected_remainder+ assert pc.modulo(left, right).to_pylist() == expected_modulo+ assert pc.modulo_checked(left, right).to_pylist() == expected_modulo++
@pytest.mark.parametrize("ty", ["round", "round_to_multiple"])
def test_round_to_integer(ty): |
Rationale for this change
Arrow is currently missing remainder and modulo kernels.
What changes are included in this PR?
Add the kernels
remainder,remainder_checked,modulo, andmodulo_checked, following the terminology in the divmod proposal:remainderuses truncated (C/C++) semantics — the result has the sign of the dividend, e.g.remainder(-7, 3) == -1.modulouses floored (Python/R) semantics — the result has the sign of the divisor, e.g.modulo(-7, 3) == 2.Both are supported for integer, floating-point and decimal inputs, and are exposed as
compute::Remainder/compute::Moduloinapi_scalar.h.Decimal arguments are promoted to a common scale like
add, but the result type is resolved by a dedicated resolver rather than reusingResolveDecimalAdditionOrSubtractionOutput: a remainder is always smaller in magnitude than the divisor, so no extra digit is needed for a carry, and the result isprecision = max(p1, p2),scale = s1. This keeps maximum-precision inputs (decimal128(38, 0),decimal256(76, 0)) from overflowing the decimal precision range.Left as follow-ups: combined divmod #27909, floor division #39386, and duration support.
Are these changes tested?
Yes. New tests cover truncated vs. floored semantics across signed/unsigned integer, floating-point and decimal types, division by zero,
INT_MIN % -1overflow, and decimal result-type resolution (same type in/out, mixed precisions and scales, Decimal128/Decimal256 promotion, decimal/integer promotion, and maximum-precision decimals).Are there any user-facing changes?
Yes — four new compute functions (
remainder,remainder_checked,modulo,modulo_checked), the correspondingcompute::Remainder/compute::ModuloC++ APIs, and a newBasicDecimal256 operator%. These are additions only; no existing API is changed or removed.