[GitHub] [arrow] bkietz commented on a change in pull request #10349: ARROW-12744: [C++][Compute] Add rounding kernel

[GitBox <gi...@apache.org>]

bkietz commented on a change in pull request #10349:
URL: https://github.com/apache/arrow/pull/10349#discussion_r633838367



##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -933,22 +933,23 @@ TEST(TestBinaryArithmetic, AddWithImplicitCastsUint64EdgeCase) {
 }
 
 TEST(TestUnaryArithmetic, DispatchBest) {
-  for (std::string name : {"negate"}) {
+  for (std::string name : {"negate", "round", "round_checked"}) {
     for (const auto& ty : {int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),

Review comment:
       It seems more than a little odd to support rounding for integer types. I suppose the noop kernels aren't taking up much space, but I'd really expect that rounding would only support floating point or decimal inputs (and would either raise an error or require an implicit cast for integral inputs)

##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -238,6 +238,17 @@ Result<Datum> Power(const Datum& left, const Datum& right,
                     ArithmeticOptions options = ArithmeticOptions(),
                     ExecContext* ctx = NULLPTR);
 
+/// \brief Round a value. Array values can be of arbitrary length. If argument
+/// is null the result will be null.
+///
+/// \param[in] arg the value rounded
+/// \param[in] options arithmetic options (rounding precision, round-to-nearest method,
+/// overflow handling), optional \param[in] ctx the function execution context, optional
+/// \return the elementwise rounded value
+ARROW_EXPORT
+Result<Datum> Round(const Datum& arg, ArithmeticOptions options = ArithmeticOptions(),

Review comment:
       Round will require separate options including the number of digits which should remain in the rounded output

##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -238,6 +238,17 @@ Result<Datum> Power(const Datum& left, const Datum& right,
                     ArithmeticOptions options = ArithmeticOptions(),
                     ExecContext* ctx = NULLPTR);
 
+/// \brief Round a value. Array values can be of arbitrary length. If argument
+/// is null the result will be null.
+///
+/// \param[in] arg the value rounded
+/// \param[in] options arithmetic options (rounding precision, round-to-nearest method,
+/// overflow handling), optional \param[in] ctx the function execution context, optional
+/// \return the elementwise rounded value
+ARROW_EXPORT
+Result<Datum> Round(const Datum& arg, ArithmeticOptions options = ArithmeticOptions(),

Review comment:
       Since overflow doesn't pertain to rounding, I think it doesn't make sense to provide ArithmeticOptions (which only contains `bool check_overflow` here)

##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -355,6 +355,32 @@ struct PowerChecked {
   }
 };
 
+struct Round {
+  template <typename T, typename Arg>
+  static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg arg, Status*) {
+    return std::round(arg);
+  }
+
+  template <typename T, typename Arg>
+  static constexpr enable_if_integer<T> Call(KernelContext*, Arg arg, Status*) {
+    return arg;
+  }
+};
+
+struct RoundChecked {
+  template <typename T, typename Arg>
+  static constexpr enable_if_integer<T> Call(KernelContext*, Arg arg, Status*) {
+    static_assert(std::is_same<T, Arg>::value, "");
+    return arg;
+  }
+
+  template <typename T, typename Arg>
+  static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg arg, Status*) {
+    static_assert(std::is_same<T, Arg>::value, "");

Review comment:
       It's not worthwhile to have a _checked version of this function if there is no difference in runtime behavior between the two functions.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org