[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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:
us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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::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:
us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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::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:
us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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:
us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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:
us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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 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: us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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: us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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 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: us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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
+ static constexpr enable_if_floating_point Call(KernelContext*, Arg0 left,
+Arg1 right) {
+return left / right;
+ }
+
+ template
+ static enable_if_integer 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
+ static enable_if_integer Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+static_assert(std::is_same::value && std::is_same::value, "");
+T result;
+if (ARROW_PREDICT_FALSE(DivideWithOverflow(left, right, ))) {
+ ctx->SetStatus(Status::Invalid("overflow"));
+}
+return result;
+ }
+
+ template
+ static constexpr enable_if_floating_point Call(KernelContext*, Arg0 left,
+Arg1 right) {
+static_assert(std::is_same::value && std::is_same::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:
us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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
+ static constexpr enable_if_floating_point 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:
us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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
+ static constexpr enable_if_floating_point Call(KernelContext*, Arg0 left,
+Arg1 right) {
+return left / right;
+ }
+
+ template
+ static enable_if_integer 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
+ static enable_if_integer Call(KernelContext* ctx, Arg0 left, Arg1 right) {
+static_assert(std::is_same::value && std::is_same::value, "");
+T result;
+if (ARROW_PREDICT_FALSE(DivideWithOverflow(left, right, ))) {
+ 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:
us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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
+ static constexpr enable_if_floating_point Call(KernelContext*, Arg0 left,
+Arg1 right) {
+return left / right;
+ }
+
+ template
+ static enable_if_integer 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:
us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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 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:
us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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
+struct ScalarBinaryNotNull {
+ using OUT = typename GetOutputType::T;
+
+ static void ArrayArray(KernelContext* ctx, const ArrayData& arg0, const
ArrayData& arg1,
+ Datum* out) {
+ArrayIterator arg0_it(arg0);
+ArrayIterator arg1_it(arg1);
+OutputAdapter::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 arg0_it(arg0);
+auto arg1_val = UnboxScalar::Unbox(arg1);
+OutputAdapter::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::Unbox(arg0);
+ArrayIterator arg1_it(arg1);
+OutputAdapter::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::Unbox(arg0);
+ auto arg1_val = UnboxScalar::Unbox(arg1);
+ BoxScalar::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:
us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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
+ static constexpr enable_if_floating_point Call(KernelContext*, T left, T
right) {
+return left / right;
+ }
+
+ template
+ static constexpr enable_if_integer Call(KernelContext*, T left, T right) {
+return left / right;
+ }
+};
+
+struct DivideChecked {
+ template
+ static enable_if_integer 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
+ static constexpr enable_if_floating_point 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:
us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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 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: us...@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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 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: us...@infra.apache.org
