Skip to content

ARROW-9388: [C++] Division kernels #7748

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
liyafan82 wants to merge 1 commit into from
Closed

ARROW-9388: [C++] Division kernels #7748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cpp/src/arrow/compute/api_scalar.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ namespace compute {
SCALAR_ARITHMETIC_BINARY(Add, "add", "add_checked")
SCALAR_ARITHMETIC_BINARY(Subtract, "subtract", "subtract_checked")
SCALAR_ARITHMETIC_BINARY(Multiply, "multiply", "multiply_checked")
SCALAR_ARITHMETIC_BINARY(Divide, "divide", "divide_checked")

// ----------------------------------------------------------------------
// Set-related operations
Expand Down
14 changes: 14 additions & 0 deletions cpp/src/arrow/compute/api_scalar.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
/// argument is null the result will be null. For integer types, if there is
/// a zero divisor, an error will be raised.
///
/// \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
ARROW_EXPORT
Result<Datum> Divide(const Datum& left, const Datum& right,
ArithmeticOptions options = ArithmeticOptions(),
ExecContext* ctx = NULLPTR);

/// \brief Compare a numeric array with a scalar.
///
/// \param[in] left datum to compare, must be an Array
Expand Down
3 changes: 2 additions & 1 deletion cpp/src/arrow/compute/kernels/codegen_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,6 @@ struct ScalarBinary {
}
} else {
if (batch[1].kind() == Datum::ARRAY) {
// e.g. if we were doing scalar < array, we flip and do array >= scalar
return ScalarArray(ctx, *batch[0].scalar(), *batch[1].array(), out);
} else {
return ScalarScalar(ctx, *batch[0].scalar(), *batch[1].scalar(), out);
Expand Down Expand Up @@ -842,6 +841,8 @@ struct ScalarBinaryNotNull {
template <typename OutType, typename ArgType, typename Op>
using ScalarBinaryEqualTypes = ScalarBinary<OutType, ArgType, ArgType, Op>;

// A kernel exec generator for non-null binary kernels where both input types are the
// same
template <typename OutType, typename ArgType, typename Op>
using ScalarBinaryNotNullEqualTypes = ScalarBinaryNotNull<OutType, ArgType, ArgType, Op>;

Expand Down
59 changes: 59 additions & 0 deletions cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
namespace arrow {

using internal::AddWithOverflow;
using internal::DivideWithOverflow;
using internal::MultiplyWithOverflow;
using internal::SubtractWithOverflow;

Expand Down Expand Up @@ -186,6 +187,56 @@ struct MultiplyChecked {
}
};

struct Divide {
template <typename T, typename Arg0, typename Arg1>
static enable_if_floating_point<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
if (ARROW_PREDICT_FALSE(right == 0)) {
ctx->SetStatus(Status::Invalid("divide by zero"));
return 0;
}
return left / right;
Copy link
Member

@bkietz bkietz Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return left / right;
if (ARROW_PREDICT_FALSE(right == 0)) {
ctx->SetStatus(Status::Invalid("divide by zero"));
return 0;
}
return left / right;

Copy link
Contributor Author

@liyafan82 liyafan82 Aug 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bkietz bkietz Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true, but division by zero can result in SIGFPE if feenableexcept() or a hardware exception if _control87() are used. For example:

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 ?

Copy link
Member

@pitrou pitrou Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say if the user really wants to enable FP exceptions process-wise, we shouldn't try to counteract them.

Copy link
Member

@bkietz bkietz Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I withdraw this suggestion

}

template <typename T, typename Arg0, typename Arg1>
static enable_if_integer<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
T result;
if (ARROW_PREDICT_FALSE(DivideWithOverflow(left, right, &result))) {
if (right == 0) {
ctx->SetStatus(Status::Invalid("divide by zero"));
} else {
result = 0;
}
}
return result;
}
};

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))) {
if (right == 0) {
ctx->SetStatus(Status::Invalid("divide by zero"));
} else {
ctx->SetStatus(Status::Invalid("overflow"));
}
}
return result;
}

template <typename T, typename Arg0, typename Arg1>
static enable_if_floating_point<T> Call(KernelContext* ctx, Arg0 left, Arg1 right) {
static_assert(std::is_same<T, Arg0>::value && std::is_same<T, Arg1>::value, "");
if (ARROW_PREDICT_FALSE(right == 0)) {
ctx->SetStatus(Status::Invalid("divide by zero"));
return 0;
}
return left / right;
}
};

// Generate a kernel given an arithmetic functor
template <template <typename... Args> class KernelGenerator, typename Op>
ArrayKernelExec NumericEqualTypesBinary(detail::GetTypeId get_id) {
Expand Down Expand Up @@ -277,6 +328,14 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
auto multiply_checked =
MakeArithmeticFunctionNotNull<MultiplyChecked>("multiply_checked");
DCHECK_OK(registry->AddFunction(std::move(multiply_checked)));

// ----------------------------------------------------------------------
auto divide = MakeArithmeticFunctionNotNull<Divide>("divide");
DCHECK_OK(registry->AddFunction(std::move(divide)));

// ----------------------------------------------------------------------
auto divide_checked = MakeArithmeticFunctionNotNull<DivideChecked>("divide_checked");
DCHECK_OK(registry->AddFunction(std::move(divide_checked)));
}

} // namespace internal
Expand Down
2 changes: 2 additions & 0 deletions cpp/src/arrow/compute/kernels/scalar_arithmetic_benchmark.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ DECLARE_ARITHMETIC_BENCHMARKS(ArrayArrayKernel, Subtract);
DECLARE_ARITHMETIC_BENCHMARKS(ArrayScalarKernel, Subtract);
DECLARE_ARITHMETIC_BENCHMARKS(ArrayArrayKernel, Multiply);
DECLARE_ARITHMETIC_BENCHMARKS(ArrayScalarKernel, Multiply);
DECLARE_ARITHMETIC_BENCHMARKS(ArrayArrayKernel, Divide);
DECLARE_ARITHMETIC_BENCHMARKS(ArrayScalarKernel, Divide);

