You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "fangchenli (via GitHub)" <gi...@apache.org> on 2026/01/20 22:35:20 UTC
[PR] GH-46901: [C++][Compute] Add remainder mod kernels [arrow]
fangchenli opened a new pull request, #48914:
URL: https://github.com/apache/arrow/pull/48914
### Rationale for this change
Arrow is currently missing remainder and modulo kernels.
### What changes are included in this PR?
Add kernels `remainder`, `remainder_checked`, `mod`, and 'mod_checked'
### Are these changes tested?
Yes.
### Are there any user-facing changes?
**This PR includes breaking changes to public APIs.** (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #48914:
URL: https://github.com/apache/arrow/pull/48914#discussion_r3948555873
##########
docs/source/cpp/compute.rst:
##########
@@ -566,6 +575,18 @@ Mixed time resolution temporal inputs will be cast to finest input resolution.
intermediate stages of the computation. If either argument is infinite, the
result is ``+Inf`` even if the other argument is NaN.
+* \(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)``.
Review Comment:
Agreed on this point.
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -468,6 +469,172 @@ struct FloatingDivideChecked {
// TODO: Add decimal
};
+// Remainder (truncated): result has same sign as dividend (C/C++ semantics)
+struct Remainder {
Review Comment:
This currently duplicates some logic across all four functors. You could introduce `checked` and `remainder_mode` template parameters and reduce this somewhat. See sketch below in the comments.
```cpp
enum class RemainderMode { kTruncated, kFloored };
template <RemainderMode Mode, typename T, typename Divisor>
T FinishRemainder(T remainder, Divisor divisor) {
if constexpr (Mode == RemainderMode::kTruncated) {
return remainder;
}
if constexpr (std::is_floating_point_v<T>) {
if (remainder == 0) {
// Preserve the sign based on the divisor for zero results.
return std::copysign(remainder, divisor);
}
}
if constexpr (!std::is_unsigned_v<T>) {
const T zero{};
if ((remainder > zero && divisor < zero) || (remainder < zero && divisor > zero)) {
remainder += divisor;
}
}
return remainder;
}
template <RemainderMode Mode, bool Checked>
struct RemainderImpl {
template <typename T, typename Arg0, typename Arg1>
static enable_if_floating_value<T> Call(KernelContext*, Arg0 left, Arg1 right,
Status* st) {
static_assert(std::is_same_v<T, Arg0> && std::is_same_v<T, Arg1>);
if constexpr (Checked) {
if (ARROW_PREDICT_FALSE(right == 0)) {
*st = Status::Invalid("divide by zero");
return {};
}
}
return FinishRemainder<Mode>(std::fmod(left, right), right);
}
template <typename T, typename Arg0, typename Arg1>
static enable_if_integer_value<T> Call(KernelContext*, Arg0 left, Arg1 right,
Status* st) {
static_assert(std::is_same_v<T, Arg0> && std::is_same_v<T, Arg1>);
T result{};
if (ARROW_PREDICT_FALSE(ModuloWithOverflow(left, right, &result))) {
if (right == 0) {
*st = Status::Invalid("divide by zero");
} else if constexpr (Checked) {
*st = Status::Invalid("overflow");
}
return {};
}
return FinishRemainder<Mode>(result, right);
}
template <typename T, typename Arg0, typename Arg1>
static enable_if_decimal_value<T> Call(KernelContext*, Arg0 left, Arg1 right,
Status* st) {
static_assert(std::is_same_v<T, Arg0> && std::is_same_v<T, Arg1>);
if (ARROW_PREDICT_FALSE(right == 0)) {
*st = Status::Invalid("divide by zero");
return {};
}
return FinishRemainder<Mode>(left % right, right);
}
};
using Remainder = RemainderImpl<RemainderMode::kTruncated, false>;
using RemainderChecked = RemainderImpl<RemainderMode::kTruncated, true>;
using Modulo = RemainderImpl<RemainderMode::kFloored, false>;
using ModuloChecked = RemainderImpl<RemainderMode::kFloored, true>;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #48914:
URL: https://github.com/apache/arrow/pull/48914#discussion_r3908090845
##########
docs/source/cpp/compute.rst:
##########
@@ -566,6 +575,18 @@ Mixed time resolution temporal inputs will be cast to finest input resolution.
intermediate stages of the computation. If either argument is infinite, the
result is ``+Inf`` even if the other argument is NaN.
+* \(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)``.
Review Comment:
Note (4) is attached to both `modulo` and `modulo_checked`, but the text currently says floating-point division by zero returns NaN. In the implementation, `modulo_checked` returns an error on floating-point division by zero (like other *_checked arithmetic kernels), so the docs here are misleading.
This issue also appears on line 584 of the same file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "fangchenli (via GitHub)" <gi...@apache.org>.
fangchenli commented on code in PR #48914:
URL: https://github.com/apache/arrow/pull/48914#discussion_r4066720781
##########
cpp/src/arrow/util/int_util_overflow.h:
##########
@@ -137,6 +137,25 @@ template <typename Int>
return false;
}
+template <typename Int>
+[[nodiscard]] bool ModuloWithOverflowGeneric(Int u, Int v, Int* out) {
Review Comment:
Yes, indirectly: the NON_GENERIC_OPS_WITH_OVERFLOW(ModuloWithOverflow) macro generates wrappers that call it via ##Generic, and RemainderImpl uses those. Same as DivideWithOverflowGeneric.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #48914:
URL: https://github.com/apache/arrow/pull/48914#discussion_r3907906517
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -671,6 +671,40 @@ Result<Datum> Divide(const Datum& left, const Datum& right,
ArithmeticOptions options = ArithmeticOptions(),
ExecContext* ctx = NULLPTR);
+/// \brief Compute the remainder (truncated division) of two values.
+/// 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.
Review Comment:
These API docs say divide-by-zero errors are raised for integer types only, but the decimal kernels also raise on a zero divisor, and floating-point behavior depends on `ArithmeticOptions::check_overflow` (unchecked yields NaN, checked raises). Please document those cases to match actual behavior.
This issue also appears on line 692 of the same file.
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1173,6 +1189,40 @@ const FunctionDoc div_checked_doc{
"integer overflow is encountered."),
{"dividend", "divisor"}};
+const FunctionDoc remainder_doc{
+ "Compute the remainder after integer division (truncated)",
+ ("Returns the remainder after dividing the dividend by the divisor.\n"
+ "The result has the same sign as the dividend (truncated division).\n"
+ "This is equivalent to the C/C++ '%' operator.\n"
+ "Integer division by zero returns an error."),
Review Comment:
`remainder` supports floating-point and decimal inputs too, but the summary and divide-by-zero note only mention integer division. This can mislead users (e.g. floating-point division by zero yields NaN in the unchecked kernel).
This issue also appears in the following locations of the same file:
- line 1201
- line 1214
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -799,8 +799,10 @@ Result<Datum> RoundToMultiple(const Datum& arg, RoundToMultipleOptions options,
SCALAR_ARITHMETIC_BINARY(Add, "add", "add_checked")
SCALAR_ARITHMETIC_BINARY(Divide, "divide", "divide_checked")
SCALAR_ARITHMETIC_BINARY(Logb, "logb", "logb_checked")
+SCALAR_ARITHMETIC_BINARY(Modulo, "modulo", "modulo_checked")
SCALAR_ARITHMETIC_BINARY(Multiply, "multiply", "multiply_checked")
SCALAR_ARITHMETIC_BINARY(Power, "power", "power_checked")
+SCALAR_ARITHMETIC_BINARY(Remainder, "remainder", "remainder_checked")
Review Comment:
The PR description says this includes breaking public API changes and mentions `mod`/`mod_checked`, but the code adds new APIs/kernels named `modulo`/`modulo_checked` (and `remainder*`). Please align the PR description (and any release notes) with the actual API surface, or add the documented aliases if `mod*` is required.
##########
cpp/src/arrow/util/int_util_overflow.h:
##########
@@ -137,6 +137,24 @@ template <typename Int>
return false;
}
+template <typename Int>
+[[nodiscard]] bool ModuloWithOverflowGeneric(Int u, Int v, Int* out) {
+ if (v == 0) {
+ *out = Int{};
+ return true;
+ }
+ // INT_MIN % -1 causes a hardware trap on x86, but mathematically equals 0
Review Comment:
The comment is x86-specific, but `INT_MIN % -1` is undefined behavior in C++ and can trap on other targets as well. Reword to avoid implying this only affects x86.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "tadeja (via GitHub)" <gi...@apache.org>.
tadeja commented on code in PR #48914:
URL: https://github.com/apache/arrow/pull/48914#discussion_r4062289944
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -468,6 +469,172 @@ struct FloatingDivideChecked {
// TODO: Add decimal
};
+// Remainder (truncated): result has same sign as dividend (C/C++ semantics)
+struct Remainder {
Review Comment:
@fangchenli do you perhaps have the chance to continue and look into deduplicating (per @rok's sketch above) and add other nits? Or let me know if I may try to give it a go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #48914:
URL: https://github.com/apache/arrow/pull/48914#issuecomment-5838509604
@cyb70289 could you take a look at this proposal please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "tadeja (via GitHub)" <gi...@apache.org>.
tadeja commented on PR #48914:
URL: https://github.com/apache/arrow/pull/48914#issuecomment-5527225422
@fangchenli Could you also add the four new functions to the Python API autosummary to document the automatically exposed `pyarrow.compute` wrappers?
```diff
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #48914:
URL: https://github.com/apache/arrow/pull/48914#discussion_r4066315386
##########
cpp/src/arrow/util/int_util_overflow.h:
##########
@@ -137,6 +137,25 @@ template <typename Int>
return false;
}
+template <typename Int>
+[[nodiscard]] bool ModuloWithOverflowGeneric(Int u, Int v, Int* out) {
Review Comment:
Is this used anywhere?
##########
cpp/src/arrow/util/int_util_overflow.h:
##########
@@ -137,6 +137,25 @@ template <typename Int>
return false;
}
+template <typename Int>
+[[nodiscard]] bool ModuloWithOverflowGeneric(Int u, Int v, Int* out) {
Review Comment:
Is this used anywhere? @fangchenli
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "tadeja (via GitHub)" <gi...@apache.org>.
tadeja commented on code in PR #48914:
URL: https://github.com/apache/arrow/pull/48914#discussion_r3913444610
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -938,6 +938,173 @@ TYPED_TEST(TestBinaryArithmeticSigned, DivideOverflowRaises) {
this->AssertBinop(Divide, MakeArray(min), MakeArray(-1), "[0]");
}
+// ============== REMAINDER (Truncated) Tests ==============
+
+TYPED_TEST(TestBinaryArithmeticIntegral, Remainder) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ // Empty arrays
+ this->AssertBinop(Remainder, "[]", "[]", "[]");
+ // Basic positive cases
+ this->AssertBinop(Remainder, "[7, 10, 20]", "[3, 4, 7]", "[1, 2, 6]");
+ // Array with nulls
+ this->AssertBinop(Remainder, "[null, 10, 30, null, 20]", "[1, 4, 2, 5, 10]",
+ "[null, 2, 0, null, 0]");
+ // Scalar % Array
+ this->AssertBinop(Remainder, 33, "[null, 1, 3, null, 2]", "[null, 0, 0, null, 1]");
+ // Array % Scalar
+ this->AssertBinop(Remainder, "[null, 10, 30, null, 2]", 3, "[null, 1, 0, null, 2]");
+ // Scalar % Scalar
+ this->AssertBinop(Remainder, 16, 7, 2);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, Remainder) {
+ // Truncated semantics: sign follows dividend
+ this->AssertBinop(Remainder, "[7]", "[3]", "[1]");
+ this->AssertBinop(Remainder, "[-7]", "[3]", "[-1]");
+ this->AssertBinop(Remainder, "[7]", "[-3]", "[1]");
+ this->AssertBinop(Remainder, "[-7]", "[-3]", "[-1]");
+ // Mixed array
+ this->AssertBinop(Remainder, "[-3, 2, -7, 10]", "[1, 1, 2, 3]", "[0, 0, -1, 1]");
+}
+
+TYPED_TEST(TestBinaryArithmeticUnsigned, Remainder) {
+ this->AssertBinop(Remainder, "[7, 100, 255]", "[3, 30, 16]", "[1, 10, 15]");
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, RemainderOverflow) {
+ using CType = typename TestFixture::CType;
+ auto min = std::numeric_limits<CType>::lowest();
+
+ // Unchecked: returns 0 (the mathematically correct result)
+ this->SetOverflowCheck(false);
+ this->AssertBinop(Remainder, MakeArray(min), MakeArray(CType(-1)), "[0]");
+
+ // Checked: raises overflow error
+ this->SetOverflowCheck(true);
+ this->AssertBinopRaises(Remainder, MakeArray(min), MakeArray(CType(-1)), "overflow");
+}
+
+TYPED_TEST(TestBinaryArithmeticIntegral, RemainderByZero) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinopRaises(Remainder, "[3, 2, 6]", "[1, 1, 0]", "divide by zero");
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticFloating, Remainder) {
+ SKIP_IF_HALF_FLOAT();
+
+ this->SetNansEqual(true);
+
+ // Basic cases
+ this->AssertBinop(Remainder, "[7.5, 10.0]", "[2.5, 3.0]", "[0.0, 1.0]");
+ // Negative numbers - truncated semantics: sign follows dividend
+ this->AssertBinop(Remainder, "[-7.5]", "[2.5]", "[-0.0]");
+ this->AssertBinop(Remainder, "[7.5]", "[-2.5]", "[0.0]");
+ this->AssertBinop(Remainder, "[-7.5]", "[-2.5]", "[-0.0]");
+
+ // Division by zero returns NaN (unchecked)
+ this->SetOverflowCheck(false);
+ this->AssertBinop(Remainder, "[1.0]", "[0.0]", "[NaN]");
+
+ // Division by zero raises error (checked)
+ this->SetOverflowCheck(true);
+ this->AssertBinopRaises(Remainder, "[1.0]", "[0.0]", "divide by zero");
+
+ // Infinity edge cases (unchecked)
+ this->SetOverflowCheck(false);
+ this->AssertBinop(Remainder, "[Inf]", "[2.0]", "[NaN]");
+ this->AssertBinop(Remainder, "[-Inf]", "[2.0]", "[NaN]");
+ this->AssertBinop(Remainder, "[2.0]", "[Inf]", "[2.0]");
+ this->AssertBinop(Remainder, "[2.0]", "[-Inf]", "[2.0]");
+ this->AssertBinop(Remainder, "[Inf]", "[Inf]", "[NaN]");
+}
+
+// ============== MOD (Floored) Tests ==============
Review Comment:
ultra nitpick: MOD -> MODULO
##########
docs/source/cpp/compute.rst:
##########
@@ -566,6 +575,18 @@ Mixed time resolution temporal inputs will be cast to finest input resolution.
intermediate stages of the computation. If either argument is infinite, the
result is ``+Inf`` even if the other argument is NaN.
+* \(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)``.
+
+* \(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
Review Comment:
... similarly:
```rst
For floating-point inputs, it returns ``NaN`` in ``remainder`` and an error in ``remainder_checked``.
```
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -938,6 +938,173 @@ TYPED_TEST(TestBinaryArithmeticSigned, DivideOverflowRaises) {
this->AssertBinop(Divide, MakeArray(min), MakeArray(-1), "[0]");
}
+// ============== REMAINDER (Truncated) Tests ==============
+
+TYPED_TEST(TestBinaryArithmeticIntegral, Remainder) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ // Empty arrays
+ this->AssertBinop(Remainder, "[]", "[]", "[]");
+ // Basic positive cases
+ this->AssertBinop(Remainder, "[7, 10, 20]", "[3, 4, 7]", "[1, 2, 6]");
+ // Array with nulls
+ this->AssertBinop(Remainder, "[null, 10, 30, null, 20]", "[1, 4, 2, 5, 10]",
+ "[null, 2, 0, null, 0]");
+ // Scalar % Array
+ this->AssertBinop(Remainder, 33, "[null, 1, 3, null, 2]", "[null, 0, 0, null, 1]");
+ // Array % Scalar
+ this->AssertBinop(Remainder, "[null, 10, 30, null, 2]", 3, "[null, 1, 0, null, 2]");
+ // Scalar % Scalar
+ this->AssertBinop(Remainder, 16, 7, 2);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, Remainder) {
+ // Truncated semantics: sign follows dividend
+ this->AssertBinop(Remainder, "[7]", "[3]", "[1]");
+ this->AssertBinop(Remainder, "[-7]", "[3]", "[-1]");
+ this->AssertBinop(Remainder, "[7]", "[-3]", "[1]");
+ this->AssertBinop(Remainder, "[-7]", "[-3]", "[-1]");
+ // Mixed array
+ this->AssertBinop(Remainder, "[-3, 2, -7, 10]", "[1, 1, 2, 3]", "[0, 0, -1, 1]");
+}
+
+TYPED_TEST(TestBinaryArithmeticUnsigned, Remainder) {
+ this->AssertBinop(Remainder, "[7, 100, 255]", "[3, 30, 16]", "[1, 10, 15]");
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, RemainderOverflow) {
+ using CType = typename TestFixture::CType;
+ auto min = std::numeric_limits<CType>::lowest();
+
+ // Unchecked: returns 0 (the mathematically correct result)
+ this->SetOverflowCheck(false);
+ this->AssertBinop(Remainder, MakeArray(min), MakeArray(CType(-1)), "[0]");
+
+ // Checked: raises overflow error
+ this->SetOverflowCheck(true);
+ this->AssertBinopRaises(Remainder, MakeArray(min), MakeArray(CType(-1)), "overflow");
+}
+
+TYPED_TEST(TestBinaryArithmeticIntegral, RemainderByZero) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinopRaises(Remainder, "[3, 2, 6]", "[1, 1, 0]", "divide by zero");
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticFloating, Remainder) {
+ SKIP_IF_HALF_FLOAT();
+
+ this->SetNansEqual(true);
+
+ // Basic cases
+ this->AssertBinop(Remainder, "[7.5, 10.0]", "[2.5, 3.0]", "[0.0, 1.0]");
+ // Negative numbers - truncated semantics: sign follows dividend
+ this->AssertBinop(Remainder, "[-7.5]", "[2.5]", "[-0.0]");
+ this->AssertBinop(Remainder, "[7.5]", "[-2.5]", "[0.0]");
+ this->AssertBinop(Remainder, "[-7.5]", "[-2.5]", "[-0.0]");
+
+ // Division by zero returns NaN (unchecked)
+ this->SetOverflowCheck(false);
+ this->AssertBinop(Remainder, "[1.0]", "[0.0]", "[NaN]");
+
+ // Division by zero raises error (checked)
+ this->SetOverflowCheck(true);
+ this->AssertBinopRaises(Remainder, "[1.0]", "[0.0]", "divide by zero");
+
+ // Infinity edge cases (unchecked)
+ this->SetOverflowCheck(false);
+ this->AssertBinop(Remainder, "[Inf]", "[2.0]", "[NaN]");
+ this->AssertBinop(Remainder, "[-Inf]", "[2.0]", "[NaN]");
+ this->AssertBinop(Remainder, "[2.0]", "[Inf]", "[2.0]");
+ this->AssertBinop(Remainder, "[2.0]", "[-Inf]", "[2.0]");
+ this->AssertBinop(Remainder, "[Inf]", "[Inf]", "[NaN]");
+}
+
+// ============== MOD (Floored) Tests ==============
+
+TYPED_TEST(TestBinaryArithmeticIntegral, Modulo) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ // Empty arrays
+ this->AssertBinop(Modulo, "[]", "[]", "[]");
+ // Basic positive cases (same as remainder for positive numbers)
+ this->AssertBinop(Modulo, "[7, 10, 20]", "[3, 4, 7]", "[1, 2, 6]");
+ // Array with nulls
+ this->AssertBinop(Modulo, "[null, 10, 30, null, 20]", "[1, 4, 2, 5, 10]",
+ "[null, 2, 0, null, 0]");
+ // Scalar % Array
+ this->AssertBinop(Modulo, 33, "[null, 1, 3, null, 2]", "[null, 0, 0, null, 1]");
+ // Array % Scalar
+ this->AssertBinop(Modulo, "[null, 10, 30, null, 2]", 3, "[null, 1, 0, null, 2]");
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, Modulo) {
+ // Floored semantics: sign follows divisor
+ this->AssertBinop(Modulo, "[7]", "[3]", "[1]");
+ this->AssertBinop(Modulo, "[-7]", "[3]", "[2]");
+ this->AssertBinop(Modulo, "[7]", "[-3]", "[-2]");
+ this->AssertBinop(Modulo, "[-7]", "[-3]", "[-1]");
+ // Edge case: -1 mod positive
Review Comment:
another nitpick: modulo (or "negative dividend and positive divisor")
##########
docs/source/cpp/compute.rst:
##########
@@ -566,6 +575,18 @@ Mixed time resolution temporal inputs will be cast to finest input resolution.
intermediate stages of the computation. If either argument is infinite, the
result is ``+Inf`` even if the other argument is NaN.
+* \(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)``.
Review Comment:
Perhaps a change like:
```rst
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``.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #48914:
URL: https://github.com/apache/arrow/pull/48914#issuecomment-5898438391
@pitrou - I think this is ready to merge. Any comments before we do?
We'd want to add a kernel that returns quotient and remainder in one, but that can be done as a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "tadeja (via GitHub)" <gi...@apache.org>.
tadeja commented on PR #48914:
URL: https://github.com/apache/arrow/pull/48914#issuecomment-5530816452
Ah! it would be beneficial to add a Python test, perhaps like this
(thanks, @rok, for the reminder)
```diff
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):
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #48914:
URL: https://github.com/apache/arrow/pull/48914#discussion_r3907965441
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -671,6 +671,40 @@ Result<Datum> Divide(const Datum& left, const Datum& right,
ArithmeticOptions options = ArithmeticOptions(),
ExecContext* ctx = NULLPTR);
+/// \brief Compute the remainder (truncated division) of two values.
+/// 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.
+///
+/// The result has the same sign as the dividend (C/C++ semantics).
+///
+/// \param[in] left the dividend
+/// \param[in] right the divisor
+/// \param[in] options arithmetic options (enable/disable overflow checking), optional
+/// \param[in] ctx the function execution context, optional
+/// \return the elementwise remainder
+ARROW_EXPORT
+Result<Datum> Remainder(const Datum& left, const Datum& right,
+ ArithmeticOptions options = ArithmeticOptions(),
+ ExecContext* ctx = NULLPTR);
+
+/// \brief Compute the modulo (floored division) of two values.
+/// 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.
Review Comment:
This comment says divide-by-zero errors apply only to integer types, but `modulo` also raises `Invalid` on zero divisor for decimal inputs. Update the API doc to include decimals (and avoid implying floats/decimals behave like integers here).
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1173,6 +1189,40 @@ const FunctionDoc div_checked_doc{
"integer overflow is encountered."),
{"dividend", "divisor"}};
+const FunctionDoc remainder_doc{
+ "Compute the remainder after integer division (truncated)",
+ ("Returns the remainder after dividing the dividend by the divisor.\n"
+ "The result has the same sign as the dividend (truncated division).\n"
+ "This is equivalent to the C/C++ '%' operator.\n"
+ "Integer division by zero returns an error."),
+ {"dividend", "divisor"}};
+
+const FunctionDoc remainder_checked_doc{
+ "Compute the remainder after integer division (truncated)",
+ ("Returns the remainder after dividing the dividend by the divisor.\n"
+ "The result has the same sign as the dividend (truncated division).\n"
+ "This is equivalent to the C/C++ '%' operator.\n"
+ "An error is returned when trying to divide by zero, or when\n"
+ "integer overflow is encountered."),
Review Comment:
These docs describe `remainder` as operating on “integer division” and suggest divide-by-zero errors only for integer inputs, but the kernel also supports floating-point and decimal inputs (floats yield NaN on div-by-zero in the unchecked variant; decimals error). Please adjust wording to match actual behavior, and apply the same wording to `remainder_checked`.
This issue also appears on line 1210 of the same file.
##########
docs/source/cpp/compute.rst:
##########
@@ -514,6 +515,10 @@ Mixed time resolution temporal inputs will be cast to finest input resolution.
+------------------+--------+-------------------------+-------------------------------+-------+
| hypot | Binary | Numeric | Float32/Float64 | \(3) |
+------------------+--------+-------------------------+-------------------------------+-------+
+| modulo | Binary | Numeric | Numeric | \(4) |
++------------------+--------+-------------------------+-------------------------------+-------+
+| modulo_checked | Binary | Numeric | Numeric | \(4) |
++------------------+--------+-------------------------+-------------------------------+-------+
Review Comment:
The PR description says it adds kernels `mod` and `mod_checked`, but this documentation (and the C++ registry/API in this PR) uses `modulo` / `modulo_checked`. Please reconcile this (either add `mod` aliases or update the PR description / any external references to the actual names).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #48914:
URL: https://github.com/apache/arrow/pull/48914#issuecomment-5933769219
Thanks for working on this @fangchenli !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and mod kernels [arrow]
Posted by "tadeja (via GitHub)" <gi...@apache.org>.
tadeja commented on code in PR #48914:
URL: https://github.com/apache/arrow/pull/48914#discussion_r3894939482
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -663,7 +663,7 @@ void AddDecimalBinaryKernels(const std::string& name, ScalarFunction* func) {
OutputType out_type(null());
std::shared_ptr<MatchConstraint> constraint = nullptr;
const std::string op = name.substr(0, name.find("_"));
- if (op == "add" || op == "subtract") {
+ if (op == "add" || op == "subtract" || op == "remainder" || op == "mod") {
out_type = OutputType(ResolveDecimalAdditionOrSubtractionOutput);
constraint = DecimalsHaveSameScale();
Review Comment:
```suggestion
if (op == "add" || op == "subtract") {
out_type = OutputType(ResolveDecimalAdditionOrSubtractionOutput);
constraint = DecimalsHaveSameScale();
} else if (op == "remainder" || op == "modulo") {
out_type = OutputType(ResolveDecimalRemainderOutput);
constraint = DecimalsHaveSameScale();
```
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
Review Comment:
```suggestion
Result<TypeHolder> ResolveDecimalRemainderOutput(KernelContext*,
const std::vector<TypeHolder>& types) {
return ResolveDecimalBinaryOperationOutput(
types,
[](int32_t p1, int32_t s1, int32_t p2,
int32_t s2) -> Result<std::pair<int32_t, int32_t>> {
DCHECK_EQ(s1, s2);
return std::make_pair(std::max(p1, p2), s1);
});
}
```
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -663,7 +663,7 @@ void AddDecimalBinaryKernels(const std::string& name, ScalarFunction* func) {
OutputType out_type(null());
std::shared_ptr<MatchConstraint> constraint = nullptr;
const std::string op = name.substr(0, name.find("_"));
- if (op == "add" || op == "subtract") {
+ if (op == "add" || op == "subtract" || op == "remainder" || op == "mod") {
out_type = OutputType(ResolveDecimalAdditionOrSubtractionOutput);
constraint = DecimalsHaveSameScale();
Review Comment:
(suggestion for point 2))
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
Review Comment:
(suggestion for point 2))
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #48914:
URL: https://github.com/apache/arrow/pull/48914#discussion_r3908011101
##########
cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc:
##########
@@ -938,6 +938,173 @@ TYPED_TEST(TestBinaryArithmeticSigned, DivideOverflowRaises) {
this->AssertBinop(Divide, MakeArray(min), MakeArray(-1), "[0]");
}
+// ============== REMAINDER (Truncated) Tests ==============
+
+TYPED_TEST(TestBinaryArithmeticIntegral, Remainder) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ // Empty arrays
+ this->AssertBinop(Remainder, "[]", "[]", "[]");
+ // Basic positive cases
+ this->AssertBinop(Remainder, "[7, 10, 20]", "[3, 4, 7]", "[1, 2, 6]");
+ // Array with nulls
+ this->AssertBinop(Remainder, "[null, 10, 30, null, 20]", "[1, 4, 2, 5, 10]",
+ "[null, 2, 0, null, 0]");
+ // Scalar % Array
+ this->AssertBinop(Remainder, 33, "[null, 1, 3, null, 2]", "[null, 0, 0, null, 1]");
+ // Array % Scalar
+ this->AssertBinop(Remainder, "[null, 10, 30, null, 2]", 3, "[null, 1, 0, null, 2]");
+ // Scalar % Scalar
+ this->AssertBinop(Remainder, 16, 7, 2);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, Remainder) {
+ // Truncated semantics: sign follows dividend
+ this->AssertBinop(Remainder, "[7]", "[3]", "[1]");
+ this->AssertBinop(Remainder, "[-7]", "[3]", "[-1]");
+ this->AssertBinop(Remainder, "[7]", "[-3]", "[1]");
+ this->AssertBinop(Remainder, "[-7]", "[-3]", "[-1]");
+ // Mixed array
+ this->AssertBinop(Remainder, "[-3, 2, -7, 10]", "[1, 1, 2, 3]", "[0, 0, -1, 1]");
+}
+
+TYPED_TEST(TestBinaryArithmeticUnsigned, Remainder) {
+ this->AssertBinop(Remainder, "[7, 100, 255]", "[3, 30, 16]", "[1, 10, 15]");
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, RemainderOverflow) {
+ using CType = typename TestFixture::CType;
+ auto min = std::numeric_limits<CType>::lowest();
+
+ // Unchecked: returns 0 (the mathematically correct result)
+ this->SetOverflowCheck(false);
+ this->AssertBinop(Remainder, MakeArray(min), MakeArray(CType(-1)), "[0]");
+
+ // Checked: raises overflow error
+ this->SetOverflowCheck(true);
+ this->AssertBinopRaises(Remainder, MakeArray(min), MakeArray(CType(-1)), "overflow");
+}
+
+TYPED_TEST(TestBinaryArithmeticIntegral, RemainderByZero) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinopRaises(Remainder, "[3, 2, 6]", "[1, 1, 0]", "divide by zero");
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticFloating, Remainder) {
+ SKIP_IF_HALF_FLOAT();
+
+ this->SetNansEqual(true);
+
+ // Basic cases
+ this->AssertBinop(Remainder, "[7.5, 10.0]", "[2.5, 3.0]", "[0.0, 1.0]");
+ // Negative numbers - truncated semantics: sign follows dividend
+ this->AssertBinop(Remainder, "[-7.5]", "[2.5]", "[-0.0]");
+ this->AssertBinop(Remainder, "[7.5]", "[-2.5]", "[0.0]");
+ this->AssertBinop(Remainder, "[-7.5]", "[-2.5]", "[-0.0]");
+
+ // Division by zero returns NaN (unchecked)
+ this->SetOverflowCheck(false);
+ this->AssertBinop(Remainder, "[1.0]", "[0.0]", "[NaN]");
+
+ // Division by zero raises error (checked)
+ this->SetOverflowCheck(true);
+ this->AssertBinopRaises(Remainder, "[1.0]", "[0.0]", "divide by zero");
+
+ // Infinity edge cases (unchecked)
+ this->SetOverflowCheck(false);
+ this->AssertBinop(Remainder, "[Inf]", "[2.0]", "[NaN]");
+ this->AssertBinop(Remainder, "[-Inf]", "[2.0]", "[NaN]");
+ this->AssertBinop(Remainder, "[2.0]", "[Inf]", "[2.0]");
+ this->AssertBinop(Remainder, "[2.0]", "[-Inf]", "[2.0]");
+ this->AssertBinop(Remainder, "[Inf]", "[Inf]", "[NaN]");
+}
+
+// ============== MOD (Floored) Tests ==============
+
+TYPED_TEST(TestBinaryArithmeticIntegral, Modulo) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ // Empty arrays
+ this->AssertBinop(Modulo, "[]", "[]", "[]");
+ // Basic positive cases (same as remainder for positive numbers)
+ this->AssertBinop(Modulo, "[7, 10, 20]", "[3, 4, 7]", "[1, 2, 6]");
+ // Array with nulls
+ this->AssertBinop(Modulo, "[null, 10, 30, null, 20]", "[1, 4, 2, 5, 10]",
+ "[null, 2, 0, null, 0]");
+ // Scalar % Array
+ this->AssertBinop(Modulo, 33, "[null, 1, 3, null, 2]", "[null, 0, 0, null, 1]");
+ // Array % Scalar
+ this->AssertBinop(Modulo, "[null, 10, 30, null, 2]", 3, "[null, 1, 0, null, 2]");
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, Modulo) {
+ // Floored semantics: sign follows divisor
+ this->AssertBinop(Modulo, "[7]", "[3]", "[1]");
+ this->AssertBinop(Modulo, "[-7]", "[3]", "[2]");
+ this->AssertBinop(Modulo, "[7]", "[-3]", "[-2]");
+ this->AssertBinop(Modulo, "[-7]", "[-3]", "[-1]");
+ // Edge case: -1 mod positive
+ this->AssertBinop(Modulo, "[-1]", "[3]", "[2]");
+}
+
+TYPED_TEST(TestBinaryArithmeticUnsigned, Modulo) {
+ // Same as remainder for unsigned (no negative numbers)
+ this->AssertBinop(Modulo, "[7, 100, 255]", "[3, 30, 16]", "[1, 10, 15]");
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, ModOverflow) {
Review Comment:
Test name `ModOverflow` is inconsistent with the operation name (`Modulo`) and the matching remainder test (`RemainderOverflow`), which makes test output/search less clear. Consider renaming it to `ModuloOverflow`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #48914:
URL: https://github.com/apache/arrow/pull/48914#discussion_r4066770506
##########
cpp/src/arrow/util/int_util_overflow.h:
##########
@@ -137,6 +137,25 @@ template <typename Int>
return false;
}
+template <typename Int>
+[[nodiscard]] bool ModuloWithOverflowGeneric(Int u, Int v, Int* out) {
Review Comment:
Ah sorry, I wasn't aware how that macro worked. Thanks for the explanation and I'll do another pass tomorrow!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46901: [C++][Compute] Add remainder and modulo kernels [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok merged PR #48914:
URL: https://github.com/apache/arrow/pull/48914
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org