bkietz commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1237583858
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -605,6 +606,96 @@ struct Sign {
}
};
+struct Max {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_not_floating_value<T> Call(KernelContext*, Arg0
arg0,
+ Arg1 arg1, Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0,
Arg1>::value);
+ return std::max(arg0, arg1);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_value<T> Call(KernelContext*, Arg0 left,
Arg1 right,
+ Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0,
Arg1>::value);
+ if (std::isnan(left)) {
+ return right;
+ } else if (std::isnan(right)) {
+ return left;
+ } else {
+ return std::max(left, right);
+ }
+ }
+
+ template <typename T>
+ static constexpr enable_if_decimal_value<T, T> Identity() {
+ return T::GetMinSentinel();
+ }
Review Comment:
I think this isn't used anymore (also in Min).
```suggestion
```
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -605,6 +606,96 @@ struct Sign {
}
};
+struct Max {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_not_floating_value<T> Call(KernelContext*, Arg0
arg0,
+ Arg1 arg1, Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0,
Arg1>::value);
+ return std::max(arg0, arg1);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_value<T> Call(KernelContext*, Arg0 left,
Arg1 right,
+ Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0,
Arg1>::value);
+ if (std::isnan(left)) {
+ return right;
+ } else if (std::isnan(right)) {
+ return left;
+ } else {
+ return std::max(left, right);
+ }
+ }
+
+ template <typename T>
+ static constexpr enable_if_decimal_value<T, T> Identity() {
+ return T::GetMinSentinel();
+ }
+};
+
+struct Min {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_not_floating_value<T> Call(KernelContext*, Arg0
arg0,
+ Arg1 arg1, Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0,
Arg1>::value);
+ return std::min(arg0, arg1);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_value<T> Call(KernelContext*, Arg0 left,
Arg1 right,
+ Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0,
Arg1>::value);
+ if (std::isnan(left)) {
+ return right;
+ } else if (std::isnan(right)) {
+ return left;
+ } else {
+ return std::min(left, right);
+ }
+ }
+
+ template <typename T>
+ static constexpr enable_if_decimal_value<T, T> Identity() {
+ return T::GetMaxSentinel();
+ }
+};
+
+/// The term identity is from the mathematical notation monoid.
+/// For any associative binary operation, identity is defined as:
+/// Op(identity, x) = x for all x.
+template <typename Op>
+struct Identity;
+
+template <>
+struct Identity<Add> {
+ template <typename Value>
+ static constexpr Value value{0};
+};
+
+template <>
+struct Identity<AddChecked> : Identity<Add> {};
+
+template <>
+struct Identity<Multiply> {
+ template <typename Value>
+ static constexpr Value value{1};
+};
+
+template <>
+struct Identity<MultiplyChecked> : Identity<Multiply> {};
+
+template <>
+struct Identity<Max> {
+ template <typename Value>
+ static constexpr Value value{std::numeric_limits<Value>::min()};
Review Comment:
These cumulative ops aren't currently instantiated for decimal types anyway:
https://github.com/apache/arrow/blob/44edc27/cpp/src/arrow/compute/kernels/codegen_internal.h#L1070-L1100
... if you feel like filing a follow up issue for them, it should be fairly
straightforward to specify these as specializations:
```suggestion
template <typename Value>
static constexpr Value value{std::numeric_limits<Value>::min()};
template <>
static constexpr Decimal128 value<Decimal128> =
Decimal128::GetMinSentinel();
```
Multiplication will require more effort since we can't construct `1` without
knowing the scale.
##########
python/pyarrow/_compute.pyx:
##########
@@ -1928,31 +1928,41 @@ class PartitionNthOptions(_PartitionNthOptions):
self._set_options(pivot, null_placement)
-cdef class _CumulativeSumOptions(FunctionOptions):
+cdef class _CumulativeOptions(FunctionOptions):
def _set_options(self, start, skip_nulls):
- if not isinstance(start, Scalar):
+ if start is None:
+ self.wrapped.reset(new CCumulativeOptions(skip_nulls))
+ elif isinstance(start, Scalar):
+ self.wrapped.reset(new CCumulativeOptions(
+ pyarrow_unwrap_scalar(start), skip_nulls))
+ else:
try:
start = lib.scalar(start)
+ self.wrapped.reset(new CCumulativeOptions(
+ pyarrow_unwrap_scalar(start), skip_nulls))
except Exception:
_raise_invalid_function_option(
start, "`start` type for CumulativeSumOptions", TypeError)
- self.wrapped.reset(new CCumulativeSumOptions((<Scalar>
start).unwrap(), skip_nulls))
-
-class CumulativeSumOptions(_CumulativeSumOptions):
+class CumulativeOptions(_CumulativeOptions):
"""
- Options for `cumulative_sum` function.
+ Options for `cumulative_*` functions.
+
+ - cumulative_sum
+ - cumulative_sum_checked
+ ...
Review Comment:
Sorry, I didn't make it clear enough that my suggestion was not complete.
Please list the rest of the cumulative functions here
--
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]
