[GitHub] [arrow] pitrou commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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
+ static constexpr enable_if_floating_point 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:
us...@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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:
us...@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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::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:
us...@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #7748: ARROW-9388: [C++] Division kernels
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]",
+