DECLARE_ARITHMETIC_CHECKED_BENCHMARKS(ArrayArrayKernel, AddChecked);
DECLARE_ARITHMETIC_CHECKED_BENCHMARKS(ArrayScalarKernel, AddChecked);
Expand Down
82 changes: 82 additions & 0 deletions cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,88 @@ 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);
// Empty arrays
this->AssertBinop(Divide, "[]", "[]", "[]");
// Ordinary arrays
this->AssertBinop(Divide, "[3.4, 0.64, 1.28]", "[1, 2, 4]", "[3.4, 0.32, 0.32]");
// Array with nulls
this->AssertBinop(Divide, "[null, 1, 3.3, null, 2]", "[1, 4, 2, 5, 0.1]",
"[null, 0.25, 1.65, null, 20]");
// Scalar divides by array
this->AssertBinop(Divide, 10.0F, "[null, 1, 2.5, null, 2, 5]",
"[null, 10, 4, null, 5, 2]");
// Array divides by scalar
this->AssertBinop(Divide, "[null, 1, 2.5, null, 2, 5]", 10.0F,
"[null, 0.1, 0.25, null, 0.2, 0.5]");
// Array with infinity
this->AssertBinop(Divide, "[3.4, Inf, -Inf]", "[1, 2, 3]", "[3.4, Inf, -Inf]");
// Scalar divides by scalar
this->AssertBinop(Divide, 21.0F, 3.0F, 7.0F);
}
}

TYPED_TEST(TestBinaryArithmeticIntegral, Div) {
for (auto check_overflow : {false, true}) {
this->SetOverflowCheck(check_overflow);

// Empty arrays
this->AssertBinop(Divide, "[]", "[]", "[]");
// Ordinary arrays
this->AssertBinop(Divide, "[3, 2, 6]", "[1, 1, 2]", "[3, 2, 3]");
// Array with nulls
this->AssertBinop(Divide, "[null, 10, 30, null, 20]", "[1, 4, 2, 5, 10]",
"[null, 2, 15, null, 2]");
// Scalar divides by array
this->AssertBinop(Divide, 33, "[null, 1, 3, null, 2]", "[null, 33, 11, null, 16]");
// Array divides by scalar
this->AssertBinop(Divide, "[null, 10, 30, null, 2]", 3, "[null, 3, 10, null, 0]");
// Scalar divides by scalar
this->AssertBinop(Divide, 16, 7, 2);
}
}

TYPED_TEST(TestBinaryArithmeticSigned, Div) {
// Ordinary arrays
this->AssertBinop(Divide, "[-3, 2, -6]", "[1, 1, 2]", "[-3, 2, -3]");
// Array with nulls
this->AssertBinop(Divide, "[null, 10, 30, null, -20]", "[1, 4, 2, 5, 10]",
"[null, 2, 15, null, -2]");
// Scalar divides by array
this->AssertBinop(Divide, 33, "[null, -1, -3, null, 2]", "[null, -33, -11, null, 16]");
// Array divides by scalar
this->AssertBinop(Divide, "[null, 10, 30, null, 2]", 3, "[null, 3, 10, null, 0]");
// Scalar divides by scalar
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]", "divide by zero");
}
}

TYPED_TEST(TestBinaryArithmeticFloating, DivideByZero) {
for (auto check_overflow : {false, true}) {
this->SetOverflowCheck(check_overflow);
this->AssertBinopRaises(Divide, "[3.0, 2.0, 6.0]", "[1.0, 1.0, 0]", "divide by zero");
}
}

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");
Copy link
Member

@pitrou pitrou Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also check what happens when the overflow checks are disabled?

Copy link
Contributor Author

@liyafan82 liyafan82 Aug 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When overflow checks are disabled, we get SIGFP, and the tests crash.

Copy link
Member

@pitrou pitrou Aug 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Copy link
Contributor Author

@liyafan82 liyafan82 Aug 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable. I have updated the PR accordingly.


this->SetOverflowCheck(false);
this->AssertBinop(Divide, MakeArray(min), MakeArray(-1), "[0]");
}

TYPED_TEST(TestBinaryArithmeticFloating, Sub) {
this->AssertBinop(Subtract, "[]", "[]", "[]");

Expand Down
1 change: 1 addition & 0 deletions cpp/src/arrow/util/int_util_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ namespace internal {
OPS_WITH_OVERFLOW(AddWithOverflow, add)
OPS_WITH_OVERFLOW(SubtractWithOverflow, sub)
OPS_WITH_OVERFLOW(MultiplyWithOverflow, mul)
OPS_WITH_OVERFLOW(DivideWithOverflow, div)

#undef OP_WITH_OVERFLOW
#undef OPS_WITH_OVERFLOW
Expand Down
4 changes: 4 additions & 0 deletions docs/source/cpp/compute.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,10 @@ an ``Invalid`` :class:`Status` when overflow is detected.
+--------------------------+------------+--------------------+---------------------+
| add_checked | Binary | Numeric | Numeric |
+--------------------------+------------+--------------------+---------------------+
| divide | Binary | Numeric | Numeric |
+--------------------------+------------+--------------------+---------------------+
| divide_checked | Binary | Numeric | Numeric |
+--------------------------+------------+--------------------+---------------------+
| multiply | Binary | Numeric | Numeric |
+--------------------------+------------+--------------------+---------------------+
| multiply_checked | Binary | Numeric | Numeric |
Expand Down