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

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

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



##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -1241,6 +1373,45 @@ std::shared_ptr<ScalarFunction> MakeUnaryArithmeticFunctionNotNull(
   return func;
 }
 
+// Like MakeUnaryArithmeticFunction, but for unary rounding functions that control
+// kernel dispatch based on RoundMode, only on non-null output.
+template <template <RoundMode RndMode> class Op, typename Opts>
+std::shared_ptr<ScalarFunction> MakeUnaryRoundFunction(
+    std::string name, const FunctionDoc* doc,
+    const FunctionOptions* default_options = NULLPTR, KernelInit init = NULLPTR) {
+  auto func = std::make_shared<ArithmeticFloatingPointFunction>(name, Arity::Unary(), doc,
+                                                                default_options);
+  for (const auto& ty : FloatingPointTypes()) {
+    // Order of ArrayKernelExec w.r.t. RoundMode needs to follow the order of
+    // values in RoundMode definition (see api_scalar.h) because they are used as
+    // indexing values.
+    std::vector<ArrayKernelExec> execs = {
+        GenerateArithmeticFloatingPoint<ScalarUnary, Op<RoundMode::TOWARDS_NEG_INFINITY>>(
+            ty),
+        GenerateArithmeticFloatingPoint<ScalarUnary, Op<RoundMode::TOWARDS_POS_INFINITY>>(
+            ty),
+        GenerateArithmeticFloatingPoint<ScalarUnary, Op<RoundMode::TOWARDS_ZERO>>(ty),
+        GenerateArithmeticFloatingPoint<ScalarUnary, Op<RoundMode::TOWARDS_INFINITY>>(ty),
+        GenerateArithmeticFloatingPoint<ScalarUnary, Op<RoundMode::HALF_NEG_INFINITY>>(
+            ty),
+        GenerateArithmeticFloatingPoint<ScalarUnary, Op<RoundMode::HALF_POS_INFINITY>>(
+            ty),
+        GenerateArithmeticFloatingPoint<ScalarUnary, Op<RoundMode::HALF_TOWARDS_ZERO>>(
+            ty),
+        GenerateArithmeticFloatingPoint<ScalarUnary,
+                                        Op<RoundMode::HALF_TOWARDS_INFINITY>>(ty),
+        GenerateArithmeticFloatingPoint<ScalarUnary, Op<RoundMode::HALF_TO_EVEN>>(ty),
+        GenerateArithmeticFloatingPoint<ScalarUnary, Op<RoundMode::HALF_TO_ODD>>(ty),
+    };

Review comment:
       I wonder, given the branch predictor, if a switch-case might perform similarly to generating separate kernels for every type and rounding mode, if you're concerned about having to place the rounding mode in the template parameters.

##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -1606,6 +1774,20 @@ const FunctionDoc trunc_doc{
     ("Calculate the nearest integer not greater in magnitude than to the "
      "argument element-wise."),
     {"x"}};
+
+const FunctionDoc round_doc{
+    "Round the arguments element-wise",

Review comment:
       Maybe "Round arguments to a [decimal place/multiple] element-wise"?

##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -1333,6 +1400,305 @@ TYPED_TEST(TestUnaryArithmeticFloating, AbsoluteValue) {
   }
 }
 
+constexpr std::initializer_list<RoundMode> kRoundModes = {

Review comment:
       Having a list like this is fine. We probably don't want to expose EnumTraits<Foo> (unless you really want to make another internal header/translation unit to hold the definitions).

##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -1606,6 +1774,20 @@ const FunctionDoc trunc_doc{
     ("Calculate the nearest integer not greater in magnitude than to the "
      "argument element-wise."),
     {"x"}};
+
+const FunctionDoc round_doc{
+    "Round the arguments element-wise",
+    ("Options are used to control the rounding mode and number of digits.\n"
+     "Default behavior is to round to nearest integer."),
+    {"x"},
+    "RoundOptions"};
+
+const FunctionDoc mround_doc{

Review comment:
       nit: perhaps RoundToMultiple (and possibly RoundToPlace) might be clearer than round/mround (unless mround is a common name? I see Excel uses it at least)

##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -49,10 +49,52 @@ class ARROW_EXPORT ElementWiseAggregateOptions : public FunctionOptions {
   explicit ElementWiseAggregateOptions(bool skip_nulls = true);
   constexpr static char const kTypeName[] = "ElementWiseAggregateOptions";
   static ElementWiseAggregateOptions Defaults() { return ElementWiseAggregateOptions{}; }
-
   bool skip_nulls;
 };
 
+/// Rounding modes for Round and MRound functions. General modes are prefixed
+/// with TOWARDS and tie-breaker modes are prefixed with HALF. Common aliases
+/// are available for several modes.

Review comment:
       Are the aliases worth keeping? Maybe it's better to just document the various names for them but keep only a single name for each possible case.

##########
File path: docs/source/python/api/compute.rst
##########
@@ -127,6 +129,18 @@ variants which detect domain errors where appropriate.
    tan
    tan_checked
 
+Rounding

Review comment:
       Did you mean to add rounding functions twice in this 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