[GitHub] [arrow] westonpace commented on a diff in pull request #14434: ARROW-15538: [C++] Expanding coverage of math functions from Substrait to Acero

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

westonpace commented on code in PR #14434:
URL: https://github.com/apache/arrow/pull/14434#discussion_r997559329


##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -485,6 +485,34 @@ struct NegateChecked {
   }
 };
 
+struct Exp {
+  template <typename T, typename Arg>
+  static enable_if_floating_value<Arg, T> Call(KernelContext*, Arg exp, Status*) {
+    static_assert(std::is_same<T, Arg>::value, "");
+    return std::exp(exp);
+  }
+
+  template <typename T, typename Arg>
+  static enable_if_decimal_value<Arg, T> Call(KernelContext*, Arg exp, Status*) {
+    static_assert(std::is_same<T, Arg>::value, "");
+    return std::exp(exp);
+  }
+};
+
+struct ExpChecked {

Review Comment:
   I'm not sure there is any value in creating a checked variant that does the same thing as the unchecked variant.



##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -2224,6 +2230,15 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
           "power_checked", pow_checked_doc);
   DCHECK_OK(registry->AddFunction(std::move(power_checked)));
 
+  // ----------------------------------------------------------------------
+  auto exp = MakeUnaryArithmeticFunctionFloatingPoint<Exp>("exp", exp_doc);

Review Comment:
   Any new kernel function will need unit tests.



##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1717,6 +1717,12 @@ const FunctionDoc pow_doc{
      "wraps around. If either base or exponent is null the result will be null."),
     {"base", "exponent"}};
 
+const FunctionDoc exp_doc{
+    "Computes Euler's number raised to the specified power, element-wise",

Review Comment:
   ```suggestion
       "Compute Euler's number raised to the specified power, element-wise",
   ```



##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -1717,6 +1717,12 @@ const FunctionDoc pow_doc{
      "wraps around. If either base or exponent is null the result will be null."),
     {"base", "exponent"}};
 
+const FunctionDoc exp_doc{
+    "Computes Euler's number raised to the specified power, element-wise",
+    ("Negative integer power returns an error. However, integer overflow\n"

Review Comment:
   ```suggestion
       ("Negative integer power returns an error. However, integer overflow\n"
   ```
   What does negative integer power mean?  This function only has non-integral kernels.
   
   I'm not sure what you mean by integer overflow.  Why "integer"?  The return value is a floating point.  If there is an overflow then "[+HUGE_VAL](https://en.cppreference.com/w/cpp/numeric/math/HUGE_VAL), +HUGE_VALF, or +HUGE_VALL is returned."



##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -858,11 +858,27 @@ struct DefaultExtensionIdRegistry : ExtensionIdRegistryImpl {
 
     // -------------- Substrait -> Arrow Functions -----------------
     // Mappings with a _checked variant
-    for (const auto& function_name : {"add", "subtract", "multiply", "divide"}) {
+    for (const auto& function_name : {"add", "subtract", "multiply", "divide", "sign",
+                                      "power", "sqrt", "exp", "abs"}) {
       DCHECK_OK(
           AddSubstraitCallToArrow({kSubstraitArithmeticFunctionsUri, function_name},
                                   DecodeOptionlessOverflowableArithmetic(function_name)));
     }
+
+    // Mappings for log functions
+    for (const auto& function_name : {"ln", "log10", "log2", "logb", "log1p"}) {
+      DCHECK_OK(
+          AddSubstraitCallToArrow({kSubstraitLogarithmicFunctionsUri, function_name},
+                                  DecodeOptionlessOverflowableArithmetic(function_name)));
+    }
+
+    // Mappings for rounding functions
+    for (const auto& function_name : {"ceil", "floor"}) {
+      DCHECK_OK(
+          AddSubstraitCallToArrow({kSubstraitRoundingFunctionsUri, function_name},
+                                  DecodeOptionlessOverflowableArithmetic(function_name)));

Review Comment:
   Why use "Overflowable" here?  I don't think there is a `ceil_checked` or `floor_checked`?



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