[GitHub] [arrow] bkietz commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels

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

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



##########
File path: cpp/src/arrow/compute/kernels/codegen_internal.h
##########
@@ -675,11 +704,80 @@ struct ScalarBinary {
   }
 };
 
+// An alternative to ScalarBinary that Applies a scalar operation on only the
+// not-null values of arrays.
+template <typename OutType, typename Arg0Type, typename Arg1Type, typename Op>
+struct ScalarBinaryNotNull {
+  using OUT = typename GetOutputType<OutType>::T;
+
+  static void ArrayArray(KernelContext* ctx, const ArrayData& arg0, const ArrayData& arg1,
+                         Datum* out) {
+    ArrayIterator<Arg0Type> arg0_it(arg0);
+    ArrayIterator<Arg1Type> arg1_it(arg1);
+    OutputAdapter<OutType>::WriteOnlyValid(
+        ctx, out, [&]() -> OUT { return Op::template Call(ctx, arg0_it(), arg1_it()); },
+        [&]() {
+          ARROW_UNUSED(arg0_it());
+          ARROW_UNUSED(arg1_it());
+        });
+  }
+
+  static void ArrayScalar(KernelContext* ctx, const ArrayData& arg0, const Scalar& arg1,
+                          Datum* out) {
+    ArrayIterator<Arg0Type> arg0_it(arg0);
+    auto arg1_val = UnboxScalar<Arg1Type>::Unbox(arg1);
+    OutputAdapter<OutType>::WriteOnlyValid(
+        ctx, out, [&]() -> OUT { return Op::template Call(ctx, arg0_it(), arg1_val); },
+        [&]() { ARROW_UNUSED(arg0_it()); });
+  }
+
+  static void ScalarArray(KernelContext* ctx, const Scalar& arg0, const ArrayData& arg1,
+                          Datum* out) {
+    auto arg0_val = UnboxScalar<Arg0Type>::Unbox(arg0);
+    ArrayIterator<Arg1Type> arg1_it(arg1);
+    OutputAdapter<OutType>::WriteOnlyValid(
+        ctx, out, [&]() -> OUT { return Op::template Call(ctx, arg0_val, arg1_it()); },
+        [&]() { ARROW_UNUSED(arg1_it()); });
+  }
+
+  static void ScalarScalar(KernelContext* ctx, const Scalar& arg0, const Scalar& arg1,
+                           Datum* out) {
+    if (out->scalar()->is_valid) {
+      auto arg0_val = UnboxScalar<Arg0Type>::Unbox(arg0);
+      auto arg1_val = UnboxScalar<Arg1Type>::Unbox(arg1);
+      BoxScalar<OutType>::Box(Op::template Call(ctx, arg0_val, arg1_val),
+                              out->scalar().get());
+    }
+  }
+
+  static void Exec(KernelContext* ctx, const ExecBatch& batch, Datum* out) {
+    if (batch[0].kind() == Datum::ARRAY) {
+      if (batch[1].kind() == Datum::ARRAY) {
+        return ArrayArray(ctx, *batch[0].array(), *batch[1].array(), out);
+      } else {
+        return ArrayScalar(ctx, *batch[0].array(), *batch[1].scalar(), out);
+      }
+    } else {
+      if (batch[1].kind() == Datum::ARRAY) {
+        // e.g. if we were doing scalar < array, we flip and do array >= scalar

Review comment:
       I think this comment is not applicable
   ```suggestion
   ```

##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -130,6 +130,19 @@ Result<Datum> Multiply(const Datum& left, const Datum& right,
                        ArithmeticOptions options = ArithmeticOptions(),
                        ExecContext* ctx = NULLPTR);
 
+/// \brief Divide two values. Array values must be the same length. If either
+/// factor is null the result will be null.

Review comment:
       Please describe division-by-zero behavior

##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -219,48 +219,77 @@ struct MultiplyChecked {
   }
 };
 
+struct Divide {
+  template <typename T>
+  static constexpr enable_if_floating_point<T> Call(KernelContext*, T left, T right) {
+    return left / right;
+  }
+
+  template <typename T>
+  static constexpr enable_if_integer<T> Call(KernelContext*, T left, T right) {
+    return left / right;
+  }
+};
+
+struct DivideChecked {
+  template <typename T>
+  static enable_if_integer<T> Call(KernelContext* ctx, T left, T right) {
+    if (right == 0) {
+      ctx->SetStatus(Status::Invalid("divide by 0"));
+      return right;
+    }
+    return Divide::Call(ctx, left, right);
+  }
+
+  template <typename T>
+  static constexpr enable_if_floating_point<T> Call(KernelContext*, T left, T right) {
+    return left * right;

Review comment:
       ```suggestion
       return left / right;
   ```

##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -130,6 +130,19 @@ Result<Datum> Multiply(const Datum& left, const Datum& right,
                        ArithmeticOptions options = ArithmeticOptions(),
                        ExecContext* ctx = NULLPTR);
 
+/// \brief Divide two values. Array values must be the same length. If either
+/// factor is null the result will be null.
+///
+/// \param[in] left the dividend
+/// \param[in] right the divisor
+/// \param[in] options arithmetic options (overflow handling), optional

Review comment:
       Division does not overflow unless the dividend is INT_MIN and the divisor is -1; IIUC `overflow_handling` is being used to indicate whether divide-by-zero should raise an error. If so please amend the comment




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