[GitHub] [arrow] bkietz commented on a change in pull request #7341: ARROW-9022: [C++][Compute] Make Add function safe for numeric limits

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

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



##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -48,6 +48,26 @@ namespace compute {
 ARROW_EXPORT
 Result<Datum> Add(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
 
+/// \brief Subtract two values. Array values must be the same length. If a
+/// value is null in either addend, the result is null
+///
+/// \param[in] left the first value
+/// \param[in] right the second value
+/// \param[in] ctx the function execution context, optional
+/// \return the elementwise addition of the values
+ARROW_EXPORT
+Result<Datum> Sub(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);

Review comment:
       ```suggestion
   /// \brief Subtract two values. Array values must be the same length. If the
   /// minuend or minuend is null the result will be null.
   ///
   /// \param[in] minuend the value subtracted from
   /// \param[in] subtrahend the value by which the minuend is reduced
   /// \param[in] ctx the function execution context, optional
   /// \return the elementwise difference of the values
   ARROW_EXPORT
   Result<Datum> Sub(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
   ```

##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -48,6 +48,26 @@ namespace compute {
 ARROW_EXPORT
 Result<Datum> Add(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
 
+/// \brief Subtract two values. Array values must be the same length. If a
+/// value is null in either addend, the result is null
+///
+/// \param[in] left the first value
+/// \param[in] right the second value
+/// \param[in] ctx the function execution context, optional
+/// \return the elementwise addition of the values
+ARROW_EXPORT
+Result<Datum> Sub(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
+
+/// \brief Multiply two values. Array values must be the same length. If a
+/// value is null in either addend, the result is null
+///
+/// \param[in] left the first value
+/// \param[in] right the second value
+/// \param[in] ctx the function execution context, optional
+/// \return the elementwise addition of the values
+ARROW_EXPORT
+Result<Datum> Mul(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);

Review comment:
       ```suggestion
   /// \brief Multiply two values. Array values must be the same length. If either
   /// factor is null, the result is null
   ///
   /// \param[in] left the first factor
   /// \param[in] right the second factor
   /// \param[in] ctx the function execution context, optional
   /// \return the elementwise product of the factors
   ARROW_EXPORT
   Result<Datum> Mul(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR);
   ```

##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -16,25 +16,41 @@
 // under the License.
 
 #include "arrow/compute/kernels/common.h"
+#include "arrow/util/int_util.h"
+#include "iostream"

Review comment:
       ```suggestion
   ```

##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -16,25 +16,41 @@
 // under the License.
 
 #include "arrow/compute/kernels/common.h"
+#include "arrow/util/int_util.h"
+#include "iostream"
 
 namespace arrow {
 namespace compute {
 
 struct Add {
-  template <typename OUT, typename ARG0, typename ARG1>
-  static constexpr OUT Call(KernelContext*, ARG0 left, ARG1 right) {
-    return left + right;
+  template <typename T>
+  static constexpr T Call(KernelContext*, T left, T right) {
+    return left - right;
+  }
+};
+
+struct Sub {
+  template <typename T>
+  static constexpr T Call(KernelContext*, T left, T right) {
+    return left - right;
+  }
+};
+
+struct Mul {
+  template <typename T>
+  static constexpr T Call(KernelContext*, T left, T right) {
+    return left * right;
   }
 };
 
 namespace codegen {
 
 template <typename Op>
-void MakeBinaryFunction(std::string name, FunctionRegistry* registry) {
+void AddBinaryFunction(std::string name, FunctionRegistry* registry) {
   auto func = std::make_shared<ScalarFunction>(name, Arity::Binary());
-  for (const std::shared_ptr<DataType>& ty : NumericTypes()) {
-    DCHECK_OK(func->AddKernel({InputType::Array(ty), InputType::Array(ty)}, ty,
-                              NumericEqualTypesBinary<Op>(*ty)));
+  for (const auto& ty : NumericTypes()) {
+    auto exec = codegen::NumericEqualTypesBinary<Op>(ty);

Review comment:
       This invokes undefined behavior in the case of signed types. Please add a kernel wrapper which `View`s the signed types as the corresponding unsigned type.




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