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