From b225681e6e42b85bc803b509ced612cac2498e41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Fri, 12 Jun 2020 17:03:31 +0200 Subject: [PATCH 1/5] quick draft for checked arithmetics --- cpp/src/arrow/compute/api_scalar.cc | 3 + cpp/src/arrow/compute/api_scalar.h | 18 ++++++ .../compute/kernels/scalar_arithmetic.cc | 55 ++++++++++++++++++ .../compute/kernels/scalar_arithmetic_test.cc | 58 +++++++++++++++++++ 4 files changed, 134 insertions(+) diff --git a/cpp/src/arrow/compute/api_scalar.cc b/cpp/src/arrow/compute/api_scalar.cc index 9cdce7c1f16..9abb22d0c9b 100644 --- a/cpp/src/arrow/compute/api_scalar.cc +++ b/cpp/src/arrow/compute/api_scalar.cc @@ -42,8 +42,11 @@ namespace compute { // Arithmetic SCALAR_EAGER_BINARY(Add, "add") +SCALAR_EAGER_BINARY(AddChecked, "add_checked") SCALAR_EAGER_BINARY(Subtract, "subtract") +SCALAR_EAGER_BINARY(SubtractChecked, "subtract_checked") SCALAR_EAGER_BINARY(Multiply, "multiply") +SCALAR_EAGER_BINARY(MultiplyChecked, "multiply_checked") // ---------------------------------------------------------------------- // Set-related operations diff --git a/cpp/src/arrow/compute/api_scalar.h b/cpp/src/arrow/compute/api_scalar.h index bc502f7bcb9..3aac84a26d3 100644 --- a/cpp/src/arrow/compute/api_scalar.h +++ b/cpp/src/arrow/compute/api_scalar.h @@ -45,6 +45,12 @@ namespace compute { ARROW_EXPORT Result Add(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); +// TODO(kszucs): create add/arithmetic options to select between the underlying kernels +// and remove these functions +ARROW_EXPORT +Result AddChecked(const Datum& left, const Datum& right, + ExecContext* ctx = NULLPTR); + /// \brief Subtract two values. Array values must be the same length. If the /// minuend or subtrahend is null the result will be null. /// @@ -55,6 +61,12 @@ Result Add(const Datum& left, const Datum& right, ExecContext* ctx = NULL ARROW_EXPORT Result Subtract(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); +// TODO(kszucs): create sub/arithmetic options to select between the underlying kernels +// and remove these functions +ARROW_EXPORT +Result SubtractChecked(const Datum& left, const Datum& right, + ExecContext* ctx = NULLPTR); + /// \brief Multiply two values. Array values must be the same length. If either /// factor is null the result will be null. /// @@ -65,6 +77,12 @@ Result Subtract(const Datum& left, const Datum& right, ExecContext* ctx = ARROW_EXPORT Result Multiply(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); +// TODO(kszucs): create mul/arithmetic options to select between the underlying kernels +// and remove these functions +ARROW_EXPORT +Result MultiplyChecked(const Datum& left, const Datum& right, + ExecContext* ctx = NULLPTR); + enum CompareOperator { EQUAL, NOT_EQUAL, diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 4459e4ffa35..c2e3cbb2612 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -35,6 +35,10 @@ using enable_if_signed_integer = enable_if_t::value, T>; template using enable_if_unsigned_integer = enable_if_t::value, T>; +template +using enable_if_integer = + enable_if_t::value || is_unsigned_integer::value, T>; + template using enable_if_floating_point = enable_if_t::value, T>; @@ -60,6 +64,22 @@ struct Add { } }; +struct AddChecked { + template + static enable_if_integer Call(KernelContext* ctx, T left, T right) { + T result; + if (__builtin_add_overflow(left, right, &result)) { + ctx->SetStatus(Status::Invalid("overflow")); + } + return result; + } + + template + static constexpr enable_if_floating_point Call(KernelContext*, T left, T right) { + return left + right; + } +}; + struct Subtract { template static constexpr enable_if_floating_point Call(KernelContext*, T left, T right) { @@ -77,6 +97,22 @@ struct Subtract { } }; +struct SubtractChecked { + template + static enable_if_integer Call(KernelContext* ctx, T left, T right) { + T result; + if (__builtin_sub_overflow(left, right, &result)) { + ctx->SetStatus(Status::Invalid("overflow")); + } + return result; + } + + template + static constexpr enable_if_floating_point Call(KernelContext*, T left, T right) { + return left - right; + } +}; + struct Multiply { static_assert(std::is_same::value, ""); static_assert(std::is_same::value, ""); @@ -116,6 +152,22 @@ struct Multiply { } }; +struct MultiplyChecked { + template + static enable_if_integer Call(KernelContext* ctx, T left, T right) { + T result; + if (__builtin_mul_overflow(left, right, &result)) { + ctx->SetStatus(Status::Invalid("overflow")); + } + return result; + } + + template + static constexpr enable_if_floating_point Call(KernelContext*, T left, T right) { + return left * right; + } +}; + namespace codegen { // Generate a kernel given an arithmetic functor @@ -168,8 +220,11 @@ namespace internal { void RegisterScalarArithmetic(FunctionRegistry* registry) { codegen::AddBinaryFunction("add", registry); + codegen::AddBinaryFunction("add_checked", registry); codegen::AddBinaryFunction("subtract", registry); + codegen::AddBinaryFunction("subtract_checked", registry); codegen::AddBinaryFunction("multiply", registry); + codegen::AddBinaryFunction("multiply_checked", registry); } } // namespace internal diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 2f2159ef642..da08e43969a 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -81,6 +81,15 @@ class TestBinaryArithmetics : public TestBase { ValidateAndAssertApproxEqual(actual.make_array(), expected); } + void AssertBinopRaises(BinaryFunction func, const std::string& lhs, + const std::string& rhs, const std::string& expected_msg) { + auto left = ArrayFromJSON(type_singleton(), lhs); + auto right = ArrayFromJSON(type_singleton(), rhs); + + EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, testing::HasSubstr(expected_msg), + func(left, right, nullptr)); + } + void ValidateAndAssertApproxEqual(std::shared_ptr actual, const std::string& expected) { auto exp = ArrayFromJSON(type_singleton(), expected); @@ -147,6 +156,25 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { "[null, 11, 13, null, 12, 15]"); this->AssertBinop(arrow::compute::Add, 17, 42, 59); + + // TODO(kszucs): consolidate these + this->AssertBinop(arrow::compute::AddChecked, "[]", "[]", "[]"); + this->AssertBinop(arrow::compute::AddChecked, "[null]", "[null]", "[null]"); + this->AssertBinop(arrow::compute::AddChecked, "[3, 2, 6]", "[1, 0, 2]", "[4, 2, 8]"); + + this->AssertBinop(arrow::compute::AddChecked, "[1, 2, 3, 4, 5, 6, 7]", + "[0, 1, 2, 3, 4, 5, 6]", "[1, 3, 5, 7, 9, 11, 13]"); + + this->AssertBinop(arrow::compute::AddChecked, "[10, 12, 4, 50, 50, 32, 11]", + "[2, 0, 6, 1, 5, 3, 4]", "[12, 12, 10, 51, 55, 35, 15]"); + + this->AssertBinop(arrow::compute::AddChecked, "[null, 1, 3, null, 2, 5]", + "[1, 4, 2, 5, 0, 3]", "[null, 5, 5, null, 2, 8]"); + + this->AssertBinop(arrow::compute::AddChecked, 10, "[null, 1, 3, null, 2, 5]", + "[null, 11, 13, null, 12, 15]"); + + this->AssertBinop(arrow::compute::AddChecked, 17, 42, 59); } TYPED_TEST(TestBinaryArithmeticsIntegral, Sub) { @@ -161,6 +189,20 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Sub) { "[null, 9, 7, null, 8, 5]"); this->AssertBinop(arrow::compute::Subtract, 20, 9, 11); + + // TODO(kszucs): consolidate these + this->AssertBinop(arrow::compute::SubtractChecked, "[]", "[]", "[]"); + this->AssertBinop(arrow::compute::SubtractChecked, "[null]", "[null]", "[null]"); + this->AssertBinop(arrow::compute::SubtractChecked, "[3, 2, 6]", "[1, 0, 2]", + "[2, 2, 4]"); + + this->AssertBinop(arrow::compute::SubtractChecked, "[1, 2, 3, 4, 5, 6, 7]", + "[0, 1, 2, 3, 4, 5, 6]", "[1, 1, 1, 1, 1, 1, 1]"); + + this->AssertBinop(arrow::compute::SubtractChecked, 10, "[null, 1, 3, null, 2, 5]", + "[null, 9, 7, null, 8, 5]"); + + this->AssertBinop(arrow::compute::SubtractChecked, 20, 9, 11); } TYPED_TEST(TestBinaryArithmeticsIntegral, Mul) { @@ -207,6 +249,22 @@ TYPED_TEST(TestBinaryArithmeticsSigned, OverflowWraps) { MakeArray(max, 2, max), MakeArray(min, CType(-2), 1)); } +TYPED_TEST(TestBinaryArithmeticsSigned, OverflowRaises) { + using CType = typename TestFixture::CType; + + auto min = std::numeric_limits::lowest(); + auto max = std::numeric_limits::max(); + + this->AssertBinopRaises(arrow::compute::AddChecked, MakeArray(min, max, max), + MakeArray(CType(-1), 1, max), "overflow"); + + this->AssertBinopRaises(arrow::compute::SubtractChecked, MakeArray(min, max, min), + MakeArray(1, max, max), "overflow"); + + this->AssertBinopRaises(arrow::compute::MultiplyChecked, MakeArray(min, max, max), + MakeArray(max, 2, max), "overflow"); +} + TYPED_TEST(TestBinaryArithmeticsUnsigned, OverflowWraps) { using CType = typename TestFixture::CType; From 7da943b9ae7bd172ba735f228da738bd1bba4226 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Mon, 15 Jun 2020 14:54:59 +0200 Subject: [PATCH 2/5] ArithmeticOptions --- cpp/src/arrow/compute/api_scalar.cc | 16 +- cpp/src/arrow/compute/api_scalar.h | 35 ++-- .../kernels/scalar_arithmetic_benchmark.cc | 7 +- .../compute/kernels/scalar_arithmetic_test.cc | 158 ++++++++---------- 4 files changed, 97 insertions(+), 119 deletions(-) diff --git a/cpp/src/arrow/compute/api_scalar.cc b/cpp/src/arrow/compute/api_scalar.cc index 9abb22d0c9b..381545fff8b 100644 --- a/cpp/src/arrow/compute/api_scalar.cc +++ b/cpp/src/arrow/compute/api_scalar.cc @@ -41,12 +41,16 @@ namespace compute { // ---------------------------------------------------------------------- // Arithmetic -SCALAR_EAGER_BINARY(Add, "add") -SCALAR_EAGER_BINARY(AddChecked, "add_checked") -SCALAR_EAGER_BINARY(Subtract, "subtract") -SCALAR_EAGER_BINARY(SubtractChecked, "subtract_checked") -SCALAR_EAGER_BINARY(Multiply, "multiply") -SCALAR_EAGER_BINARY(MultiplyChecked, "multiply_checked") +#define SCALAR_ARITHMETIC_BINARY(NAME, REGISTRY_NAME, REGISTRY_CHECKED_NAME) \ + Result NAME(const Datum& left, const Datum& right, ArithmeticOptions options, \ + ExecContext* ctx) { \ + auto func_name = (options.check_overflow) ? REGISTRY_CHECKED_NAME : REGISTRY_NAME; \ + return CallFunction(func_name, {left, right}, ctx); \ + } + +SCALAR_ARITHMETIC_BINARY(Add, "add", "add_checked") +SCALAR_ARITHMETIC_BINARY(Subtract, "subtract", "subtract_checked") +SCALAR_ARITHMETIC_BINARY(Multiply, "multiply", "multiply_checked") // ---------------------------------------------------------------------- // Set-related operations diff --git a/cpp/src/arrow/compute/api_scalar.h b/cpp/src/arrow/compute/api_scalar.h index 3aac84a26d3..28609618729 100644 --- a/cpp/src/arrow/compute/api_scalar.h +++ b/cpp/src/arrow/compute/api_scalar.h @@ -35,6 +35,11 @@ namespace compute { // ---------------------------------------------------------------------- +struct ArithmeticOptions : public FunctionOptions { + ArithmeticOptions() : check_overflow(false) {} + bool check_overflow; +}; + /// \brief Add two values together. Array values must be the same length. If /// either addend is null the result will be null. /// @@ -43,13 +48,9 @@ namespace compute { /// \param[in] ctx the function execution context, optional /// \return the elementwise sum ARROW_EXPORT -Result Add(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); - -// TODO(kszucs): create add/arithmetic options to select between the underlying kernels -// and remove these functions -ARROW_EXPORT -Result AddChecked(const Datum& left, const Datum& right, - ExecContext* ctx = NULLPTR); +Result Add(const Datum& left, const Datum& right, + ArithmeticOptions options = ArithmeticOptions(), + ExecContext* ctx = NULLPTR); /// \brief Subtract two values. Array values must be the same length. If the /// minuend or subtrahend is null the result will be null. @@ -59,13 +60,9 @@ Result AddChecked(const Datum& left, const Datum& right, /// \param[in] ctx the function execution context, optional /// \return the elementwise difference ARROW_EXPORT -Result Subtract(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); - -// TODO(kszucs): create sub/arithmetic options to select between the underlying kernels -// and remove these functions -ARROW_EXPORT -Result SubtractChecked(const Datum& left, const Datum& right, - ExecContext* ctx = NULLPTR); +Result Subtract(const Datum& left, const Datum& right, + ArithmeticOptions options = ArithmeticOptions(), + ExecContext* ctx = NULLPTR); /// \brief Multiply two values. Array values must be the same length. If either /// factor is null the result will be null. @@ -75,13 +72,9 @@ Result SubtractChecked(const Datum& left, const Datum& right, /// \param[in] ctx the function execution context, optional /// \return the elementwise product ARROW_EXPORT -Result Multiply(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); - -// TODO(kszucs): create mul/arithmetic options to select between the underlying kernels -// and remove these functions -ARROW_EXPORT -Result MultiplyChecked(const Datum& left, const Datum& right, - ExecContext* ctx = NULLPTR); +Result Multiply(const Datum& left, const Datum& right, + ArithmeticOptions options = ArithmeticOptions(), + ExecContext* ctx = NULLPTR); enum CompareOperator { EQUAL, diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_benchmark.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_benchmark.cc index 34991b99819..b301c95c680 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_benchmark.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_benchmark.cc @@ -30,7 +30,8 @@ namespace compute { constexpr auto kSeed = 0x94378165; -using BinaryOp = Result(const Datum&, const Datum&, ExecContext*); +using BinaryOp = Result(const Datum&, const Datum&, ArithmeticOptions, + ExecContext*); template static void ArrayScalarKernel(benchmark::State& state) { @@ -46,7 +47,7 @@ static void ArrayScalarKernel(benchmark::State& state) { Datum fifteen(CType(15)); for (auto _ : state) { - ABORT_NOT_OK(Op(lhs, fifteen, nullptr).status()); + ABORT_NOT_OK(Op(lhs, fifteen, ArithmeticOptions(), nullptr).status()); } state.SetItemsProcessed(state.iterations() * array_size); } @@ -66,7 +67,7 @@ static void ArrayArrayKernel(benchmark::State& state) { rand.Numeric(array_size, min, max, args.null_proportion)); for (auto _ : state) { - ABORT_NOT_OK(Op(lhs, rhs, nullptr).status()); + ABORT_NOT_OK(Op(lhs, rhs, ArithmeticOptions(), nullptr).status()); } state.SetItemsProcessed(state.iterations() * array_size); } diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index da08e43969a..b1f56cdabf7 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -47,8 +47,15 @@ class TestBinaryArithmetics : public TestBase { return TypeTraits::type_singleton(); } - using BinaryFunction = - std::function(const Datum&, const Datum&, ExecContext*)>; + using BinaryFunction = std::function(const Datum&, const Datum&, + ArithmeticOptions, ExecContext*)>; + + void SetUp() { options_.check_overflow = false; } + + void TearDown() { + options_.check_overflow = false; + // writer_ = StreamWriter{}; + } // (Scalar, Scalar) void AssertBinop(BinaryFunction func, CType lhs, CType rhs, CType expected) { @@ -56,7 +63,7 @@ class TestBinaryArithmetics : public TestBase { ASSERT_OK_AND_ASSIGN(auto right, MakeScalar(type_singleton(), rhs)); ASSERT_OK_AND_ASSIGN(auto exp, MakeScalar(type_singleton(), expected)); - ASSERT_OK_AND_ASSIGN(auto actual, func(left, right, nullptr)); + ASSERT_OK_AND_ASSIGN(auto actual, func(left, right, options_, nullptr)); AssertScalarsEqual(*exp, *actual.scalar(), true); } @@ -67,7 +74,7 @@ class TestBinaryArithmetics : public TestBase { auto right = ArrayFromJSON(type_singleton(), rhs); auto exp = ArrayFromJSON(type_singleton(), expected); - ASSERT_OK_AND_ASSIGN(auto actual, func(left, right, nullptr)); + ASSERT_OK_AND_ASSIGN(auto actual, func(left, right, options_, nullptr)); ValidateAndAssertApproxEqual(actual.make_array(), expected); } @@ -77,7 +84,7 @@ class TestBinaryArithmetics : public TestBase { auto left = ArrayFromJSON(type_singleton(), lhs); auto right = ArrayFromJSON(type_singleton(), rhs); - ASSERT_OK_AND_ASSIGN(Datum actual, func(left, right, nullptr)); + ASSERT_OK_AND_ASSIGN(Datum actual, func(left, right, options_, nullptr)); ValidateAndAssertApproxEqual(actual.make_array(), expected); } @@ -87,7 +94,7 @@ class TestBinaryArithmetics : public TestBase { auto right = ArrayFromJSON(type_singleton(), rhs); EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, testing::HasSubstr(expected_msg), - func(left, right, nullptr)); + func(left, right, options_, nullptr)); } void ValidateAndAssertApproxEqual(std::shared_ptr actual, @@ -96,6 +103,10 @@ class TestBinaryArithmetics : public TestBase { ASSERT_OK(actual->ValidateFull()); AssertArraysApproxEqual(*exp, *actual); } + + void SetOverflowCheck(bool value = true) { options_.check_overflow = value; } + + ArithmeticOptions options_ = ArithmeticOptions(); }; template @@ -139,90 +150,58 @@ TYPED_TEST_SUITE(TestBinaryArithmeticsUnsigned, UnsignedIntegerTypes); TYPED_TEST_SUITE(TestBinaryArithmeticsFloating, FloatingTypes); TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { - this->AssertBinop(arrow::compute::Add, "[]", "[]", "[]"); - this->AssertBinop(arrow::compute::Add, "[null]", "[null]", "[null]"); - this->AssertBinop(arrow::compute::Add, "[3, 2, 6]", "[1, 0, 2]", "[4, 2, 8]"); - - this->AssertBinop(arrow::compute::Add, "[1, 2, 3, 4, 5, 6, 7]", "[0, 1, 2, 3, 4, 5, 6]", - "[1, 3, 5, 7, 9, 11, 13]"); - - this->AssertBinop(arrow::compute::Add, "[10, 12, 4, 50, 50, 32, 11]", - "[2, 0, 6, 1, 5, 3, 4]", "[12, 12, 10, 51, 55, 35, 15]"); - - this->AssertBinop(arrow::compute::Add, "[null, 1, 3, null, 2, 5]", "[1, 4, 2, 5, 0, 3]", - "[null, 5, 5, null, 2, 8]"); - - this->AssertBinop(arrow::compute::Add, 10, "[null, 1, 3, null, 2, 5]", - "[null, 11, 13, null, 12, 15]"); - - this->AssertBinop(arrow::compute::Add, 17, 42, 59); - - // TODO(kszucs): consolidate these - this->AssertBinop(arrow::compute::AddChecked, "[]", "[]", "[]"); - this->AssertBinop(arrow::compute::AddChecked, "[null]", "[null]", "[null]"); - this->AssertBinop(arrow::compute::AddChecked, "[3, 2, 6]", "[1, 0, 2]", "[4, 2, 8]"); - - this->AssertBinop(arrow::compute::AddChecked, "[1, 2, 3, 4, 5, 6, 7]", - "[0, 1, 2, 3, 4, 5, 6]", "[1, 3, 5, 7, 9, 11, 13]"); - - this->AssertBinop(arrow::compute::AddChecked, "[10, 12, 4, 50, 50, 32, 11]", - "[2, 0, 6, 1, 5, 3, 4]", "[12, 12, 10, 51, 55, 35, 15]"); - - this->AssertBinop(arrow::compute::AddChecked, "[null, 1, 3, null, 2, 5]", - "[1, 4, 2, 5, 0, 3]", "[null, 5, 5, null, 2, 8]"); - - this->AssertBinop(arrow::compute::AddChecked, 10, "[null, 1, 3, null, 2, 5]", - "[null, 11, 13, null, 12, 15]"); - - this->AssertBinop(arrow::compute::AddChecked, 17, 42, 59); + for (auto check_overflow : {false, true}) { + this->SetOverflowCheck(check_overflow); + + this->AssertBinop(arrow::compute::Add, "[]", "[]", "[]"); + this->AssertBinop(arrow::compute::Add, "[null]", "[null]", "[null]"); + this->AssertBinop(arrow::compute::Add, "[3, 2, 6]", "[1, 0, 2]", "[4, 2, 8]"); + + this->AssertBinop(arrow::compute::Add, "[1, 2, 3, 4, 5, 6, 7]", + "[0, 1, 2, 3, 4, 5, 6]", "[1, 3, 5, 7, 9, 11, 13]"); + + this->AssertBinop(arrow::compute::Add, "[10, 12, 4, 50, 50, 32, 11]", + "[2, 0, 6, 1, 5, 3, 4]", "[12, 12, 10, 51, 55, 35, 15]"); + this->AssertBinop(arrow::compute::Add, "[null, 1, 3, null, 2, 5]", + "[1, 4, 2, 5, 0, 3]", "[null, 5, 5, null, 2, 8]"); + this->AssertBinop(arrow::compute::Add, 10, "[null, 1, 3, null, 2, 5]", + "[null, 11, 13, null, 12, 15]"); + this->AssertBinop(arrow::compute::Add, 17, 42, 59); + } } TYPED_TEST(TestBinaryArithmeticsIntegral, Sub) { - this->AssertBinop(arrow::compute::Subtract, "[]", "[]", "[]"); - this->AssertBinop(arrow::compute::Subtract, "[null]", "[null]", "[null]"); - this->AssertBinop(arrow::compute::Subtract, "[3, 2, 6]", "[1, 0, 2]", "[2, 2, 4]"); - - this->AssertBinop(arrow::compute::Subtract, "[1, 2, 3, 4, 5, 6, 7]", - "[0, 1, 2, 3, 4, 5, 6]", "[1, 1, 1, 1, 1, 1, 1]"); - - this->AssertBinop(arrow::compute::Subtract, 10, "[null, 1, 3, null, 2, 5]", - "[null, 9, 7, null, 8, 5]"); - - this->AssertBinop(arrow::compute::Subtract, 20, 9, 11); - - // TODO(kszucs): consolidate these - this->AssertBinop(arrow::compute::SubtractChecked, "[]", "[]", "[]"); - this->AssertBinop(arrow::compute::SubtractChecked, "[null]", "[null]", "[null]"); - this->AssertBinop(arrow::compute::SubtractChecked, "[3, 2, 6]", "[1, 0, 2]", - "[2, 2, 4]"); - - this->AssertBinop(arrow::compute::SubtractChecked, "[1, 2, 3, 4, 5, 6, 7]", - "[0, 1, 2, 3, 4, 5, 6]", "[1, 1, 1, 1, 1, 1, 1]"); - - this->AssertBinop(arrow::compute::SubtractChecked, 10, "[null, 1, 3, null, 2, 5]", - "[null, 9, 7, null, 8, 5]"); - - this->AssertBinop(arrow::compute::SubtractChecked, 20, 9, 11); + for (auto check_overflow : {false, true}) { + this->SetOverflowCheck(check_overflow); + + this->AssertBinop(arrow::compute::Subtract, "[]", "[]", "[]"); + this->AssertBinop(arrow::compute::Subtract, "[null]", "[null]", "[null]"); + this->AssertBinop(arrow::compute::Subtract, "[3, 2, 6]", "[1, 0, 2]", "[2, 2, 4]"); + this->AssertBinop(arrow::compute::Subtract, "[1, 2, 3, 4, 5, 6, 7]", + "[0, 1, 2, 3, 4, 5, 6]", "[1, 1, 1, 1, 1, 1, 1]"); + this->AssertBinop(arrow::compute::Subtract, 10, "[null, 1, 3, null, 2, 5]", + "[null, 9, 7, null, 8, 5]"); + this->AssertBinop(arrow::compute::Subtract, 20, 9, 11); + } } TYPED_TEST(TestBinaryArithmeticsIntegral, Mul) { - this->AssertBinop(arrow::compute::Multiply, "[]", "[]", "[]"); - this->AssertBinop(arrow::compute::Multiply, "[null]", "[null]", "[null]"); - this->AssertBinop(arrow::compute::Multiply, "[3, 2, 6]", "[1, 0, 2]", "[3, 0, 12]"); - - this->AssertBinop(arrow::compute::Multiply, "[1, 2, 3, 4, 5, 6, 7]", - "[0, 1, 2, 3, 4, 5, 6]", "[0, 2, 6, 12, 20, 30, 42]"); - - this->AssertBinop(arrow::compute::Multiply, "[7, 6, 5, 4, 3, 2, 1]", - "[6, 5, 4, 3, 2, 1, 0]", "[42, 30, 20, 12, 6, 2, 0]"); - - this->AssertBinop(arrow::compute::Multiply, "[null, 1, 3, null, 2, 5]", - "[1, 4, 2, 5, 0, 3]", "[null, 4, 6, null, 0, 15]"); - - this->AssertBinop(arrow::compute::Multiply, 3, "[null, 1, 3, null, 2, 5]", - "[null, 3, 9, null, 6, 15]"); - - this->AssertBinop(arrow::compute::Multiply, 6, 7, 42); + for (auto check_overflow : {false, true}) { + this->SetOverflowCheck(check_overflow); + + this->AssertBinop(arrow::compute::Multiply, "[]", "[]", "[]"); + this->AssertBinop(arrow::compute::Multiply, "[null]", "[null]", "[null]"); + this->AssertBinop(arrow::compute::Multiply, "[3, 2, 6]", "[1, 0, 2]", "[3, 0, 12]"); + this->AssertBinop(arrow::compute::Multiply, "[1, 2, 3, 4, 5, 6, 7]", + "[0, 1, 2, 3, 4, 5, 6]", "[0, 2, 6, 12, 20, 30, 42]"); + this->AssertBinop(arrow::compute::Multiply, "[7, 6, 5, 4, 3, 2, 1]", + "[6, 5, 4, 3, 2, 1, 0]", "[42, 30, 20, 12, 6, 2, 0]"); + this->AssertBinop(arrow::compute::Multiply, "[null, 1, 3, null, 2, 5]", + "[1, 4, 2, 5, 0, 3]", "[null, 4, 6, null, 0, 15]"); + this->AssertBinop(arrow::compute::Multiply, 3, "[null, 1, 3, null, 2, 5]", + "[null, 3, 9, null, 6, 15]"); + this->AssertBinop(arrow::compute::Multiply, 6, 7, 42); + } } TYPED_TEST(TestBinaryArithmeticsSigned, Add) { @@ -255,13 +234,13 @@ TYPED_TEST(TestBinaryArithmeticsSigned, OverflowRaises) { auto min = std::numeric_limits::lowest(); auto max = std::numeric_limits::max(); - this->AssertBinopRaises(arrow::compute::AddChecked, MakeArray(min, max, max), - MakeArray(CType(-1), 1, max), "overflow"); + this->SetOverflowCheck(true); - this->AssertBinopRaises(arrow::compute::SubtractChecked, MakeArray(min, max, min), + this->AssertBinopRaises(arrow::compute::Add, MakeArray(min, max, max), + MakeArray(CType(-1), 1, max), "overflow"); + this->AssertBinopRaises(arrow::compute::Subtract, MakeArray(min, max, min), MakeArray(1, max, max), "overflow"); - - this->AssertBinopRaises(arrow::compute::MultiplyChecked, MakeArray(min, max, max), + this->AssertBinopRaises(arrow::compute::Multiply, MakeArray(min, max, max), MakeArray(max, 2, max), "overflow"); } @@ -271,6 +250,7 @@ TYPED_TEST(TestBinaryArithmeticsUnsigned, OverflowWraps) { auto min = std::numeric_limits::lowest(); auto max = std::numeric_limits::max(); + this->SetOverflowCheck(false); this->AssertBinop(arrow::compute::Add, MakeArray(min, max, max), MakeArray(CType(-1), 1, max), MakeArray(max, min, CType(-2))); From d4b1912d436e244f32d9c509a94be56f47d6b1b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Mon, 15 Jun 2020 17:08:42 +0200 Subject: [PATCH 3/5] builtin alternatives --- .../compute/kernels/scalar_arithmetic.cc | 49 +++++++++++++++++++ .../compute/kernels/scalar_arithmetic_test.cc | 26 ++++++++-- cpp/src/arrow/util/int_util.h | 6 +++ 3 files changed, 77 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index c2e3cbb2612..ca525cb73e2 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -18,6 +18,10 @@ #include "arrow/compute/kernels/common.h" #include "arrow/util/int_util.h" +#ifndef __has_builtin +#define __has_builtin(x) 0 +#endif + namespace arrow { namespace compute { @@ -65,6 +69,7 @@ struct Add { }; struct AddChecked { +#if __has_builtin(__builtin_add_overflow) template static enable_if_integer Call(KernelContext* ctx, T left, T right) { T result; @@ -73,6 +78,25 @@ struct AddChecked { } return result; } +#else + template + static enable_if_unsigned_integer Call(KernelContext* ctx, T left, T right) { + if (internal::HasAdditionOverflow(left, right)) { + ctx->SetStatus(Status::Invalid("overflow")); + } + return left + right; + } + + template + static enable_if_signed_integer Call(KernelContext* ctx, T left, T right) { + auto unsigned_left = to_unsigned(left); + auto unsigned_right = to_unsigned(right); + if (internal::HasAdditionOverflow(unsigned_left, unsigned_right)) { + ctx->SetStatus(Status::Invalid("overflow")); + } + return unsigned_left + unsigned_right; + } +#endif template static constexpr enable_if_floating_point Call(KernelContext*, T left, T right) { @@ -98,6 +122,7 @@ struct Subtract { }; struct SubtractChecked { +#if __has_builtin(__builtin_sub_overflow) template static enable_if_integer Call(KernelContext* ctx, T left, T right) { T result; @@ -106,6 +131,23 @@ struct SubtractChecked { } return result; } +#else + template + static enable_if_unsigned_integer Call(KernelContext* ctx, T left, T right) { + if (internal::HasSubtractionOverflow(left, right)) { + ctx->SetStatus(Status::Invalid("overflow")); + } + return left - right; + } + + template + static enable_if_signed_integer Call(KernelContext* ctx, T left, T right) { + if (internal::HasSubtractionOverflow(left, right)) { + ctx->SetStatus(Status::Invalid("overflow")); + } + return to_unsigned(left) - to_unsigned(right); + } +#endif template static constexpr enable_if_floating_point Call(KernelContext*, T left, T right) { @@ -156,9 +198,16 @@ struct MultiplyChecked { template static enable_if_integer Call(KernelContext* ctx, T left, T right) { T result; +#if __has_builtin(__builtin_mul_overflow) if (__builtin_mul_overflow(left, right, &result)) { ctx->SetStatus(Status::Invalid("overflow")); } +#else + result = Multiply::Call(ctx, left, right); + if (left != 0 && result / left != right) { + ctx->SetStatus(Status::Invalid("overflow")); + } +#endif return result; } diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index b1f56cdabf7..41f8eba913e 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -223,12 +223,11 @@ TYPED_TEST(TestBinaryArithmeticsSigned, OverflowWraps) { this->AssertBinop(arrow::compute::Subtract, MakeArray(min, max, min), MakeArray(1, max, max), MakeArray(max, 0, 1)); - this->AssertBinop(arrow::compute::Multiply, MakeArray(min, max, max), MakeArray(max, 2, max), MakeArray(min, CType(-2), 1)); } -TYPED_TEST(TestBinaryArithmeticsSigned, OverflowRaises) { +TYPED_TEST(TestBinaryArithmeticsIntegral, OverflowRaises) { using CType = typename TestFixture::CType; auto min = std::numeric_limits::lowest(); @@ -238,12 +237,31 @@ TYPED_TEST(TestBinaryArithmeticsSigned, OverflowRaises) { this->AssertBinopRaises(arrow::compute::Add, MakeArray(min, max, max), MakeArray(CType(-1), 1, max), "overflow"); - this->AssertBinopRaises(arrow::compute::Subtract, MakeArray(min, max, min), - MakeArray(1, max, max), "overflow"); + this->AssertBinopRaises(arrow::compute::Subtract, MakeArray(min, max), + MakeArray(1, max), "overflow"); + this->AssertBinopRaises(arrow::compute::Subtract, MakeArray(min), MakeArray(max), + "overflow"); + this->AssertBinopRaises(arrow::compute::Multiply, MakeArray(min, max, max), MakeArray(max, 2, max), "overflow"); } +TYPED_TEST(TestBinaryArithmeticsSigned, OverflowRaises) { + using CType = typename TestFixture::CType; + + auto min = std::numeric_limits::lowest(); + auto max = std::numeric_limits::max(); + + this->SetOverflowCheck(true); + + this->AssertBinop(arrow::compute::Multiply, MakeArray(max), MakeArray(-1), + MakeArray(min + 1)); + this->AssertBinopRaises(arrow::compute::Multiply, MakeArray(max), MakeArray(2), + "overflow"); + this->AssertBinopRaises(arrow::compute::Multiply, MakeArray(min), MakeArray(-1), + "overflow"); +} + TYPED_TEST(TestBinaryArithmeticsUnsigned, OverflowWraps) { using CType = typename TestFixture::CType; diff --git a/cpp/src/arrow/util/int_util.h b/cpp/src/arrow/util/int_util.h index 0478c55e623..3131476bfec 100644 --- a/cpp/src/arrow/util/int_util.h +++ b/cpp/src/arrow/util/int_util.h @@ -100,6 +100,12 @@ bool HasAdditionOverflow(Integer value, Integer addend) { return (value > std::numeric_limits::max() - addend); } +/// Detect addition overflow between integers +template +bool HasSubtractionOverflow(Integer value, Integer minuend) { + return (value < minuend); +} + /// Upcast an integer to the largest possible width (currently 64 bits) template From 4b6c9081cb076cecf847a446f91eeee9b4fd9733 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Mon, 15 Jun 2020 17:12:05 +0200 Subject: [PATCH 4/5] unused teardown in test --- cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 41f8eba913e..87c3750f123 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -52,11 +52,6 @@ class TestBinaryArithmetics : public TestBase { void SetUp() { options_.check_overflow = false; } - void TearDown() { - options_.check_overflow = false; - // writer_ = StreamWriter{}; - } - // (Scalar, Scalar) void AssertBinop(BinaryFunction func, CType lhs, CType rhs, CType expected) { ASSERT_OK_AND_ASSIGN(auto left, MakeScalar(type_singleton(), lhs)); From d31ea5cce85623b70234d5f15f4e6fce958bf6d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Mon, 15 Jun 2020 18:22:59 +0200 Subject: [PATCH 5/5] fix utility function namespace --- cpp/src/arrow/compute/kernels/scalar_arithmetic.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index ca525cb73e2..36f52a78bc4 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -81,7 +81,7 @@ struct AddChecked { #else template static enable_if_unsigned_integer Call(KernelContext* ctx, T left, T right) { - if (internal::HasAdditionOverflow(left, right)) { + if (arrow::internal::HasAdditionOverflow(left, right)) { ctx->SetStatus(Status::Invalid("overflow")); } return left + right; @@ -91,7 +91,7 @@ struct AddChecked { static enable_if_signed_integer Call(KernelContext* ctx, T left, T right) { auto unsigned_left = to_unsigned(left); auto unsigned_right = to_unsigned(right); - if (internal::HasAdditionOverflow(unsigned_left, unsigned_right)) { + if (arrow::internal::HasAdditionOverflow(unsigned_left, unsigned_right)) { ctx->SetStatus(Status::Invalid("overflow")); } return unsigned_left + unsigned_right; @@ -134,7 +134,7 @@ struct SubtractChecked { #else template static enable_if_unsigned_integer Call(KernelContext* ctx, T left, T right) { - if (internal::HasSubtractionOverflow(left, right)) { + if (arrow::internal::HasSubtractionOverflow(left, right)) { ctx->SetStatus(Status::Invalid("overflow")); } return left - right; @@ -142,7 +142,7 @@ struct SubtractChecked { template static enable_if_signed_integer Call(KernelContext* ctx, T left, T right) { - if (internal::HasSubtractionOverflow(left, right)) { + if (arrow::internal::HasSubtractionOverflow(left, right)) { ctx->SetStatus(Status::Invalid("overflow")); } return to_unsigned(left) - to_unsigned(right);