You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/07/14 07:14:19 UTC
[GitHub] [arrow] liyafan82 opened a new pull request #7748: ARROW-9388: [C++] Division kernels
liyafan82 opened a new pull request #7748:
URL: https://github.com/apache/arrow/pull/7748
See https://issues.apache.org/jira/browse/ARROW-9388
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-658246865
But, yes, I'll take a look later.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471348419
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -492,6 +486,81 @@ TYPED_TEST(TestBinaryArithmeticFloating, Add) {
this->AssertBinop(Add, "[null, 2.0]", this->MakeNullScalar(), "[null, null]");
}
+TYPED_TEST(TestBinaryArithmeticFloating, Div) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinop(Divide, "[]", "[]", "[]");
+
+ this->AssertBinop(Divide, "[3.4, 2.6, 6.3]", "[1, 2, 2]", "[3.4, 1.3, 3.15]");
+
+ this->AssertBinop(Divide, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]",
+ "[1.0, 2.0, 0.7, 0.5, 1.7, 2.0, 5.0]",
+ "[1.1, 1.2, 5.0, 8.6, 3.0, 3.4, 1.46]");
Review comment:
Thank you for the good suggestion.
I have removed some test cases, and made sure the remaining cases cover the above items, with one exception: the inifinity comparisons should depend on another PR: https://github.com/apache/arrow/pull/7826
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471345721
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -186,6 +187,42 @@ struct MultiplyChecked {
}
};
+struct Divide {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ return left / right;
Review comment:
Thanks for your suggestion.
According to the IEEE 754 standard, dividing by zero should be well-defined for floating point numbers. Please see, for example, https://dl.acm.org/doi/10.1145/103162.103163, page 26.
----------------------------------------------------------------
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
[GitHub] [arrow] bkietz commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by 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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471348958
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -492,6 +486,81 @@ TYPED_TEST(TestBinaryArithmeticFloating, Add) {
this->AssertBinop(Add, "[null, 2.0]", this->MakeNullScalar(), "[null, null]");
}
+TYPED_TEST(TestBinaryArithmeticFloating, Div) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinop(Divide, "[]", "[]", "[]");
+
+ this->AssertBinop(Divide, "[3.4, 2.6, 6.3]", "[1, 2, 2]", "[3.4, 1.3, 3.15]");
+
+ this->AssertBinop(Divide, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]",
+ "[1.0, 2.0, 0.7, 0.5, 1.7, 2.0, 5.0]",
+ "[1.1, 1.2, 5.0, 8.6, 3.0, 3.4, 1.46]");
+
+ this->AssertBinop(Divide, "[10.4, 12, 4.2, 50, 50.5, 32, 11]",
+ "[2.0, 1.0, 6, 1, 5, 8, 2]", "[5.2, 12, 0.7, 50, 10.1, 4, 5.5]");
+
+ this->AssertBinop(Divide, "[null, 1, 3.3, null, 2, 5.1]", "[1, 4, 2, 5, 0.1, 3]",
+ "[null, 0.25, 1.65, null, 20, 1.7]");
+
+ this->AssertBinop(Divide, 10.0F, "[null, 1, 2.5, null, 2, 5]",
+ "[null, 10, 4, null, 5, 2]");
+
+ this->AssertBinop(Divide, "[null, 1, 2.5, null, 2, 5]", 10.0F,
+ "[null, 0.1, 0.25, null, 0.2, 0.5]");
+
+ this->AssertBinop(Divide, 21.0F, 3.0F, 7.0F);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticIntegral, Div) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+
+ this->AssertBinop(Divide, "[]", "[]", "[]");
+ this->AssertBinop(Divide, "[null]", "[null]", "[null]");
+ this->AssertBinop(Divide, "[3, 2, 6]", "[1, 1, 2]", "[3, 2, 3]");
+ this->AssertBinop(Divide, "[10, 20, 30, 40, 50, 60, 70]", "[10, 5, 2, 1, 25, 30, 35]",
+ "[1, 4, 15, 40, 2, 2, 2]");
+ this->AssertBinop(Divide, "[70, 60, 50, 40, 30, 20, 10]", "[7, 15, 5, 2, 6, 4, 5]",
+ "[10, 4, 10, 20, 5, 5, 2]");
+ this->AssertBinop(Divide, "[null, 10, 30, null, 20, 50]", "[1, 4, 2, 5, 10, 3]",
+ "[null, 2, 15, null, 2, 16]");
+ this->AssertBinop(Divide, 33, "[null, 1, 3, null, 2, 5]",
+ "[null, 33, 11, null, 16, 6]");
+ this->AssertBinop(Divide, 16, 7, 2);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, Div) {
+ this->AssertBinop(Divide, "[-3, 2, -6]", "[1, 1, 2]", "[-3, 2, -3]");
+ this->AssertBinop(Divide, "[10, 20, -30, 40, -50, 60, 70]", "[10, 5, 2, 1, 25, 30, 35]",
+ "[1, 4, -15, 40, -2, 2, 2]");
+ this->AssertBinop(Divide, "[70, 60, 50, 40, 30, 20, 10]", "[7, -15, 5, 2, 6, 4, -5]",
+ "[10, -4, 10, 20, 5, 5, -2]");
+ this->AssertBinop(Divide, "[null, 10, 30, null, -20, 50]", "[1, 4, 2, 5, 10, 3]",
+ "[null, 2, 15, null, -2, 16]");
+ this->AssertBinop(Divide, 33, "[null, -1, -3, null, 2, 5]",
+ "[null, -33, -11, null, 16, 6]");
+ this->AssertBinop(Divide, -16, -8, 2);
+}
+
+TYPED_TEST(TestBinaryArithmeticIntegral, DivideByZero) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinopRaises(Divide, "[3, 2, 6]", "[1, 1, 0]", "overflow");
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, DivideOverflowRaises) {
+ using CType = typename TestFixture::CType;
+
+ auto min = std::numeric_limits<CType>::lowest();
+ this->SetOverflowCheck(true);
+
+ this->AssertBinopRaises(Divide, MakeArray(min), MakeArray(-1), "overflow");
Review comment:
When overflow checks are disabled, we get SIGFP, and the tests crash.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r454959287
##########
File path: cpp/src/arrow/compute/kernels/scalar_cast_test.cc
##########
@@ -169,7 +169,6 @@ class TestCast : public TestBase {
ASSERT_EQ(in_values.size(), out_values.size());
std::shared_ptr<Array> input, expected;
if (is_valid.size() > 0) {
- ASSERT_EQ(is_valid.size(), out_values.size());
Review comment:
I happen to find this check duplicate. Admittedly, this is not related to the current issue.
----------------------------------------------------------------
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
[GitHub] [arrow] paddyhoran commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
paddyhoran commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-658304924
We have this implemented with [SIMD](https://github.com/apache/arrow/blob/master/rust/arrow/src/compute/kernels/arithmetic.rs#L162) (which was tricky) on the Rust side. We currently return a `Result` for division by zero.
----------------------------------------------------------------
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
[GitHub] [arrow] paddyhoran edited a comment on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
paddyhoran edited a comment on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-658304924
We have this [implemented](https://github.com/apache/arrow/blob/master/rust/arrow/src/compute/kernels/arithmetic.rs#L289) with [SIMD](https://github.com/apache/arrow/blob/master/rust/arrow/src/compute/kernels/arithmetic.rs#L162) (which was tricky) on the Rust side. We currently return a `Result` for division by zero.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r472827547
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -492,6 +486,81 @@ TYPED_TEST(TestBinaryArithmeticFloating, Add) {
this->AssertBinop(Add, "[null, 2.0]", this->MakeNullScalar(), "[null, null]");
}
+TYPED_TEST(TestBinaryArithmeticFloating, Div) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinop(Divide, "[]", "[]", "[]");
+
+ this->AssertBinop(Divide, "[3.4, 2.6, 6.3]", "[1, 2, 2]", "[3.4, 1.3, 3.15]");
+
+ this->AssertBinop(Divide, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]",
+ "[1.0, 2.0, 0.7, 0.5, 1.7, 2.0, 5.0]",
+ "[1.1, 1.2, 5.0, 8.6, 3.0, 3.4, 1.46]");
Review comment:
Thanks a lot for your effort. I have added test case for infinity.
In addition, I have removed/simplified more test cases, please check if it looks good to you.
----------------------------------------------------------------
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
[GitHub] [arrow] bkietz commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r469934580
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -186,6 +187,42 @@ struct MultiplyChecked {
}
};
+struct Divide {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ return left / right;
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+ if (ARROW_PREDICT_FALSE(right == 0)) {
+ ctx->SetStatus(Status::Invalid("overflow"));
Review comment:
```suggestion
ctx->SetStatus(Status::Invalid("divide by zero"));
```
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -129,6 +129,20 @@ 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:
```suggestion
/// \brief Divide two values. Array values must be the same length. If either
/// argument is null the result will be null.
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -186,6 +187,42 @@ struct MultiplyChecked {
}
};
+struct Divide {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ return left / right;
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+ if (ARROW_PREDICT_FALSE(right == 0)) {
+ ctx->SetStatus(Status::Invalid("overflow"));
+ return 0;
+ }
+ return left / right;
+ }
+};
+
+struct DivideChecked {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<T, Arg1>::value, "");
+ T result;
+ if (ARROW_PREDICT_FALSE(DivideWithOverflow(left, right, &result))) {
+ ctx->SetStatus(Status::Invalid("overflow"));
Review comment:
```suggestion
if (right == 0) {
ctx->SetStatus(Status::Invalid("divide by zero"));
} else {
ctx->SetStatus(Status::Invalid("overflow"));
}
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -186,6 +187,42 @@ struct MultiplyChecked {
}
};
+struct Divide {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ return left / right;
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+ if (ARROW_PREDICT_FALSE(right == 0)) {
+ ctx->SetStatus(Status::Invalid("overflow"));
+ return 0;
+ }
+ return left / right;
+ }
+};
+
+struct DivideChecked {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<T, Arg1>::value, "");
+ T result;
+ if (ARROW_PREDICT_FALSE(DivideWithOverflow(left, right, &result))) {
+ ctx->SetStatus(Status::Invalid("overflow"));
+ }
+ return result;
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<T, Arg1>::value, "");
+ return left / right;
Review comment:
```suggestion
if (ARROW_PREDICT_FALSE(right == 0)) {
ctx->SetStatus(Status::Invalid("divide by zero"));
return 0;
}
```
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -186,6 +187,42 @@ struct MultiplyChecked {
}
};
+struct Divide {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ return left / right;
Review comment:
```suggestion
if (ARROW_PREDICT_FALSE(right == 0)) {
ctx->SetStatus(Status::Invalid("divide by zero"));
return 0;
}
return left / right;
```
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471379295
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -492,6 +486,81 @@ TYPED_TEST(TestBinaryArithmeticFloating, Add) {
this->AssertBinop(Add, "[null, 2.0]", this->MakeNullScalar(), "[null, null]");
}
+TYPED_TEST(TestBinaryArithmeticFloating, Div) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinop(Divide, "[]", "[]", "[]");
+
+ this->AssertBinop(Divide, "[3.4, 2.6, 6.3]", "[1, 2, 2]", "[3.4, 1.3, 3.15]");
+
+ this->AssertBinop(Divide, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]",
+ "[1.0, 2.0, 0.7, 0.5, 1.7, 2.0, 5.0]",
+ "[1.1, 1.2, 5.0, 8.6, 3.0, 3.4, 1.46]");
+
+ this->AssertBinop(Divide, "[10.4, 12, 4.2, 50, 50.5, 32, 11]",
+ "[2.0, 1.0, 6, 1, 5, 8, 2]", "[5.2, 12, 0.7, 50, 10.1, 4, 5.5]");
+
+ this->AssertBinop(Divide, "[null, 1, 3.3, null, 2, 5.1]", "[1, 4, 2, 5, 0.1, 3]",
+ "[null, 0.25, 1.65, null, 20, 1.7]");
+
+ this->AssertBinop(Divide, 10.0F, "[null, 1, 2.5, null, 2, 5]",
+ "[null, 10, 4, null, 5, 2]");
+
+ this->AssertBinop(Divide, "[null, 1, 2.5, null, 2, 5]", 10.0F,
+ "[null, 0.1, 0.25, null, 0.2, 0.5]");
+
+ this->AssertBinop(Divide, 21.0F, 3.0F, 7.0F);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticIntegral, Div) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+
+ this->AssertBinop(Divide, "[]", "[]", "[]");
+ this->AssertBinop(Divide, "[null]", "[null]", "[null]");
+ this->AssertBinop(Divide, "[3, 2, 6]", "[1, 1, 2]", "[3, 2, 3]");
+ this->AssertBinop(Divide, "[10, 20, 30, 40, 50, 60, 70]", "[10, 5, 2, 1, 25, 30, 35]",
+ "[1, 4, 15, 40, 2, 2, 2]");
+ this->AssertBinop(Divide, "[70, 60, 50, 40, 30, 20, 10]", "[7, 15, 5, 2, 6, 4, 5]",
+ "[10, 4, 10, 20, 5, 5, 2]");
+ this->AssertBinop(Divide, "[null, 10, 30, null, 20, 50]", "[1, 4, 2, 5, 10, 3]",
+ "[null, 2, 15, null, 2, 16]");
+ this->AssertBinop(Divide, 33, "[null, 1, 3, null, 2, 5]",
+ "[null, 33, 11, null, 16, 6]");
+ this->AssertBinop(Divide, 16, 7, 2);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, Div) {
+ this->AssertBinop(Divide, "[-3, 2, -6]", "[1, 1, 2]", "[-3, 2, -3]");
+ this->AssertBinop(Divide, "[10, 20, -30, 40, -50, 60, 70]", "[10, 5, 2, 1, 25, 30, 35]",
+ "[1, 4, -15, 40, -2, 2, 2]");
+ this->AssertBinop(Divide, "[70, 60, 50, 40, 30, 20, 10]", "[7, -15, 5, 2, 6, 4, -5]",
+ "[10, -4, 10, 20, 5, 5, -2]");
+ this->AssertBinop(Divide, "[null, 10, 30, null, -20, 50]", "[1, 4, 2, 5, 10, 3]",
+ "[null, 2, 15, null, -2, 16]");
+ this->AssertBinop(Divide, 33, "[null, -1, -3, null, 2, 5]",
+ "[null, -33, -11, null, 16, 6]");
+ this->AssertBinop(Divide, -16, -8, 2);
+}
+
+TYPED_TEST(TestBinaryArithmeticIntegral, DivideByZero) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinopRaises(Divide, "[3, 2, 6]", "[1, 1, 0]", "overflow");
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, DivideOverflowRaises) {
+ using CType = typename TestFixture::CType;
+
+ auto min = std::numeric_limits<CType>::lowest();
+ this->SetOverflowCheck(true);
+
+ this->AssertBinopRaises(Divide, MakeArray(min), MakeArray(-1), "overflow");
Review comment:
Sounds reasonable. I have updated the PR accordingly.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r454957895
##########
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:
Nice catch. Both occurences of this comment have been removed.
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471364203
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -492,6 +486,81 @@ TYPED_TEST(TestBinaryArithmeticFloating, Add) {
this->AssertBinop(Add, "[null, 2.0]", this->MakeNullScalar(), "[null, null]");
}
+TYPED_TEST(TestBinaryArithmeticFloating, Div) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinop(Divide, "[]", "[]", "[]");
+
+ this->AssertBinop(Divide, "[3.4, 2.6, 6.3]", "[1, 2, 2]", "[3.4, 1.3, 3.15]");
+
+ this->AssertBinop(Divide, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]",
+ "[1.0, 2.0, 0.7, 0.5, 1.7, 2.0, 5.0]",
+ "[1.1, 1.2, 5.0, 8.6, 3.0, 3.4, 1.46]");
+
+ this->AssertBinop(Divide, "[10.4, 12, 4.2, 50, 50.5, 32, 11]",
+ "[2.0, 1.0, 6, 1, 5, 8, 2]", "[5.2, 12, 0.7, 50, 10.1, 4, 5.5]");
+
+ this->AssertBinop(Divide, "[null, 1, 3.3, null, 2, 5.1]", "[1, 4, 2, 5, 0.1, 3]",
+ "[null, 0.25, 1.65, null, 20, 1.7]");
+
+ this->AssertBinop(Divide, 10.0F, "[null, 1, 2.5, null, 2, 5]",
+ "[null, 10, 4, null, 5, 2]");
+
+ this->AssertBinop(Divide, "[null, 1, 2.5, null, 2, 5]", 10.0F,
+ "[null, 0.1, 0.25, null, 0.2, 0.5]");
+
+ this->AssertBinop(Divide, 21.0F, 3.0F, 7.0F);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticIntegral, Div) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+
+ this->AssertBinop(Divide, "[]", "[]", "[]");
+ this->AssertBinop(Divide, "[null]", "[null]", "[null]");
+ this->AssertBinop(Divide, "[3, 2, 6]", "[1, 1, 2]", "[3, 2, 3]");
+ this->AssertBinop(Divide, "[10, 20, 30, 40, 50, 60, 70]", "[10, 5, 2, 1, 25, 30, 35]",
+ "[1, 4, 15, 40, 2, 2, 2]");
+ this->AssertBinop(Divide, "[70, 60, 50, 40, 30, 20, 10]", "[7, 15, 5, 2, 6, 4, 5]",
+ "[10, 4, 10, 20, 5, 5, 2]");
+ this->AssertBinop(Divide, "[null, 10, 30, null, 20, 50]", "[1, 4, 2, 5, 10, 3]",
+ "[null, 2, 15, null, 2, 16]");
+ this->AssertBinop(Divide, 33, "[null, 1, 3, null, 2, 5]",
+ "[null, 33, 11, null, 16, 6]");
+ this->AssertBinop(Divide, 16, 7, 2);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, Div) {
+ this->AssertBinop(Divide, "[-3, 2, -6]", "[1, 1, 2]", "[-3, 2, -3]");
+ this->AssertBinop(Divide, "[10, 20, -30, 40, -50, 60, 70]", "[10, 5, 2, 1, 25, 30, 35]",
+ "[1, 4, -15, 40, -2, 2, 2]");
+ this->AssertBinop(Divide, "[70, 60, 50, 40, 30, 20, 10]", "[7, -15, 5, 2, 6, 4, -5]",
+ "[10, -4, 10, 20, 5, 5, -2]");
+ this->AssertBinop(Divide, "[null, 10, 30, null, -20, 50]", "[1, 4, 2, 5, 10, 3]",
+ "[null, 2, 15, null, -2, 16]");
+ this->AssertBinop(Divide, 33, "[null, -1, -3, null, 2, 5]",
+ "[null, -33, -11, null, 16, 6]");
+ this->AssertBinop(Divide, -16, -8, 2);
+}
+
+TYPED_TEST(TestBinaryArithmeticIntegral, DivideByZero) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinopRaises(Divide, "[3, 2, 6]", "[1, 1, 0]", "overflow");
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, DivideOverflowRaises) {
+ using CType = typename TestFixture::CType;
+
+ auto min = std::numeric_limits<CType>::lowest();
+ this->SetOverflowCheck(true);
+
+ this->AssertBinopRaises(Divide, MakeArray(min), MakeArray(-1), "overflow");
Review comment:
Hmm, we should definitly not crash on overflow. In this case, I think overflow should be detected and a dummy value be output (0 perhaps?).
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -139,13 +139,7 @@ class TestBinaryArithmetic : public TestBase {
ValidateAndAssertApproxEqual(actual.make_array(), expected);
// Also check (Scalar, Scalar) operations
- const int64_t length = expected->length();
- for (int64_t i = 0; i < length; ++i) {
- const auto expected_scalar = *expected->GetScalar(i);
- ASSERT_OK_AND_ASSIGN(
- actual, func(*left->GetScalar(i), *right->GetScalar(i), options_, nullptr));
- AssertScalarsEqual(*expected_scalar, *actual.scalar(), /*verbose=*/true);
- }
+ // TODO: support scalar approx equal
Review comment:
Yes, it does make sense.
----------------------------------------------------------------
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
[GitHub] [arrow] nealrichardson commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-676467710
@pitrou if you're satisfied, could you please merge?
----------------------------------------------------------------
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
[GitHub] [arrow] wesm commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r454386790
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -74,6 +74,18 @@ class TestBinaryArithmetic : public TestBase {
ValidateAndAssertApproxEqual(actual.make_array(), expected);
}
+ // (Array, Scalar)
+ void AssertBinop(BinaryFunction func, const std::string& lhs, CType rhs,
+ const std::string& expected) {
+ auto left = ArrayFromJSON(type_singleton(), lhs);
+ ASSERT_OK_AND_ASSIGN(auto right, MakeScalar(type_singleton(), rhs));
+
+ auto exp = ArrayFromJSON(type_singleton(), expected);
+
+ ASSERT_OK_AND_ASSIGN(auto actual, func(left, right, options_, nullptr));
+ ValidateAndAssertApproxEqual(actual.make_array(), expected);
+ }
Review comment:
Need to check slices also
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -219,48 +219,89 @@ 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;
+ }
+};
+
using applicator::ScalarBinaryEqualTypes;
+using applicator::ScalarBinaryNotNullEqualTypes;
// Generate a kernel given an arithmetic functor
//
// To avoid undefined behaviour of signed integer overflow treat the signed
// input argument values as unsigned then cast them to signed making them wrap
// around.
template <typename Op>
-ArrayKernelExec NumericEqualTypesBinary(detail::GetTypeId get_id) {
+ArrayKernelExec NumericEqualTypesBinary(detail::GetTypeId get_id,
+ bool skip_null = false) {
switch (get_id.id) {
case Type::INT8:
- return ScalarBinaryEqualTypes<Int8Type, Int8Type, Op>::Exec;
+ return skip_null ? ScalarBinaryNotNullEqualTypes<Int8Type, Int8Type, Op>::Exec
Review comment:
I think this will cause both templates to always be instantiated, which is not what we want
##########
File path: cpp/src/arrow/compute/kernels/scalar_cast_test.cc
##########
@@ -169,7 +169,6 @@ class TestCast : public TestBase {
ASSERT_EQ(in_values.size(), out_values.size());
std::shared_ptr<Array> input, expected;
if (is_valid.size() > 0) {
- ASSERT_EQ(is_valid.size(), out_values.size());
Review comment:
This change is out of place?
----------------------------------------------------------------
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
[GitHub] [arrow] bkietz commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r472291346
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -186,6 +187,42 @@ struct MultiplyChecked {
}
};
+struct Divide {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ return left / right;
Review comment:
Alright, I withdraw this suggestion
----------------------------------------------------------------
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
[GitHub] [arrow] wesm commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-658246270
cc @pitrou
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-658246588
This will wait for 2.0.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r454958337
##########
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:
Sorry for my mistake, and thank you for the careful review.
I have revised it, and added test case to cover it.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471346573
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -129,6 +129,20 @@ 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 (enable/disable overflow checking), optional
+/// \param[in] ctx the function execution context, optional
+/// \return the elementwise quotient, if there is a zero divisor,
+/// an error will be raised
Review comment:
Done. Thank you for the good suggestion.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471346102
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -186,6 +187,42 @@ struct MultiplyChecked {
}
};
+struct Divide {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ return left / right;
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+ if (ARROW_PREDICT_FALSE(right == 0)) {
+ ctx->SetStatus(Status::Invalid("overflow"));
+ return 0;
+ }
+ return left / right;
+ }
+};
+
+struct DivideChecked {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<T, Arg1>::value, "");
+ T result;
+ if (ARROW_PREDICT_FALSE(DivideWithOverflow(left, right, &result))) {
+ ctx->SetStatus(Status::Invalid("overflow"));
+ }
+ return result;
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<T, Arg1>::value, "");
+ return left / right;
Review comment:
For floating point numbers, dividing by zero should be well-defined?
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-676031385
After more experiments, I find @bkietz is right, as the test case crashed on AMD64 Ubuntu 18.04 when dividing by floating point 0.
So I have accepted his suggestions (Thank you @bkietz ).
----------------------------------------------------------------
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
[GitHub] [arrow] wesm commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-658245887
This can't be merged yet. Divide by zero in the unchecked case causes SIGFPE process crash.
We should probably return null when dividing by zero, this is what Impala does
https://github.com/apache/impala/blob/b5805de3e65fd1c7154e4169b323bb38ddc54f4f/be/src/exprs/operators-ir.cc#L60
I'm curious what other database-type systems do
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r454955654
##########
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:
Description added. Thank you.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471347282
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -139,13 +139,7 @@ class TestBinaryArithmetic : public TestBase {
ValidateAndAssertApproxEqual(actual.make_array(), expected);
// Also check (Scalar, Scalar) operations
- const int64_t length = expected->length();
- for (int64_t i = 0; i < length; ++i) {
- const auto expected_scalar = *expected->GetScalar(i);
- ASSERT_OK_AND_ASSIGN(
- actual, func(*left->GetScalar(i), *right->GetScalar(i), options_, nullptr));
- AssertScalarsEqual(*expected_scalar, *actual.scalar(), /*verbose=*/true);
- }
+ // TODO: support scalar approx equal
Review comment:
I have restored the checks.
I still think it makes sense to provide approx equality checks for scalar?
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r469995900
##########
File path: docs/source/cpp/compute.rst
##########
@@ -197,6 +197,10 @@ an ``Invalid`` :class:`Status` when overflow is detected.
+--------------------------+------------+--------------------+---------------------+
| subtract_checked | Binary | Numeric | Numeric |
+--------------------------+------------+--------------------+---------------------+
+| divide | Binary | Numeric | Numeric |
Review comment:
Please keep this table in alphabetical order.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -139,13 +139,7 @@ class TestBinaryArithmetic : public TestBase {
ValidateAndAssertApproxEqual(actual.make_array(), expected);
// Also check (Scalar, Scalar) operations
- const int64_t length = expected->length();
- for (int64_t i = 0; i < length; ++i) {
- const auto expected_scalar = *expected->GetScalar(i);
- ASSERT_OK_AND_ASSIGN(
- actual, func(*left->GetScalar(i), *right->GetScalar(i), options_, nullptr));
- AssertScalarsEqual(*expected_scalar, *actual.scalar(), /*verbose=*/true);
- }
+ // TODO: support scalar approx equal
Review comment:
Please don't remove these checks, instead write the tests so that they don't require approximate equality checks.
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -492,6 +486,81 @@ TYPED_TEST(TestBinaryArithmeticFloating, Add) {
this->AssertBinop(Add, "[null, 2.0]", this->MakeNullScalar(), "[null, null]");
}
+TYPED_TEST(TestBinaryArithmeticFloating, Div) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinop(Divide, "[]", "[]", "[]");
+
+ this->AssertBinop(Divide, "[3.4, 2.6, 6.3]", "[1, 2, 2]", "[3.4, 1.3, 3.15]");
+
+ this->AssertBinop(Divide, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]",
+ "[1.0, 2.0, 0.7, 0.5, 1.7, 2.0, 5.0]",
+ "[1.1, 1.2, 5.0, 8.6, 3.0, 3.4, 1.46]");
Review comment:
It's not useful to test so many values. We know the CPU handles division properly. Instead, focus on test cases that may trigger specific behaviour:
* empty array
* nulls
* zeros
* for floating-point: infinities, perhaps not-a-number
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -492,6 +486,81 @@ TYPED_TEST(TestBinaryArithmeticFloating, Add) {
this->AssertBinop(Add, "[null, 2.0]", this->MakeNullScalar(), "[null, null]");
}
+TYPED_TEST(TestBinaryArithmeticFloating, Div) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinop(Divide, "[]", "[]", "[]");
+
+ this->AssertBinop(Divide, "[3.4, 2.6, 6.3]", "[1, 2, 2]", "[3.4, 1.3, 3.15]");
+
+ this->AssertBinop(Divide, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]",
+ "[1.0, 2.0, 0.7, 0.5, 1.7, 2.0, 5.0]",
+ "[1.1, 1.2, 5.0, 8.6, 3.0, 3.4, 1.46]");
+
+ this->AssertBinop(Divide, "[10.4, 12, 4.2, 50, 50.5, 32, 11]",
+ "[2.0, 1.0, 6, 1, 5, 8, 2]", "[5.2, 12, 0.7, 50, 10.1, 4, 5.5]");
+
+ this->AssertBinop(Divide, "[null, 1, 3.3, null, 2, 5.1]", "[1, 4, 2, 5, 0.1, 3]",
+ "[null, 0.25, 1.65, null, 20, 1.7]");
+
+ this->AssertBinop(Divide, 10.0F, "[null, 1, 2.5, null, 2, 5]",
+ "[null, 10, 4, null, 5, 2]");
+
+ this->AssertBinop(Divide, "[null, 1, 2.5, null, 2, 5]", 10.0F,
+ "[null, 0.1, 0.25, null, 0.2, 0.5]");
+
+ this->AssertBinop(Divide, 21.0F, 3.0F, 7.0F);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticIntegral, Div) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+
+ this->AssertBinop(Divide, "[]", "[]", "[]");
+ this->AssertBinop(Divide, "[null]", "[null]", "[null]");
+ this->AssertBinop(Divide, "[3, 2, 6]", "[1, 1, 2]", "[3, 2, 3]");
+ this->AssertBinop(Divide, "[10, 20, 30, 40, 50, 60, 70]", "[10, 5, 2, 1, 25, 30, 35]",
+ "[1, 4, 15, 40, 2, 2, 2]");
+ this->AssertBinop(Divide, "[70, 60, 50, 40, 30, 20, 10]", "[7, 15, 5, 2, 6, 4, 5]",
+ "[10, 4, 10, 20, 5, 5, 2]");
+ this->AssertBinop(Divide, "[null, 10, 30, null, 20, 50]", "[1, 4, 2, 5, 10, 3]",
+ "[null, 2, 15, null, 2, 16]");
+ this->AssertBinop(Divide, 33, "[null, 1, 3, null, 2, 5]",
+ "[null, 33, 11, null, 16, 6]");
+ this->AssertBinop(Divide, 16, 7, 2);
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, Div) {
+ this->AssertBinop(Divide, "[-3, 2, -6]", "[1, 1, 2]", "[-3, 2, -3]");
+ this->AssertBinop(Divide, "[10, 20, -30, 40, -50, 60, 70]", "[10, 5, 2, 1, 25, 30, 35]",
+ "[1, 4, -15, 40, -2, 2, 2]");
+ this->AssertBinop(Divide, "[70, 60, 50, 40, 30, 20, 10]", "[7, -15, 5, 2, 6, 4, -5]",
+ "[10, -4, 10, 20, 5, 5, -2]");
+ this->AssertBinop(Divide, "[null, 10, 30, null, -20, 50]", "[1, 4, 2, 5, 10, 3]",
+ "[null, 2, 15, null, -2, 16]");
+ this->AssertBinop(Divide, 33, "[null, -1, -3, null, 2, 5]",
+ "[null, -33, -11, null, 16, 6]");
+ this->AssertBinop(Divide, -16, -8, 2);
+}
+
+TYPED_TEST(TestBinaryArithmeticIntegral, DivideByZero) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinopRaises(Divide, "[3, 2, 6]", "[1, 1, 0]", "overflow");
+ }
+}
+
+TYPED_TEST(TestBinaryArithmeticSigned, DivideOverflowRaises) {
+ using CType = typename TestFixture::CType;
+
+ auto min = std::numeric_limits<CType>::lowest();
+ this->SetOverflowCheck(true);
+
+ this->AssertBinopRaises(Divide, MakeArray(min), MakeArray(-1), "overflow");
Review comment:
Should also check what happens when the overflow checks are disabled?
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -129,6 +129,20 @@ 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 (enable/disable overflow checking), optional
+/// \param[in] ctx the function execution context, optional
+/// \return the elementwise quotient, if there is a zero divisor,
+/// an error will be raised
Review comment:
Can you move the zero divisor mention to the body of the docstring (together with "If either argument is null...")?
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-658658261
> This can't be merged yet. Divide by zero in the unchecked case causes SIGFPE process crash.
>
> We should probably return null when dividing by zero, this is what Impala does
>
> https://github.com/apache/impala/blob/b5805de3e65fd1c7154e4169b323bb38ddc54f4f/be/src/exprs/operators-ir.cc#L60
>
> I'm curious what other database-type systems do
I made some initial investigations, and it shows that some systems return null upon divide by zero (e.g. Impala and Hive (https://stackoverflow.com/questions/41165708/hive-dividing-numbers-from-the-same-column)), while others returns an error (like MySQL (https://www.got-it.ai/solutions/sqlquerychat/sql-help/data-manipulation/mysql-divide-by-zero-error-querychat/) and SQL Server (https://stackoverflow.com/questions/23230041/how-to-protect-sql-statement-from-divide-by-zero-error/23230084))
Anyway, we should handle it, instead of letting the process crash.
----------------------------------------------------------------
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
[GitHub] [arrow] github-actions[bot] commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-658017491
https://issues.apache.org/jira/browse/ARROW-9388
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r472277809
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -186,6 +187,42 @@ struct MultiplyChecked {
}
};
+struct Divide {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ return left / right;
Review comment:
I'd say if the user really wants to enable FP exceptions process-wise, we shouldn't try to counteract them.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r454956210
##########
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:
Comment revised. Please check.
Also, I have revised the kernel to check the overflow condition described above. Thank you.
----------------------------------------------------------------
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
[GitHub] [arrow] bkietz commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r472240168
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -186,6 +187,42 @@ struct MultiplyChecked {
}
};
+struct Divide {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ return left / right;
Review comment:
This is true, but division by zero can result in `SIGFPE` if [`feenableexcept()`](http://www.gnu.org/savannah-checkouts/gnu/libc/manual/html_node/Control-Functions.html) or a hardware exception if [`_control87()`](https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/control87-controlfp-control87-2) are used. For example:
```sh
g++ -lm -xc++ - <<!
#include <fenv.h>
int main(int argc, char* argv[]) {
feenableexcept(FE_ALL_EXCEPT);
return static_cast<int>(1.0 / 0.0);
}
!
./a.out # abort
```
These are rare, so maybe we don't need to support them (or at least restrict this check to the checked division kernel), but they do represent a potential crash when dividing by `0.0`. @pitrou ?
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou closed pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #7748:
URL: https://github.com/apache/arrow/pull/7748
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-658655201
> Thanks for starting this. I'm going to pull this down and make some changes per my comments
@wesm Thanks a lot for your effort. Your changes look much more reasonable than mine.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471346362
##########
File path: docs/source/cpp/compute.rst
##########
@@ -197,6 +197,10 @@ an ``Invalid`` :class:`Status` when overflow is detected.
+--------------------------+------------+--------------------+---------------------+
| subtract_checked | Binary | Numeric | Numeric |
+--------------------------+------------+--------------------+---------------------+
+| divide | Binary | Numeric | Numeric |
Review comment:
Done. Thanks for your kind reminder.
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-677570863
I think unchecked floating point division by zero should simply return an infinite, but we can leave that for a separate JIRA.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471345890
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -186,6 +187,42 @@ struct MultiplyChecked {
}
};
+struct Divide {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ return left / right;
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+ if (ARROW_PREDICT_FALSE(right == 0)) {
+ ctx->SetStatus(Status::Invalid("overflow"));
+ return 0;
+ }
+ return left / right;
+ }
+};
+
+struct DivideChecked {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<T, Arg1>::value, "");
+ T result;
+ if (ARROW_PREDICT_FALSE(DivideWithOverflow(left, right, &result))) {
+ ctx->SetStatus(Status::Invalid("overflow"));
Review comment:
Accepted. Thanks for the good suggestion.
----------------------------------------------------------------
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
[GitHub] [arrow] pitrou commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471518131
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
##########
@@ -492,6 +486,81 @@ TYPED_TEST(TestBinaryArithmeticFloating, Add) {
this->AssertBinop(Add, "[null, 2.0]", this->MakeNullScalar(), "[null, null]");
}
+TYPED_TEST(TestBinaryArithmeticFloating, Div) {
+ for (auto check_overflow : {false, true}) {
+ this->SetOverflowCheck(check_overflow);
+ this->AssertBinop(Divide, "[]", "[]", "[]");
+
+ this->AssertBinop(Divide, "[3.4, 2.6, 6.3]", "[1, 2, 2]", "[3.4, 1.3, 3.15]");
+
+ this->AssertBinop(Divide, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]",
+ "[1.0, 2.0, 0.7, 0.5, 1.7, 2.0, 5.0]",
+ "[1.1, 1.2, 5.0, 8.6, 3.0, 3.4, 1.46]");
Review comment:
#7826 was merged now. Also, the tested values are still plethoric IMHO, which makes the tests less readable.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-659333289
> Thanks for doing this. Could you please be sure to add this function to https://github.com/apache/arrow/blob/master/docs/source/cpp/compute.rst?
Done. Thanks for your kind reminder.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#issuecomment-677605642
> I think unchecked floating point division by zero should simply return an infinite, but we can leave that for a separate JIRA.
@pitrou Thanks for your feedback, and sorry I did not take care of this problem. I will fix it up in the follow up issue.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471343433
##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -186,6 +187,42 @@ struct MultiplyChecked {
}
};
+struct Divide {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_point<T> Call(KernelContext*, Arg0 left,
+ Arg1 right) {
+ return left / right;
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+ if (ARROW_PREDICT_FALSE(right == 0)) {
+ ctx->SetStatus(Status::Invalid("overflow"));
Review comment:
Accepted. Thank you.
----------------------------------------------------------------
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
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #7748:
URL: https://github.com/apache/arrow/pull/7748#discussion_r471346229
##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -129,6 +129,20 @@ 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:
Nice catch. Thank you.
----------------------------------------------------------------
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