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:
us...@infra.apache.org
