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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
