From 00bb29fe3ec78b926eccdcb399435a96e31c0094 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Wed, 3 Jun 2020 00:18:02 +0200 Subject: [PATCH 01/23] safe add --- .../arrow/compute/kernels/codegen_internal.h | 38 ----- .../compute/kernels/scalar_arithmetic.cc | 66 +++++++- .../compute/kernels/scalar_arithmetic_test.cc | 159 +++++++++++++----- 3 files changed, 179 insertions(+), 84 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/codegen_internal.h b/cpp/src/arrow/compute/kernels/codegen_internal.h index de420017ec5..e2a9471e780 100644 --- a/cpp/src/arrow/compute/kernels/codegen_internal.h +++ b/cpp/src/arrow/compute/kernels/codegen_internal.h @@ -644,7 +644,6 @@ struct ScalarBinary { } 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, out); @@ -726,43 +725,6 @@ ArrayKernelExec NumericEqualTypesUnary(detail::GetTypeId get_id) { } } -// Generate a kernel given a functor of type -// -// struct OPERATOR_NAME { -// template -// static OUT Call(KernelContext*, ARG0 left, ARG1 right) { -// // IMPLEMENTATION -// } -// }; -template -ArrayKernelExec NumericEqualTypesBinary(detail::GetTypeId get_id) { - switch (get_id.id) { - case Type::INT8: - return ScalarPrimitiveExecBinary; - case Type::UINT8: - return ScalarPrimitiveExecBinary; - case Type::INT16: - return ScalarPrimitiveExecBinary; - case Type::UINT16: - return ScalarPrimitiveExecBinary; - case Type::INT32: - return ScalarPrimitiveExecBinary; - case Type::UINT32: - return ScalarPrimitiveExecBinary; - case Type::INT64: - return ScalarPrimitiveExecBinary; - case Type::UINT64: - return ScalarPrimitiveExecBinary; - case Type::FLOAT: - return ScalarPrimitiveExecBinary; - case Type::DOUBLE: - return ScalarPrimitiveExecBinary; - default: - DCHECK(false); - return ExecFail; - } -} - // Generate a kernel given a templated functor. This template effectively // "curries" the first type argument. The functor must be of the form: // diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 57129bcb6a4..93b8684efe3 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -29,12 +29,72 @@ struct Add { namespace codegen { +template +ArrayKernelExec GetBinaryExec(detail::GetTypeId get_id) { + switch (get_id.id) { + case Type::INT8: + return ScalarPrimitiveExecBinary; + case Type::UINT8: + return ScalarPrimitiveExecBinary; + case Type::INT16: + return ScalarPrimitiveExecBinary; + case Type::UINT16: + return ScalarPrimitiveExecBinary; + case Type::INT32: + return ScalarPrimitiveExecBinary; + case Type::UINT32: + return ScalarPrimitiveExecBinary; + case Type::INT64: + return ScalarPrimitiveExecBinary; + case Type::UINT64: + return ScalarPrimitiveExecBinary; + case Type::FLOAT: + return ScalarPrimitiveExecBinary; + case Type::DOUBLE: + return ScalarPrimitiveExecBinary; + default: + DCHECK(false); + return ExecFail; + } +} + +std::shared_ptr GetOutputType(detail::GetTypeId get_id) { + switch (get_id.id) { + case Type::INT8: + return int16(); + case Type::INT16: + return int32(); + case Type::INT32: + case Type::INT64: + return int64(); + case Type::UINT8: + return uint16(); + case Type::UINT16: + return uint32(); + case Type::UINT32: + case Type::UINT64: + return uint64(); + case Type::FLOAT: + return float32(); + case Type::DOUBLE: + return float64(); + default: + DCHECK(false); + return nullptr; + } +} + template void MakeBinaryFunction(std::string name, FunctionRegistry* registry) { auto func = std::make_shared(name, Arity::Binary()); - for (const std::shared_ptr& ty : NumericTypes()) { - DCHECK_OK(func->AddKernel({InputType::Array(ty), InputType::Array(ty)}, ty, - NumericEqualTypesBinary(*ty))); + for (const std::shared_ptr& input_type : NumericTypes()) { + DCHECK_OK( + func->AddKernel( + {InputType::Array(input_type), InputType::Array(input_type)}, + GetOutputType(input_type), + GetBinaryExec(*input_type) + ) + ); } DCHECK_OK(registry->AddFunction(std::move(func))); } diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 017c9f5f034..25ea343adce 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -37,68 +37,141 @@ namespace arrow { namespace compute { -template -class TestArithmeticKernel : public TestBase { - private: - void AssertAddArrays(const std::shared_ptr lhs, const std::shared_ptr rhs, - const std::shared_ptr expected) { - ASSERT_OK_AND_ASSIGN(Datum out, arrow::compute::Add(lhs, rhs)); - std::shared_ptr actual = out.make_array(); - ASSERT_OK(actual->ValidateFull()); - AssertArraysEqual(*expected, *actual); - } +template +class TestBinaryArithmetics; +template +class TestBinaryArithmetics> : public TestBase { protected: - virtual void AssertAdd(const std::string& lhs, const std::string& rhs, - const std::string& expected) { - auto type = TypeTraits::type_singleton(); - AssertAddArrays(ArrayFromJSON(type, lhs), ArrayFromJSON(type, rhs), - ArrayFromJSON(type, expected)); + using InputType = I; + using OutputType = O; + using InputCType = typename I::c_type; + using OutputCType = typename O::c_type; + + using BinaryFunction = std::function(const Datum&, const Datum&, ExecContext*)>; + + static InputCType GetMin() { + return std::numeric_limits::min(); } -}; -template -class TestArithmeticKernelFloating : public TestArithmeticKernel {}; -TYPED_TEST_SUITE(TestArithmeticKernelFloating, RealArrowTypes); + static InputCType GetMax() { + return std::numeric_limits::max(); + } -template -class TestArithmeticKernelIntegral : public TestArithmeticKernel {}; -TYPED_TEST_SUITE(TestArithmeticKernelIntegral, IntegralArrowTypes); + std::shared_ptr MakeInputArray(const std::vector& values) { + std::shared_ptr out; + ArrayFromVector(values, &out); + return out; + } -TYPED_TEST(TestArithmeticKernelFloating, Add) { - this->AssertAdd("[]", "[]", "[]"); + std::shared_ptr MakeOutputArray(const std::vector& values) { + std::shared_ptr out; + ArrayFromVector(values, &out); + return out; + } - this->AssertAdd("[3.4, 2.6, 6.3]", "[1, 0, 2]", "[4.4, 2.6, 8.3]"); + virtual void AssertBinop(BinaryFunction func, + const std::shared_ptr& lhs, + const std::shared_ptr& rhs, + const std::shared_ptr& expected) { + ASSERT_OK_AND_ASSIGN(Datum result, func(lhs, rhs, nullptr)); + std::shared_ptr out = result.make_array(); + ASSERT_OK(out->ValidateFull()); + AssertArraysEqual(*expected, *out); + } - this->AssertAdd("[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]", "[0, 1, 2, 3, 4, 5, 6]", - "[1.1, 3.4, 5.5, 7.3, 9.1, 11.8, 13.3]"); + virtual void AssertBinop(BinaryFunction func, const std::string& lhs, + const std::string& rhs, const std::string& expected) { + auto input_type = TypeTraits::type_singleton(); + auto output_type = TypeTraits::type_singleton(); + AssertBinop(func, ArrayFromJSON(input_type, lhs), ArrayFromJSON(input_type, rhs), + ArrayFromJSON(output_type, expected)); + } +}; - this->AssertAdd("[7, 6, 5, 4, 3, 2, 1]", "[6, 5, 4, 3, 2, 1, 0]", - "[13, 11, 9, 7, 5, 3, 1]"); +template +class TestBinaryArithmeticsIntegral : public TestBinaryArithmetics {}; + +template +class TestBinaryArithmeticsFloating : public TestBinaryArithmetics {}; + +// InputType - OutputType pairs +using IntegralPairs = testing::Types< + // std::pair, + std::pair, + std::pair, + std::pair, + // std::pair, + // std::pair, + std::pair, + std::pair +>; + +// InputType - OutputType pairs +using FloatingPairs = testing::Types< + // std::pair, + std::pair, + std::pair +>; + +TYPED_TEST_SUITE(TestBinaryArithmeticsIntegral, IntegralPairs); +TYPED_TEST_SUITE(TestBinaryArithmeticsFloating, FloatingPairs); + +TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { + this->AssertBinop(arrow::compute::Add, "[]", "[]", "[]"); + this->AssertBinop(arrow::compute::Add, "[]", "[]", "[]"); + 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, "[7, 6, 5, 4, 3, 2, 1]", + "[6, 5, 4, 3, 2, 1, 0]", "[13, 11, 9, 7, 5, 3, 1]"); + + 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->AssertAdd("[10.4, 12, 4.2, 50, 50.3, 32, 11]", "[2, 0, 6, 1, 5, 3, 4]", - "[12.4, 12, 10.2, 51, 55.3, 35, 15]"); +// If I uncomment the commented out signed integer pairs above then clang gives me the following +// warning: +// +// ../src/arrow/compute/kernels/scalar_arithmetic_test.cc:150:64: note: insert an explicit cast to silence this issue +// auto expected = this->MakeOutputArray({1, 12, 14, min + min, max + max}); +// ^~~~~~~~~ +// static_cast( ) +// +// Since I don't really access the types within this typed test, I assume I need a different +// approach? +TYPED_TEST(TestBinaryArithmeticsIntegral, AddCheckExtremes) { + auto min = this->GetMin(); + auto max = this->GetMax(); - this->AssertAdd("[null, 1, 3.3, null, 2, 5.3]", "[1, 4, 2, 5, 0, 3]", - "[null, 5, 5.3, null, 2, 8.3]"); + auto left = this->MakeInputArray({1, 2, 3, min, max}); + auto right = this->MakeInputArray({0, 10, 11, min, max}); + auto expected = this->MakeOutputArray({1, 12, 14, min + min, max + max}); + + this->AssertBinop(arrow::compute::Add, left, right, expected); } -TYPED_TEST(TestArithmeticKernelIntegral, Add) { - this->AssertAdd("[]", "[]", "[]"); +TYPED_TEST(TestBinaryArithmeticsFloating, Add) { + this->AssertBinop(arrow::compute::Add, "[]", "[]", "[]"); - this->AssertAdd("[3, 2, 6]", "[1, 0, 2]", "[4, 2, 8]"); + this->AssertBinop(arrow::compute::Add, "[3.4, 2.6, 6.3]", "[1, 0, 2]", "[4.4, 2.6, 8.3]"); - this->AssertAdd("[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, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]", "[0, 1, 2, 3, 4, 5, 6]", + "[1.1, 3.4, 5.5, 7.3, 9.1, 11.8, 13.3]"); - this->AssertAdd("[7, 6, 5, 4, 3, 2, 1]", "[6, 5, 4, 3, 2, 1, 0]", + this->AssertBinop(arrow::compute::Add, "[7, 6, 5, 4, 3, 2, 1]", "[6, 5, 4, 3, 2, 1, 0]", "[13, 11, 9, 7, 5, 3, 1]"); - this->AssertAdd("[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, "[10.4, 12, 4.2, 50, 50.3, 32, 11]", "[2, 0, 6, 1, 5, 3, 4]", + "[12.4, 12, 10.2, 51, 55.3, 35, 15]"); - this->AssertAdd("[null, 1, 3, null, 2, 5]", "[1, 4, 2, 5, 0, 3]", - "[null, 5, 5, null, 2, 8]"); + this->AssertBinop(arrow::compute::Add, "[null, 1, 3.3, null, 2, 5.3]", "[1, 4, 2, 5, 0, 3]", + "[null, 5, 5.3, null, 2, 8.3]"); } } // namespace compute From d2b37aaf2fc170b5856be662952e144c994b3bb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Wed, 3 Jun 2020 00:19:19 +0200 Subject: [PATCH 02/23] clang format --- .../compute/kernels/scalar_arithmetic.cc | 8 +- .../compute/kernels/scalar_arithmetic_test.cc | 78 +++++++++---------- 2 files changed, 38 insertions(+), 48 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 93b8684efe3..5d2691918db 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -89,12 +89,8 @@ void MakeBinaryFunction(std::string name, FunctionRegistry* registry) { auto func = std::make_shared(name, Arity::Binary()); for (const std::shared_ptr& input_type : NumericTypes()) { DCHECK_OK( - func->AddKernel( - {InputType::Array(input_type), InputType::Array(input_type)}, - GetOutputType(input_type), - GetBinaryExec(*input_type) - ) - ); + func->AddKernel({InputType::Array(input_type), InputType::Array(input_type)}, + GetOutputType(input_type), GetBinaryExec(*input_type))); } DCHECK_OK(registry->AddFunction(std::move(func))); } diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 25ea343adce..a1520fb7120 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -48,15 +48,12 @@ class TestBinaryArithmetics> : public TestBase { using InputCType = typename I::c_type; using OutputCType = typename O::c_type; - using BinaryFunction = std::function(const Datum&, const Datum&, ExecContext*)>; + using BinaryFunction = + std::function(const Datum&, const Datum&, ExecContext*)>; - static InputCType GetMin() { - return std::numeric_limits::min(); - } + static InputCType GetMin() { return std::numeric_limits::min(); } - static InputCType GetMax() { - return std::numeric_limits::max(); - } + static InputCType GetMax() { return std::numeric_limits::max(); } std::shared_ptr MakeInputArray(const std::vector& values) { std::shared_ptr out; @@ -70,8 +67,7 @@ class TestBinaryArithmetics> : public TestBase { return out; } - virtual void AssertBinop(BinaryFunction func, - const std::shared_ptr& lhs, + virtual void AssertBinop(BinaryFunction func, const std::shared_ptr& lhs, const std::shared_ptr& rhs, const std::shared_ptr& expected) { ASSERT_OK_AND_ASSIGN(Datum result, func(lhs, rhs, nullptr)); @@ -97,22 +93,17 @@ class TestBinaryArithmeticsFloating : public TestBinaryArithmetics {}; // InputType - OutputType pairs using IntegralPairs = testing::Types< - // std::pair, - std::pair, - std::pair, - std::pair, - // std::pair, - // std::pair, - std::pair, - std::pair ->; + // std::pair, + std::pair, std::pair, + std::pair, + // std::pair, + // std::pair, + std::pair, std::pair>; // InputType - OutputType pairs using FloatingPairs = testing::Types< - // std::pair, - std::pair, - std::pair ->; + // std::pair, + std::pair, std::pair>; TYPED_TEST_SUITE(TestBinaryArithmeticsIntegral, IntegralPairs); TYPED_TEST_SUITE(TestBinaryArithmeticsFloating, FloatingPairs); @@ -122,29 +113,31 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { this->AssertBinop(arrow::compute::Add, "[]", "[]", "[]"); 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, "[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, "[7, 6, 5, 4, 3, 2, 1]", - "[6, 5, 4, 3, 2, 1, 0]", "[13, 11, 9, 7, 5, 3, 1]"); + this->AssertBinop(arrow::compute::Add, "[7, 6, 5, 4, 3, 2, 1]", "[6, 5, 4, 3, 2, 1, 0]", + "[13, 11, 9, 7, 5, 3, 1]"); 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, "[null, 1, 3, null, 2, 5]", "[1, 4, 2, 5, 0, 3]", + "[null, 5, 5, null, 2, 8]"); } -// If I uncomment the commented out signed integer pairs above then clang gives me the following -// warning: +// If I uncomment the commented out signed integer pairs above then clang gives me the +// following warning: // -// ../src/arrow/compute/kernels/scalar_arithmetic_test.cc:150:64: note: insert an explicit cast to silence this issue +// ../src/arrow/compute/kernels/scalar_arithmetic_test.cc:150:64: note: insert an explicit +// cast to silence this issue // auto expected = this->MakeOutputArray({1, 12, 14, min + min, max + max}); // ^~~~~~~~~ -// static_cast( ) +// static_cast( ) // -// Since I don't really access the types within this typed test, I assume I need a different -// approach? +// Since I don't really access the types within this typed test, I assume I need a +// different approach? TYPED_TEST(TestBinaryArithmeticsIntegral, AddCheckExtremes) { auto min = this->GetMin(); auto max = this->GetMax(); @@ -159,19 +152,20 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, AddCheckExtremes) { TYPED_TEST(TestBinaryArithmeticsFloating, Add) { this->AssertBinop(arrow::compute::Add, "[]", "[]", "[]"); - this->AssertBinop(arrow::compute::Add, "[3.4, 2.6, 6.3]", "[1, 0, 2]", "[4.4, 2.6, 8.3]"); + this->AssertBinop(arrow::compute::Add, "[3.4, 2.6, 6.3]", "[1, 0, 2]", + "[4.4, 2.6, 8.3]"); - this->AssertBinop(arrow::compute::Add, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]", "[0, 1, 2, 3, 4, 5, 6]", - "[1.1, 3.4, 5.5, 7.3, 9.1, 11.8, 13.3]"); + this->AssertBinop(arrow::compute::Add, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]", + "[0, 1, 2, 3, 4, 5, 6]", "[1.1, 3.4, 5.5, 7.3, 9.1, 11.8, 13.3]"); this->AssertBinop(arrow::compute::Add, "[7, 6, 5, 4, 3, 2, 1]", "[6, 5, 4, 3, 2, 1, 0]", - "[13, 11, 9, 7, 5, 3, 1]"); + "[13, 11, 9, 7, 5, 3, 1]"); - this->AssertBinop(arrow::compute::Add, "[10.4, 12, 4.2, 50, 50.3, 32, 11]", "[2, 0, 6, 1, 5, 3, 4]", - "[12.4, 12, 10.2, 51, 55.3, 35, 15]"); + this->AssertBinop(arrow::compute::Add, "[10.4, 12, 4.2, 50, 50.3, 32, 11]", + "[2, 0, 6, 1, 5, 3, 4]", "[12.4, 12, 10.2, 51, 55.3, 35, 15]"); - this->AssertBinop(arrow::compute::Add, "[null, 1, 3.3, null, 2, 5.3]", "[1, 4, 2, 5, 0, 3]", - "[null, 5, 5.3, null, 2, 8.3]"); + this->AssertBinop(arrow::compute::Add, "[null, 1, 3.3, null, 2, 5.3]", + "[1, 4, 2, 5, 0, 3]", "[null, 5, 5.3, null, 2, 8.3]"); } } // namespace compute From bd94196e1af9c7bb8f5179bba0ed850c9287aa48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Wed, 3 Jun 2020 00:27:48 +0200 Subject: [PATCH 03/23] remove flippedop --- .../arrow/compute/kernels/codegen_internal.h | 30 +++++++++------- .../arrow/compute/kernels/scalar_compare.cc | 34 ++++++++----------- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/codegen_internal.h b/cpp/src/arrow/compute/kernels/codegen_internal.h index e2a9471e780..9d688020e1a 100644 --- a/cpp/src/arrow/compute/kernels/codegen_internal.h +++ b/cpp/src/arrow/compute/kernels/codegen_internal.h @@ -610,52 +610,56 @@ struct ScalarUnaryNotNull { // // implementation // } // }; -template +template struct ScalarBinary { using OUT = typename GetOutputType::T; using ARG0 = typename GetViewType::T; using ARG1 = typename GetViewType::T; - template static void ArrayArray(KernelContext* ctx, const ExecBatch& batch, Datum* out) { ArrayIterator arg0(*batch[0].array()); ArrayIterator arg1(*batch[1].array()); OutputAdapter::Write(ctx, out, [&]() -> OUT { - return ChosenOp::template Call(ctx, arg0(), arg1()); + return Op::template Call(ctx, arg0(), arg1()); }); } - template static void ArrayScalar(KernelContext* ctx, const ExecBatch& batch, Datum* out) { ArrayIterator arg0(*batch[0].array()); auto arg1 = UnboxScalar::Unbox(batch[1]); OutputAdapter::Write(ctx, out, [&]() -> OUT { - return ChosenOp::template Call(ctx, arg0(), arg1); + return Op::template Call(ctx, arg0(), arg1); + }); + } + + static void ScalarArray(KernelContext* ctx, const ExecBatch& batch, Datum* out) { + auto arg0 = UnboxScalar::Unbox(batch[0]); + ArrayIterator arg1(*batch[1].array()); + OutputAdapter::Write(ctx, out, [&]() -> OUT { + return Op::template Call(ctx, arg0, arg1()); }); } - template static void ScalarScalar(KernelContext* ctx, const ExecBatch& batch, Datum* out) { auto arg0 = UnboxScalar::Unbox(batch[0]); auto arg1 = UnboxScalar::Unbox(batch[1]); - out->value = BoxScalar::Box(ChosenOp::template Call(ctx, arg0, arg1), + out->value = BoxScalar::Box(Op::template Call(ctx, arg0, arg1), out->type()); } 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, out); + return ArrayArray(ctx, batch, out); } else { - return ArrayScalar(ctx, batch, out); + return ArrayScalar(ctx, batch, out); } } else { if (batch[1].kind() == Datum::ARRAY) { // e.g. if we were doing scalar < array, we flip and do array >= scalar - return BinaryExecFlipped(ctx, ArrayScalar, batch, out); + return ScalarArray(ctx, batch, out); } else { - return ScalarScalar(ctx, batch, out); + return ScalarScalar(ctx, batch, out); } } } @@ -665,7 +669,7 @@ struct ScalarBinary { // same template -using ScalarBinaryEqualTypes = ScalarBinary; +using ScalarBinaryEqualTypes = ScalarBinary; // ---------------------------------------------------------------------- // Dynamic kernel selectors. These functors allow a kernel implementation to be diff --git a/cpp/src/arrow/compute/kernels/scalar_compare.cc b/cpp/src/arrow/compute/kernels/scalar_compare.cc index 42a911a6e09..d782a3d41e7 100644 --- a/cpp/src/arrow/compute/kernels/scalar_compare.cc +++ b/cpp/src/arrow/compute/kernels/scalar_compare.cc @@ -68,49 +68,45 @@ struct LessEqual { } }; -template +template void AddCompare(const std::shared_ptr& ty, ScalarFunction* func) { - ArrayKernelExec exec = - codegen::ScalarBinaryEqualTypes::Exec; + ArrayKernelExec exec = codegen::ScalarBinaryEqualTypes::Exec; DCHECK_OK(func->AddKernel({ty, ty}, boolean(), exec)); } -template +template void AddTimestampComparisons(ScalarFunction* func) { ArrayKernelExec exec = - codegen::ScalarBinaryEqualTypes::Exec; + codegen::ScalarBinaryEqualTypes::Exec; for (auto unit : {TimeUnit::SECOND, TimeUnit::MILLI, TimeUnit::MICRO, TimeUnit::NANO}) { InputType in_type(match::TimestampUnit(unit)); DCHECK_OK(func->AddKernel({in_type, in_type}, boolean(), exec)); } } -template +template void MakeCompareFunction(std::string name, FunctionRegistry* registry) { auto func = std::make_shared(name, Arity::Binary()); DCHECK_OK(func->AddKernel( {boolean(), boolean()}, boolean(), - codegen::ScalarBinary::Exec)); + codegen::ScalarBinary::Exec)); for (const std::shared_ptr& ty : NumericTypes()) { - auto exec = - codegen::Numeric( - *ty); + auto exec = codegen::Numeric(*ty); DCHECK_OK(func->AddKernel({ty, ty}, boolean(), exec)); } for (const std::shared_ptr& ty : BaseBinaryTypes()) { auto exec = - codegen::BaseBinary( - *ty); + codegen::BaseBinary(*ty); DCHECK_OK(func->AddKernel({ty, ty}, boolean(), exec)); } // Temporal types requires some care because cross-unit comparisons with // everything but DATE32 and DATE64 are not implemented yet - AddCompare(date32(), func.get()); - AddCompare(date64(), func.get()); - AddTimestampComparisons(func.get()); + AddCompare(date32(), func.get()); + AddCompare(date64(), func.get()); + AddTimestampComparisons(func.get()); // TODO: Leave time32, time64, and duration for follow up work @@ -120,10 +116,10 @@ void MakeCompareFunction(std::string name, FunctionRegistry* registry) { void RegisterScalarComparison(FunctionRegistry* registry) { MakeCompareFunction("equal", registry); MakeCompareFunction("not_equal", registry); - MakeCompareFunction("less", registry); - MakeCompareFunction("less_equal", registry); - MakeCompareFunction("greater", registry); - MakeCompareFunction("greater_equal", registry); + MakeCompareFunction("less", registry); + MakeCompareFunction("less_equal", registry); + MakeCompareFunction("greater", registry); + MakeCompareFunction("greater_equal", registry); } } // namespace internal From 047d7d2b2acb3cb7926c49fdba1148b4cc8af785 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Wed, 3 Jun 2020 00:40:56 +0200 Subject: [PATCH 04/23] deduce error --- .../arrow/compute/kernels/codegen_internal.h | 2 ++ .../compute/kernels/scalar_arithmetic.cc | 26 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/cpp/src/arrow/compute/kernels/codegen_internal.h b/cpp/src/arrow/compute/kernels/codegen_internal.h index 9d688020e1a..e69dfc101ae 100644 --- a/cpp/src/arrow/compute/kernels/codegen_internal.h +++ b/cpp/src/arrow/compute/kernels/codegen_internal.h @@ -619,6 +619,8 @@ struct ScalarBinary { static void ArrayArray(KernelContext* ctx, const ExecBatch& batch, Datum* out) { ArrayIterator arg0(*batch[0].array()); ArrayIterator arg1(*batch[1].array()); + // I don't get it why it is unable to deduce the OUT type of Call since it is + // explicitly defined as the return type of the lambda. OutputAdapter::Write(ctx, out, [&]() -> OUT { return Op::template Call(ctx, arg0(), arg1()); }); diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 5d2691918db..782b2177318 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -31,6 +31,15 @@ namespace codegen { template ArrayKernelExec GetBinaryExec(detail::GetTypeId get_id) { + // These execution functions doesn't support operations between multiple shapes, + // only op(array like, array like) is supported. + // Note that we don't have plans to add binary kernels with differntly types + // lhs and rhs arguments, that will be handled with implicit casts during the + // kernel dispatch procedure. + // + // There is a ScalarBinary facility in the codegen_internals.h which already + // handles those cases. I illustrate my plan MakeBinaryFunction2 by trying to + // add a single kernel to the function. switch (get_id.id) { case Type::INT8: return ScalarPrimitiveExecBinary; @@ -95,12 +104,29 @@ void MakeBinaryFunction(std::string name, FunctionRegistry* registry) { DCHECK_OK(registry->AddFunction(std::move(func))); } +// Here is the function which doesn't compile because of type deduction error +template +void MakeBinaryFunction2(std::string name, FunctionRegistry* registry) { + auto func = std::make_shared(name, Arity::Binary()); + + // create an exec function with signature (int8, int8) -> int16 + ArrayKernelExec exec = codegen::ScalarBinaryEqualTypes::Exec; + + // add this exec function as a kernel with the appropiate signature + DCHECK_OK(func->AddKernel({int8(), int8()}, int16(), exec)); + + // finally register the function, but the + DCHECK_OK(registry->AddFunction(std::move(func))); +} + } // namespace codegen namespace internal { void RegisterScalarArithmetic(FunctionRegistry* registry) { codegen::MakeBinaryFunction("add", registry); + // Comment me out to raise my second problem + // codegen::MakeBinaryFunction2("add", registry); } } // namespace internal From f1fce32daa9ba445ad1a911d4a4c36a30b96bb5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Wed, 3 Jun 2020 00:53:21 +0200 Subject: [PATCH 05/23] add error as a comment --- .../compute/kernels/scalar_arithmetic.cc | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 782b2177318..bb06bfc080c 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -125,7 +125,27 @@ namespace internal { void RegisterScalarArithmetic(FunctionRegistry* registry) { codegen::MakeBinaryFunction("add", registry); - // Comment me out to raise my second problem + + // Uncommenting the line below raises the following compiler error: + // + // In file included from ../src/arrow/compute/kernels/scalar_arithmetic.cc:18: + // In file included from ../src/arrow/compute/kernels/common.h:31: + // ../src/arrow/compute/kernels/codegen_internal.h:537:16: error: no matching function for call to 'Call' + // return Op::template Call(ctx, arg0(), arg1()); + // ^~~~~~~~~~~~~~~~~ + // ../src/arrow/compute/kernels/codegen_internal.h:567:16: note: in instantiation of member function 'arrow::compute::codegen::ScalarBinary::ArrayArray' requested here + // return ArrayArray(ctx, batch, out); + // ^ + // ../src/arrow/compute/kernels/scalar_arithmetic.cc:113:84: note: in instantiation of member function 'arrow::compute::codegen::ScalarBinary::Exec' requested here + // ArrayKernelExec exec = codegen::ScalarBinaryEqualTypes::Exec; + // ^ + // ../src/arrow/compute/kernels/scalar_arithmetic.cc:129:12: note: in instantiation of function template specialization 'arrow::compute::codegen::MakeBinaryFunction2' requested here + // codegen::MakeBinaryFunction2("add", registry); + // ^ + // ../src/arrow/compute/kernels/scalar_arithmetic.cc:25:24: note: candidate template ignored: couldn't infer template argument 'OUT' + // static constexpr OUT Call(KernelContext*, ARG0 left, ARG1 right) { + // + ^ // codegen::MakeBinaryFunction2("add", registry); } From 0a44c14ebd6fb4d6932221ec8fa1ca13a79f624e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Wed, 3 Jun 2020 00:55:05 +0200 Subject: [PATCH 06/23] left char behind --- cpp/src/arrow/compute/kernels/scalar_arithmetic.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index bb06bfc080c..ae6fdbfdb63 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -145,7 +145,7 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) { // ../src/arrow/compute/kernels/scalar_arithmetic.cc:25:24: note: candidate template ignored: couldn't infer template argument 'OUT' // static constexpr OUT Call(KernelContext*, ARG0 left, ARG1 right) { // - ^ + // codegen::MakeBinaryFunction2("add", registry); } From 24835c2a23666e27a95ed8340e97cb5c5d8d42d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Wed, 3 Jun 2020 13:26:14 +0200 Subject: [PATCH 07/23] fix tests and use ScalarBinary exec function to support multiple input shapes --- .../arrow/compute/kernels/codegen_internal.h | 21 --- .../compute/kernels/scalar_arithmetic.cc | 133 ++++-------------- .../compute/kernels/scalar_arithmetic_test.cc | 45 ++---- 3 files changed, 42 insertions(+), 157 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/codegen_internal.h b/cpp/src/arrow/compute/kernels/codegen_internal.h index e69dfc101ae..ffbdcb2f008 100644 --- a/cpp/src/arrow/compute/kernels/codegen_internal.h +++ b/cpp/src/arrow/compute/kernels/codegen_internal.h @@ -337,25 +337,6 @@ void ScalarPrimitiveExecUnary(KernelContext* ctx, const ExecBatch& batch, Datum* } } -template -void ScalarPrimitiveExecBinary(KernelContext* ctx, const ExecBatch& batch, Datum* out) { - using OUT = typename OutType::c_type; - using ARG0 = typename Arg0Type::c_type; - using ARG1 = typename Arg1Type::c_type; - - if (batch[0].kind() == Datum::SCALAR || batch[1].kind() == Datum::SCALAR) { - ctx->SetStatus(Status::NotImplemented("NYI")); - } else { - ArrayData* out_arr = out->mutable_array(); - auto out_data = out_arr->GetMutableValues(1); - auto arg0_data = batch[0].array()->GetValues(1); - auto arg1_data = batch[1].array()->GetValues(1); - for (int64_t i = 0; i < batch.length; ++i) { - *out_data++ = Op::template Call(ctx, *arg0_data++, *arg1_data++); - } - } -} - // OutputAdapter allows passing an inlineable lambda that provides a sequence // of output values to write into output memory. Boolean and primitive outputs // are currently implemented, and the validity bitmap is presumed to be handled @@ -619,8 +600,6 @@ struct ScalarBinary { static void ArrayArray(KernelContext* ctx, const ExecBatch& batch, Datum* out) { ArrayIterator arg0(*batch[0].array()); ArrayIterator arg1(*batch[1].array()); - // I don't get it why it is unable to deduce the OUT type of Call since it is - // explicitly defined as the return type of the lambda. OutputAdapter::Write(ctx, out, [&]() -> OUT { return Op::template Call(ctx, arg0(), arg1()); }); diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index ae6fdbfdb63..cee1ac6ceae 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -21,101 +21,44 @@ namespace arrow { namespace compute { struct Add { - template - static constexpr OUT Call(KernelContext*, ARG0 left, ARG1 right) { + template + static constexpr auto Call(KernelContext*, ARG0 left, ARG1 right) + -> decltype(left + right) { return left + right; } }; namespace codegen { -template -ArrayKernelExec GetBinaryExec(detail::GetTypeId get_id) { - // These execution functions doesn't support operations between multiple shapes, - // only op(array like, array like) is supported. - // Note that we don't have plans to add binary kernels with differntly types - // lhs and rhs arguments, that will be handled with implicit casts during the - // kernel dispatch procedure. - // - // There is a ScalarBinary facility in the codegen_internals.h which already - // handles those cases. I illustrate my plan MakeBinaryFunction2 by trying to - // add a single kernel to the function. - switch (get_id.id) { - case Type::INT8: - return ScalarPrimitiveExecBinary; - case Type::UINT8: - return ScalarPrimitiveExecBinary; - case Type::INT16: - return ScalarPrimitiveExecBinary; - case Type::UINT16: - return ScalarPrimitiveExecBinary; - case Type::INT32: - return ScalarPrimitiveExecBinary; - case Type::UINT32: - return ScalarPrimitiveExecBinary; - case Type::INT64: - return ScalarPrimitiveExecBinary; - case Type::UINT64: - return ScalarPrimitiveExecBinary; - case Type::FLOAT: - return ScalarPrimitiveExecBinary; - case Type::DOUBLE: - return ScalarPrimitiveExecBinary; - default: - DCHECK(false); - return ExecFail; - } -} - -std::shared_ptr GetOutputType(detail::GetTypeId get_id) { - switch (get_id.id) { - case Type::INT8: - return int16(); - case Type::INT16: - return int32(); - case Type::INT32: - case Type::INT64: - return int64(); - case Type::UINT8: - return uint16(); - case Type::UINT16: - return uint32(); - case Type::UINT32: - case Type::UINT64: - return uint64(); - case Type::FLOAT: - return float32(); - case Type::DOUBLE: - return float64(); - default: - DCHECK(false); - return nullptr; - } -} - -template -void MakeBinaryFunction(std::string name, FunctionRegistry* registry) { - auto func = std::make_shared(name, Arity::Binary()); - for (const std::shared_ptr& input_type : NumericTypes()) { - DCHECK_OK( - func->AddKernel({InputType::Array(input_type), InputType::Array(input_type)}, - GetOutputType(input_type), GetBinaryExec(*input_type))); - } - DCHECK_OK(registry->AddFunction(std::move(func))); +template +void AddBinaryKernel(const std::shared_ptr& func) { + // create an exec function with the requested signature + ArrayKernelExec exec = ScalarBinaryEqualTypes::Exec; + // create type objects based on the template arguments + auto arg = TypeTraits::type_singleton(); + auto out = TypeTraits::type_singleton(); + // add the exec function as a kernel with the appropiate signature + DCHECK_OK(func->AddKernel({arg, arg}, out, exec)); } -// Here is the function which doesn't compile because of type deduction error template -void MakeBinaryFunction2(std::string name, FunctionRegistry* registry) { +void AddBinaryFunction(std::string name, FunctionRegistry* registry) { auto func = std::make_shared(name, Arity::Binary()); - // create an exec function with signature (int8, int8) -> int16 - ArrayKernelExec exec = codegen::ScalarBinaryEqualTypes::Exec; + // signed integers + AddBinaryKernel(func); + AddBinaryKernel(func); + AddBinaryKernel(func); + AddBinaryKernel(func); + // unsigned integers + AddBinaryKernel(func); + AddBinaryKernel(func); + AddBinaryKernel(func); + AddBinaryKernel(func); + // floating-point types, TODO(kszucs): add half-float + AddBinaryKernel(func); + AddBinaryKernel(func); - // add this exec function as a kernel with the appropiate signature - DCHECK_OK(func->AddKernel({int8(), int8()}, int16(), exec)); - - // finally register the function, but the DCHECK_OK(registry->AddFunction(std::move(func))); } @@ -124,29 +67,7 @@ void MakeBinaryFunction2(std::string name, FunctionRegistry* registry) { namespace internal { void RegisterScalarArithmetic(FunctionRegistry* registry) { - codegen::MakeBinaryFunction("add", registry); - - // Uncommenting the line below raises the following compiler error: - // - // In file included from ../src/arrow/compute/kernels/scalar_arithmetic.cc:18: - // In file included from ../src/arrow/compute/kernels/common.h:31: - // ../src/arrow/compute/kernels/codegen_internal.h:537:16: error: no matching function for call to 'Call' - // return Op::template Call(ctx, arg0(), arg1()); - // ^~~~~~~~~~~~~~~~~ - // ../src/arrow/compute/kernels/codegen_internal.h:567:16: note: in instantiation of member function 'arrow::compute::codegen::ScalarBinary::ArrayArray' requested here - // return ArrayArray(ctx, batch, out); - // ^ - // ../src/arrow/compute/kernels/scalar_arithmetic.cc:113:84: note: in instantiation of member function 'arrow::compute::codegen::ScalarBinary::Exec' requested here - // ArrayKernelExec exec = codegen::ScalarBinaryEqualTypes::Exec; - // ^ - // ../src/arrow/compute/kernels/scalar_arithmetic.cc:129:12: note: in instantiation of function template specialization 'arrow::compute::codegen::MakeBinaryFunction2' requested here - // codegen::MakeBinaryFunction2("add", registry); - // ^ - // ../src/arrow/compute/kernels/scalar_arithmetic.cc:25:24: note: candidate template ignored: couldn't infer template argument 'OUT' - // static constexpr OUT Call(KernelContext*, ARG0 left, ARG1 right) { - // - - // codegen::MakeBinaryFunction2("add", registry); + codegen::AddBinaryFunction("add", 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 a1520fb7120..9c5edbfefcb 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -51,10 +51,6 @@ class TestBinaryArithmetics> : public TestBase { using BinaryFunction = std::function(const Datum&, const Datum&, ExecContext*)>; - static InputCType GetMin() { return std::numeric_limits::min(); } - - static InputCType GetMax() { return std::numeric_limits::max(); } - std::shared_ptr MakeInputArray(const std::vector& values) { std::shared_ptr out; ArrayFromVector(values, &out); @@ -92,18 +88,15 @@ template class TestBinaryArithmeticsFloating : public TestBinaryArithmetics {}; // InputType - OutputType pairs -using IntegralPairs = testing::Types< - // std::pair, - std::pair, std::pair, - std::pair, - // std::pair, - // std::pair, - std::pair, std::pair>; +using IntegralPairs = + testing::Types, std::pair, + std::pair, std::pair, + std::pair, std::pair, + std::pair, std::pair>; -// InputType - OutputType pairs -using FloatingPairs = testing::Types< - // std::pair, - std::pair, std::pair>; +// InputType - OutputType pairs, TODO(kszucs): add half-float +using FloatingPairs = + testing::Types, std::pair>; TYPED_TEST_SUITE(TestBinaryArithmeticsIntegral, IntegralPairs); TYPED_TEST_SUITE(TestBinaryArithmeticsFloating, FloatingPairs); @@ -126,25 +119,17 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { "[null, 5, 5, null, 2, 8]"); } -// If I uncomment the commented out signed integer pairs above then clang gives me the -// following warning: -// -// ../src/arrow/compute/kernels/scalar_arithmetic_test.cc:150:64: note: insert an explicit -// cast to silence this issue -// auto expected = this->MakeOutputArray({1, 12, 14, min + min, max + max}); -// ^~~~~~~~~ -// static_cast( ) -// -// Since I don't really access the types within this typed test, I assume I need a -// different approach? TYPED_TEST(TestBinaryArithmeticsIntegral, AddCheckExtremes) { - auto min = this->GetMin(); - auto max = this->GetMax(); + using InputCType = typename TestFixture::InputCType; + using OutputCType = typename TestFixture::OutputCType; + + auto min = std::numeric_limits::min(); + auto max = std::numeric_limits::max(); auto left = this->MakeInputArray({1, 2, 3, min, max}); auto right = this->MakeInputArray({0, 10, 11, min, max}); - auto expected = this->MakeOutputArray({1, 12, 14, min + min, max + max}); + auto expected = this->MakeOutputArray({1, 12, 14, static_cast(min + min), + static_cast(max + max)}); this->AssertBinop(arrow::compute::Add, left, right, expected); } From b75327c69c5527442e098d70a25cbfe86ab68865 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Wed, 3 Jun 2020 16:05:24 +0200 Subject: [PATCH 08/23] specialize Add operation explicitly for smaller integer types, please ubsan --- .../compute/kernels/scalar_arithmetic.cc | 37 +++++++++++++++++-- .../compute/kernels/scalar_arithmetic_test.cc | 13 ++++++- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index cee1ac6ceae..f04d0fb72b2 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -21,10 +21,39 @@ namespace arrow { namespace compute { struct Add { - template - static constexpr auto Call(KernelContext*, ARG0 left, ARG1 right) - -> decltype(left + right) { - return left + right; + template + static constexpr OUT Call(KernelContext*, int8_t l, int8_t r) { + return static_cast(l) + static_cast(r); + } + + template + static constexpr OUT Call(KernelContext*, int16_t l, int16_t r) { + return static_cast(l) + static_cast(r); + } + + template + static constexpr OUT Call(KernelContext*, int32_t l, int32_t r) { + return static_cast(l) + static_cast(r); + } + + template + static constexpr OUT Call(KernelContext*, uint8_t l, uint8_t r) { + return static_cast(l) + static_cast(r); + } + + template + static constexpr OUT Call(KernelContext*, uint16_t l, uint16_t r) { + return static_cast(l) + static_cast(r); + } + + template + static constexpr OUT Call(KernelContext*, uint32_t l, uint32_t r) { + return static_cast(l) + static_cast(r); + } + + template + static constexpr OUT Call(KernelContext*, OUT l, OUT r) { + return l + r; } }; diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 9c5edbfefcb..2a5d9fdd5f6 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -123,13 +123,22 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, AddCheckExtremes) { using InputCType = typename TestFixture::InputCType; using OutputCType = typename TestFixture::OutputCType; + if (std::is_same::value) { + // this test case is incompatible with Int64 and UInt64 types because there + // are no wider integer types to overflow to + return; + } + auto min = std::numeric_limits::min(); auto max = std::numeric_limits::max(); auto left = this->MakeInputArray({1, 2, 3, min, max}); auto right = this->MakeInputArray({0, 10, 11, min, max}); - auto expected = this->MakeOutputArray({1, 12, 14, static_cast(min + min), - static_cast(max + max)}); + + OutputCType expected_min = 2 * static_cast(min); + OutputCType expected_max = 2 * static_cast(max); + + auto expected = this->MakeOutputArray({1, 12, 14, expected_min, expected_max}); this->AssertBinop(arrow::compute::Add, left, right, expected); } From b5bb26f312e3fb75fc78950951e991c5e991f764 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Thu, 4 Jun 2020 16:11:51 +0200 Subject: [PATCH 09/23] unchecked add, sub, mul --- cpp/src/arrow/compute/api_scalar.cc | 2 + cpp/src/arrow/compute/api_scalar.h | 20 ++++ .../arrow/compute/kernels/codegen_internal.h | 40 ++++++- .../compute/kernels/scalar_arithmetic.cc | 82 +++++-------- .../compute/kernels/scalar_arithmetic_test.cc | 111 ++++++++++++++---- 5 files changed, 173 insertions(+), 82 deletions(-) diff --git a/cpp/src/arrow/compute/api_scalar.cc b/cpp/src/arrow/compute/api_scalar.cc index 86ef74946df..842aed2bf76 100644 --- a/cpp/src/arrow/compute/api_scalar.cc +++ b/cpp/src/arrow/compute/api_scalar.cc @@ -42,6 +42,8 @@ namespace compute { // Arithmetic SCALAR_EAGER_BINARY(Add, "add") +SCALAR_EAGER_BINARY(Sub, "sub") +SCALAR_EAGER_BINARY(Mul, "mul") // ---------------------------------------------------------------------- // Set-related operations diff --git a/cpp/src/arrow/compute/api_scalar.h b/cpp/src/arrow/compute/api_scalar.h index e3210d3ac91..33a35e3b17e 100644 --- a/cpp/src/arrow/compute/api_scalar.h +++ b/cpp/src/arrow/compute/api_scalar.h @@ -45,6 +45,26 @@ namespace compute { ARROW_EXPORT Result Add(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); +/// \brief Subtract two values. Array values must be the same length. If a +/// value is null in either addend, the result is null +/// +/// \param[in] left the first value +/// \param[in] right the second value +/// \param[in] ctx the function execution context, optional +/// \return the elementwise addition of the values +ARROW_EXPORT +Result Sub(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); + +/// \brief Multiply two values. Array values must be the same length. If a +/// value is null in either addend, the result is null +/// +/// \param[in] left the first value +/// \param[in] right the second value +/// \param[in] ctx the function execution context, optional +/// \return the elementwise addition of the values +ARROW_EXPORT +Result Mul(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); + enum CompareOperator { EQUAL, NOT_EQUAL, diff --git a/cpp/src/arrow/compute/kernels/codegen_internal.h b/cpp/src/arrow/compute/kernels/codegen_internal.h index ffbdcb2f008..b019a9f57db 100644 --- a/cpp/src/arrow/compute/kernels/codegen_internal.h +++ b/cpp/src/arrow/compute/kernels/codegen_internal.h @@ -648,8 +648,7 @@ struct ScalarBinary { // A kernel exec generator for binary kernels where both input types are the // same -template +template using ScalarBinaryEqualTypes = ScalarBinary; // ---------------------------------------------------------------------- @@ -710,6 +709,43 @@ ArrayKernelExec NumericEqualTypesUnary(detail::GetTypeId get_id) { } } +// Generate a kernel given a functor of type +// +// struct OPERATOR_NAME { +// template +// static OUT Call(KernelContext*, ARG0 left, ARG1 right) { +// // IMPLEMENTATION +// } +// }; +template +ArrayKernelExec NumericEqualTypesBinary(detail::GetTypeId get_id) { + switch (get_id.id) { + case Type::INT8: + return ScalarBinaryEqualTypes::Exec; + case Type::UINT8: + return ScalarBinaryEqualTypes::Exec; + case Type::INT16: + return ScalarBinaryEqualTypes::Exec; + case Type::UINT16: + return ScalarBinaryEqualTypes::Exec; + case Type::INT32: + return ScalarBinaryEqualTypes::Exec; + case Type::UINT32: + return ScalarBinaryEqualTypes::Exec; + case Type::INT64: + return ScalarBinaryEqualTypes::Exec; + case Type::UINT64: + return ScalarBinaryEqualTypes::Exec; + case Type::FLOAT: + return ScalarBinaryEqualTypes::Exec; + case Type::DOUBLE: + return ScalarBinaryEqualTypes::Exec; + default: + DCHECK(false); + return ExecFail; + } +} + // Generate a kernel given a templated functor. This template effectively // "curries" the first type argument. The functor must be of the form: // diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index f04d0fb72b2..71951aa7d11 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -16,78 +16,42 @@ // under the License. #include "arrow/compute/kernels/common.h" +#include "arrow/util/int_util.h" +#include "iostream" namespace arrow { namespace compute { struct Add { - template - static constexpr OUT Call(KernelContext*, int8_t l, int8_t r) { - return static_cast(l) + static_cast(r); - } - - template - static constexpr OUT Call(KernelContext*, int16_t l, int16_t r) { - return static_cast(l) + static_cast(r); - } - - template - static constexpr OUT Call(KernelContext*, int32_t l, int32_t r) { - return static_cast(l) + static_cast(r); - } - - template - static constexpr OUT Call(KernelContext*, uint8_t l, uint8_t r) { - return static_cast(l) + static_cast(r); - } - - template - static constexpr OUT Call(KernelContext*, uint16_t l, uint16_t r) { - return static_cast(l) + static_cast(r); + template + static constexpr T Call(KernelContext*, T left, T right) { + return left - right; } +}; - template - static constexpr OUT Call(KernelContext*, uint32_t l, uint32_t r) { - return static_cast(l) + static_cast(r); +struct Sub { + template + static constexpr T Call(KernelContext*, T left, T right) { + return left - right; } +}; - template - static constexpr OUT Call(KernelContext*, OUT l, OUT r) { - return l + r; +struct Mul { + template + static constexpr T Call(KernelContext*, T left, T right) { + return left * right; } }; namespace codegen { -template -void AddBinaryKernel(const std::shared_ptr& func) { - // create an exec function with the requested signature - ArrayKernelExec exec = ScalarBinaryEqualTypes::Exec; - // create type objects based on the template arguments - auto arg = TypeTraits::type_singleton(); - auto out = TypeTraits::type_singleton(); - // add the exec function as a kernel with the appropiate signature - DCHECK_OK(func->AddKernel({arg, arg}, out, exec)); -} - template void AddBinaryFunction(std::string name, FunctionRegistry* registry) { auto func = std::make_shared(name, Arity::Binary()); - - // signed integers - AddBinaryKernel(func); - AddBinaryKernel(func); - AddBinaryKernel(func); - AddBinaryKernel(func); - // unsigned integers - AddBinaryKernel(func); - AddBinaryKernel(func); - AddBinaryKernel(func); - AddBinaryKernel(func); - // floating-point types, TODO(kszucs): add half-float - AddBinaryKernel(func); - AddBinaryKernel(func); - + for (const auto& ty : NumericTypes()) { + auto exec = codegen::NumericEqualTypesBinary(ty); + DCHECK_OK(func->AddKernel({ty, ty}, ty, exec)); + } DCHECK_OK(registry->AddFunction(std::move(func))); } @@ -97,8 +61,16 @@ namespace internal { void RegisterScalarArithmetic(FunctionRegistry* registry) { codegen::AddBinaryFunction("add", registry); + codegen::AddBinaryFunction("sub", registry); + codegen::AddBinaryFunction("mul", registry); } } // namespace internal } // namespace compute } // namespace arrow + + +// add, checked_add, safe_add +// ne legyen ertelmeze sub unsigned tipusokon +// unsigned arithmetikat hasznalni signed muveletekhez hogy az undefined behaviourt elkeruljuk +// diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 2a5d9fdd5f6..77b16bc3c3a 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -84,34 +84,47 @@ class TestBinaryArithmetics> : public TestBase { template class TestBinaryArithmeticsIntegral : public TestBinaryArithmetics {}; +template +class TestBinaryArithmeticsSigned : public TestBinaryArithmeticsIntegral {}; + +// template +// class TestBinaryArithmeticsUnsigned : public TestBinaryArithmeticsIntegral {}; + template class TestBinaryArithmeticsFloating : public TestBinaryArithmetics {}; // InputType - OutputType pairs using IntegralPairs = - testing::Types, std::pair, - std::pair, std::pair, - std::pair, std::pair, - std::pair, std::pair>; + testing::Types, std::pair, + std::pair, std::pair, + std::pair, std::pair, + std::pair, std::pair>; + +using SignedIntegerPairs = + testing::Types, std::pair, + std::pair, std::pair>; -// InputType - OutputType pairs, TODO(kszucs): add half-float +// using UnsignedIntegerPairs = +// testing::Types, std::pair, +// std::pair, std::pair>; + +// TODO(kszucs): add half-float using FloatingPairs = testing::Types, std::pair>; TYPED_TEST_SUITE(TestBinaryArithmeticsIntegral, IntegralPairs); +TYPED_TEST_SUITE(TestBinaryArithmeticsSigned, SignedIntegerPairs); +// TYPED_TEST_SUITE(TestBinaryArithmeticsSigned, UnsignedIntegerPairs); TYPED_TEST_SUITE(TestBinaryArithmeticsFloating, FloatingPairs); TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { this->AssertBinop(arrow::compute::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, "[7, 6, 5, 4, 3, 2, 1]", "[6, 5, 4, 3, 2, 1, 0]", - "[13, 11, 9, 7, 5, 3, 1]"); - 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]"); @@ -119,30 +132,78 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { "[null, 5, 5, null, 2, 8]"); } -TYPED_TEST(TestBinaryArithmeticsIntegral, AddCheckExtremes) { - using InputCType = typename TestFixture::InputCType; - using OutputCType = typename TestFixture::OutputCType; +TYPED_TEST(TestBinaryArithmeticsIntegral, Sub) { + this->AssertBinop(arrow::compute::Sub, "[]", "[]", "[]"); + this->AssertBinop(arrow::compute::Sub, "[null]", "[null]", "[null]"); + this->AssertBinop(arrow::compute::Sub, "[3, 2, 6]", "[1, 0, 2]", "[2, 2, 4]"); - if (std::is_same::value) { - // this test case is incompatible with Int64 and UInt64 types because there - // are no wider integer types to overflow to - return; - } + this->AssertBinop(arrow::compute::Sub, "[1, 2, 3, 4, 5, 6, 7]", "[0, 1, 2, 3, 4, 5, 6]", + "[1, 1, 1, 1, 1, 1, 1]"); +} + +TYPED_TEST(TestBinaryArithmeticsIntegral, Mul) { + this->AssertBinop(arrow::compute::Mul, "[]", "[]", "[]"); + this->AssertBinop(arrow::compute::Mul, "[null]", "[null]", "[null]"); + this->AssertBinop(arrow::compute::Mul, "[3, 2, 6]", "[1, 0, 2]", "[3, 0, 12]"); + + this->AssertBinop(arrow::compute::Mul, "[1, 2, 3, 4, 5, 6, 7]", "[0, 1, 2, 3, 4, 5, 6]", + "[0, 2, 6, 12, 20, 30, 42]"); - auto min = std::numeric_limits::min(); - auto max = std::numeric_limits::max(); + this->AssertBinop(arrow::compute::Mul, "[7, 6, 5, 4, 3, 2, 1]", "[6, 5, 4, 3, 2, 1, 0]", + "[42, 30, 20, 12, 6, 2, 0]"); - auto left = this->MakeInputArray({1, 2, 3, min, max}); - auto right = this->MakeInputArray({0, 10, 11, min, max}); + this->AssertBinop(arrow::compute::Mul, "[null, 1, 3, null, 2, 5]", "[1, 4, 2, 5, 0, 3]", + "[null, 4, 6, null, 0, 15]"); +} + +TYPED_TEST(TestBinaryArithmeticsSigned, Add) { + this->AssertBinop(arrow::compute::Add, "[-7, 6, 5, 4, 3, 2, 1]", "[-6, 5, -4, 3, -2, 1, 0]", + "[-13, 11, 1, 7, 1, 3, 1]"); +} + +TYPED_TEST(TestBinaryArithmeticsSigned, Sub) { + this->AssertBinop(arrow::compute::Sub, "[0, 1, 2, 3, 4, 5, 6]", "[1, 2, 3, 4, 5, 6, 7]", + "[-1, -1, -1, -1, -1, -1, -1]"); - OutputCType expected_min = 2 * static_cast(min); - OutputCType expected_max = 2 * static_cast(max); + this->AssertBinop(arrow::compute::Sub, "[0, 0, 0, 0, 0, 0, 0]", "[6, 5, 4, 3, 2, 1, 0]", + "[-6, -5, -4, -3, -2, -1, 0]"); - auto expected = this->MakeOutputArray({1, 12, 14, expected_min, expected_max}); + this->AssertBinop(arrow::compute::Sub, "[10, 12, 4, 50, 50, 32, 11]", + "[2, 0, 6, 1, 5, 3, 4]", "[8, 12, -2, 49, 45, 29, 7]"); - this->AssertBinop(arrow::compute::Add, left, right, expected); + this->AssertBinop(arrow::compute::Sub, "[null, 1, 3, null, 2, 5]", "[1, 4, 2, 5, 0, 3]", + "[null, -3, 1, null, 2, 2]"); } +TYPED_TEST(TestBinaryArithmeticsSigned, Mul) { + this->AssertBinop(arrow::compute::Mul, "[-10, 12, 4, 50, -5, 32, 11]", + "[-2, 0, -6, 1, 5, 3, 4]", "[20, 0, -24, 50, -25, 96, 44]"); +} + +// TYPED_TEST(TestBinaryArithmeticsIntegral, AddCheckExtremes) { +// using InputCType = typename TestFixture::InputCType; +// using OutputCType = typename TestFixture::OutputCType; + +// if (std::is_same::value) { +// // this test case is incompatible with Int64 and UInt64 types because there +// // are no wider integer types to overflow to +// return; +// } + +// auto min = std::numeric_limits::min(); +// auto max = std::numeric_limits::max(); + +// auto left = this->MakeInputArray({1, 2, 3, min, max}); +// auto right = this->MakeInputArray({0, 10, 11, min, max}); + +// OutputCType expected_min = 2 * static_cast(min); +// OutputCType expected_max = 2 * static_cast(max); + +// auto expected = this->MakeOutputArray({1, 12, 14, expected_min, expected_max}); + +// this->AssertBinop(arrow::compute::Add, left, right, expected); +// } + TYPED_TEST(TestBinaryArithmeticsFloating, Add) { this->AssertBinop(arrow::compute::Add, "[]", "[]", "[]"); From 82b8dd076c3a9676b459621791b2f359d3db3a57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Thu, 4 Jun 2020 16:13:17 +0200 Subject: [PATCH 10/23] format, remove comments --- cpp/src/arrow/compute/kernels/scalar_arithmetic.cc | 6 ------ .../arrow/compute/kernels/scalar_arithmetic_test.cc | 10 ++++++---- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 71951aa7d11..095af858297 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -68,9 +68,3 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) { } // namespace internal } // namespace compute } // namespace arrow - - -// add, checked_add, safe_add -// ne legyen ertelmeze sub unsigned tipusokon -// unsigned arithmetikat hasznalni signed muveletekhez hogy az undefined behaviourt elkeruljuk -// diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 77b16bc3c3a..f566a950835 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -88,7 +88,8 @@ template class TestBinaryArithmeticsSigned : public TestBinaryArithmeticsIntegral {}; // template -// class TestBinaryArithmeticsUnsigned : public TestBinaryArithmeticsIntegral {}; +// class TestBinaryArithmeticsUnsigned : public TestBinaryArithmeticsIntegral +// {}; template class TestBinaryArithmeticsFloating : public TestBinaryArithmetics {}; @@ -106,7 +107,8 @@ using SignedIntegerPairs = // using UnsignedIntegerPairs = // testing::Types, std::pair, -// std::pair, std::pair>; +// std::pair, std::pair>; // TODO(kszucs): add half-float using FloatingPairs = @@ -157,8 +159,8 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Mul) { } TYPED_TEST(TestBinaryArithmeticsSigned, Add) { - this->AssertBinop(arrow::compute::Add, "[-7, 6, 5, 4, 3, 2, 1]", "[-6, 5, -4, 3, -2, 1, 0]", - "[-13, 11, 1, 7, 1, 3, 1]"); + this->AssertBinop(arrow::compute::Add, "[-7, 6, 5, 4, 3, 2, 1]", + "[-6, 5, -4, 3, -2, 1, 0]", "[-13, 11, 1, 7, 1, 3, 1]"); } TYPED_TEST(TestBinaryArithmeticsSigned, Sub) { From af3e452c5530652e7bf675e785ead97583be8ad8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Thu, 4 Jun 2020 16:44:07 +0200 Subject: [PATCH 11/23] fix op --- cpp/src/arrow/compute/kernels/scalar_arithmetic.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 095af858297..5d2de94a3a2 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -25,7 +25,7 @@ namespace compute { struct Add { template static constexpr T Call(KernelContext*, T left, T right) { - return left - right; + return left + right; } }; From 6d9924b94215b749439a40f4cb8e50da46299abc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Thu, 4 Jun 2020 19:54:42 +0200 Subject: [PATCH 12/23] failing ubsan test for signed integer overflow --- .../arrow/compute/kernels/scalar_arithmetic_test.cc | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index f566a950835..910837dd475 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -163,6 +163,19 @@ TYPED_TEST(TestBinaryArithmeticsSigned, Add) { "[-6, 5, -4, 3, -2, 1, 0]", "[-13, 11, 1, 7, 1, 3, 1]"); } +TYPED_TEST(TestBinaryArithmeticsSigned, AddOverflow) { + using InputCType = typename TestFixture::InputCType; + + auto min = std::numeric_limits::min(); + auto max = std::numeric_limits::max(); + + auto left = this->MakeInputArray({max}); + auto right = this->MakeInputArray({1}); + auto expected = this->MakeOutputArray({min}); + + this->AssertBinop(arrow::compute::Add, left, right, expected); +} + TYPED_TEST(TestBinaryArithmeticsSigned, Sub) { this->AssertBinop(arrow::compute::Sub, "[0, 1, 2, 3, 4, 5, 6]", "[1, 2, 3, 4, 5, 6, 7]", "[-1, -1, -1, -1, -1, -1, -1]"); From 744c27db4dddad107a428c8f2c40cfd221979194 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Thu, 4 Jun 2020 20:31:02 +0200 Subject: [PATCH 13/23] avoid undefined signed integer overflow --- .../arrow/compute/kernels/codegen_internal.h | 37 ------------------- .../compute/kernels/scalar_arithmetic.cc | 34 +++++++++++++++++ .../compute/kernels/scalar_arithmetic_test.cc | 31 ++-------------- 3 files changed, 38 insertions(+), 64 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/codegen_internal.h b/cpp/src/arrow/compute/kernels/codegen_internal.h index b019a9f57db..f4e2a8f9ca8 100644 --- a/cpp/src/arrow/compute/kernels/codegen_internal.h +++ b/cpp/src/arrow/compute/kernels/codegen_internal.h @@ -709,43 +709,6 @@ ArrayKernelExec NumericEqualTypesUnary(detail::GetTypeId get_id) { } } -// Generate a kernel given a functor of type -// -// struct OPERATOR_NAME { -// template -// static OUT Call(KernelContext*, ARG0 left, ARG1 right) { -// // IMPLEMENTATION -// } -// }; -template -ArrayKernelExec NumericEqualTypesBinary(detail::GetTypeId get_id) { - switch (get_id.id) { - case Type::INT8: - return ScalarBinaryEqualTypes::Exec; - case Type::UINT8: - return ScalarBinaryEqualTypes::Exec; - case Type::INT16: - return ScalarBinaryEqualTypes::Exec; - case Type::UINT16: - return ScalarBinaryEqualTypes::Exec; - case Type::INT32: - return ScalarBinaryEqualTypes::Exec; - case Type::UINT32: - return ScalarBinaryEqualTypes::Exec; - case Type::INT64: - return ScalarBinaryEqualTypes::Exec; - case Type::UINT64: - return ScalarBinaryEqualTypes::Exec; - case Type::FLOAT: - return ScalarBinaryEqualTypes::Exec; - case Type::DOUBLE: - return ScalarBinaryEqualTypes::Exec; - default: - DCHECK(false); - return ExecFail; - } -} - // Generate a kernel given a templated functor. This template effectively // "curries" the first type argument. The functor must be of the form: // diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 5d2de94a3a2..c56704898bf 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -45,6 +45,40 @@ struct Mul { namespace codegen { +// Generate a kernel given an arithmetic functor +// +// To avoid undefined behaviour of signed integer overflow treat the signed +// input argument values as unsigned then cast them to signed making them wrap +// around. +template +ArrayKernelExec NumericEqualTypesBinary(detail::GetTypeId get_id) { + switch (get_id.id) { + case Type::INT8: + return ScalarBinaryEqualTypes::Exec; + case Type::UINT8: + return ScalarBinaryEqualTypes::Exec; + case Type::INT16: + return ScalarBinaryEqualTypes::Exec; + case Type::UINT16: + return ScalarBinaryEqualTypes::Exec; + case Type::INT32: + return ScalarBinaryEqualTypes::Exec; + case Type::UINT32: + return ScalarBinaryEqualTypes::Exec; + case Type::INT64: + return ScalarBinaryEqualTypes::Exec; + case Type::UINT64: + return ScalarBinaryEqualTypes::Exec; + case Type::FLOAT: + return ScalarBinaryEqualTypes::Exec; + case Type::DOUBLE: + return ScalarBinaryEqualTypes::Exec; + default: + DCHECK(false); + return ExecFail; + } +} + template void AddBinaryFunction(std::string name, FunctionRegistry* registry) { auto func = std::make_shared(name, Arity::Binary()); diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 910837dd475..32a3c98d7c2 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -16,6 +16,7 @@ // under the License. #include +#include #include #include #include @@ -169,9 +170,9 @@ TYPED_TEST(TestBinaryArithmeticsSigned, AddOverflow) { auto min = std::numeric_limits::min(); auto max = std::numeric_limits::max(); - auto left = this->MakeInputArray({max}); - auto right = this->MakeInputArray({1}); - auto expected = this->MakeOutputArray({min}); + auto left = this->MakeInputArray({max, min}); + auto right = this->MakeInputArray({1, -1}); + auto expected = this->MakeOutputArray({min, max}); this->AssertBinop(arrow::compute::Add, left, right, expected); } @@ -195,30 +196,6 @@ TYPED_TEST(TestBinaryArithmeticsSigned, Mul) { "[-2, 0, -6, 1, 5, 3, 4]", "[20, 0, -24, 50, -25, 96, 44]"); } -// TYPED_TEST(TestBinaryArithmeticsIntegral, AddCheckExtremes) { -// using InputCType = typename TestFixture::InputCType; -// using OutputCType = typename TestFixture::OutputCType; - -// if (std::is_same::value) { -// // this test case is incompatible with Int64 and UInt64 types because there -// // are no wider integer types to overflow to -// return; -// } - -// auto min = std::numeric_limits::min(); -// auto max = std::numeric_limits::max(); - -// auto left = this->MakeInputArray({1, 2, 3, min, max}); -// auto right = this->MakeInputArray({0, 10, 11, min, max}); - -// OutputCType expected_min = 2 * static_cast(min); -// OutputCType expected_max = 2 * static_cast(max); - -// auto expected = this->MakeOutputArray({1, 12, 14, expected_min, expected_max}); - -// this->AssertBinop(arrow::compute::Add, left, right, expected); -// } - TYPED_TEST(TestBinaryArithmeticsFloating, Add) { this->AssertBinop(arrow::compute::Add, "[]", "[]", "[]"); From c12ca4fa4eb7a4205287dec3259f911a0202b1bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Thu, 4 Jun 2020 21:58:58 +0200 Subject: [PATCH 14/23] more overflow tests --- .../compute/kernels/scalar_arithmetic.cc | 11 ++- .../compute/kernels/scalar_arithmetic_test.cc | 67 +++++++++++++++---- 2 files changed, 62 insertions(+), 16 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index c56704898bf..acb3b6ce4b5 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -22,22 +22,27 @@ namespace arrow { namespace compute { +// explicitly disallow signed integers as input arguments +template +using enable_if_unsigned_or_floating = + enable_if_t::value || std::is_floating_point::value>; + struct Add { - template + template > static constexpr T Call(KernelContext*, T left, T right) { return left + right; } }; struct Sub { - template + template > static constexpr T Call(KernelContext*, T left, T right) { return left - right; } }; struct Mul { - template + template > static constexpr T Call(KernelContext*, T left, T right) { return left * right; } diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 32a3c98d7c2..34dd2b2addb 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -88,9 +88,8 @@ class TestBinaryArithmeticsIntegral : public TestBinaryArithmetics {}; template class TestBinaryArithmeticsSigned : public TestBinaryArithmeticsIntegral {}; -// template -// class TestBinaryArithmeticsUnsigned : public TestBinaryArithmeticsIntegral -// {}; +template +class TestBinaryArithmeticsUnsigned : public TestBinaryArithmeticsIntegral {}; template class TestBinaryArithmeticsFloating : public TestBinaryArithmetics {}; @@ -106,10 +105,9 @@ using SignedIntegerPairs = testing::Types, std::pair, std::pair, std::pair>; -// using UnsignedIntegerPairs = -// testing::Types, std::pair, -// std::pair, std::pair>; +using UnsignedIntegerPairs = + testing::Types, std::pair, + std::pair, std::pair>; // TODO(kszucs): add half-float using FloatingPairs = @@ -117,7 +115,7 @@ using FloatingPairs = TYPED_TEST_SUITE(TestBinaryArithmeticsIntegral, IntegralPairs); TYPED_TEST_SUITE(TestBinaryArithmeticsSigned, SignedIntegerPairs); -// TYPED_TEST_SUITE(TestBinaryArithmeticsSigned, UnsignedIntegerPairs); +TYPED_TEST_SUITE(TestBinaryArithmeticsUnsigned, UnsignedIntegerPairs); TYPED_TEST_SUITE(TestBinaryArithmeticsFloating, FloatingPairs); TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { @@ -164,17 +162,60 @@ TYPED_TEST(TestBinaryArithmeticsSigned, Add) { "[-6, 5, -4, 3, -2, 1, 0]", "[-13, 11, 1, 7, 1, 3, 1]"); } -TYPED_TEST(TestBinaryArithmeticsSigned, AddOverflow) { +TYPED_TEST(TestBinaryArithmeticsUnsigned, OverflowWraps) { using InputCType = typename TestFixture::InputCType; auto min = std::numeric_limits::min(); auto max = std::numeric_limits::max(); + { + // Addition + auto left = this->MakeInputArray({max, min, max}); + auto right = this->MakeInputArray({1, 1, max}); + auto expected = this->MakeOutputArray({0, 1, static_cast(max - 1)}); + this->AssertBinop(arrow::compute::Add, left, right, expected); + } + { + // Subtraction + auto left = this->MakeInputArray({min, max}); + auto right = this->MakeInputArray({1, max}); + auto expected = this->MakeOutputArray({max, min}); + this->AssertBinop(arrow::compute::Sub, left, right, expected); + } + { + // Multiplication + auto left = this->MakeInputArray({min, max, max}); + auto right = this->MakeInputArray({max, 2, max}); + auto expected = this->MakeOutputArray({min, static_cast(max - 1), 1}); + this->AssertBinop(arrow::compute::Mul, left, right, expected); + } +} - auto left = this->MakeInputArray({max, min}); - auto right = this->MakeInputArray({1, -1}); - auto expected = this->MakeOutputArray({min, max}); +TYPED_TEST(TestBinaryArithmeticsSigned, OverflowWraps) { + using InputCType = typename TestFixture::InputCType; - this->AssertBinop(arrow::compute::Add, left, right, expected); + auto min = std::numeric_limits::min(); + auto max = std::numeric_limits::max(); + { + // Addition + auto left = this->MakeInputArray({max, min}); + auto right = this->MakeInputArray({1, -1}); + auto expected = this->MakeOutputArray({min, max}); + this->AssertBinop(arrow::compute::Add, left, right, expected); + } + { + // Subtraction + auto left = this->MakeInputArray({min, max}); + auto right = this->MakeInputArray({1, -1}); + auto expected = this->MakeOutputArray({max, min}); + this->AssertBinop(arrow::compute::Sub, left, right, expected); + } + { + // Multiplication + auto left = this->MakeInputArray({min, max}); + auto right = this->MakeInputArray({-1, 2}); + auto expected = this->MakeOutputArray({min, -2}); + this->AssertBinop(arrow::compute::Mul, left, right, expected); + } } TYPED_TEST(TestBinaryArithmeticsSigned, Sub) { From 72c33e6493e4db4da17c5af52cd0ce7253ef3cdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Thu, 4 Jun 2020 22:03:05 +0200 Subject: [PATCH 15/23] Apply suggestions from Ben Co-authored-by: Benjamin Kietzman --- cpp/src/arrow/compute/api_scalar.h | 20 +++++++++---------- .../compute/kernels/scalar_arithmetic.cc | 1 - 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/cpp/src/arrow/compute/api_scalar.h b/cpp/src/arrow/compute/api_scalar.h index 33a35e3b17e..ae4e65d4f06 100644 --- a/cpp/src/arrow/compute/api_scalar.h +++ b/cpp/src/arrow/compute/api_scalar.h @@ -45,23 +45,23 @@ namespace compute { ARROW_EXPORT Result Add(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); -/// \brief Subtract two values. Array values must be the same length. If a -/// value is null in either addend, the result is null +/// \brief Subtract two values. Array values must be the same length. If the +/// minuend or minuend is null the result will be null. /// -/// \param[in] left the first value -/// \param[in] right the second value +/// \param[in] minuend the value subtracted from +/// \param[in] subtrahend the value by which the minuend is reduced /// \param[in] ctx the function execution context, optional -/// \return the elementwise addition of the values +/// \return the elementwise difference of the values ARROW_EXPORT Result Sub(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); -/// \brief Multiply two values. Array values must be the same length. If a -/// value is null in either addend, the result is null +/// \brief Multiply two values. Array values must be the same length. If either +/// factor is null, the result is null /// -/// \param[in] left the first value -/// \param[in] right the second value +/// \param[in] left the first factor +/// \param[in] right the second factor /// \param[in] ctx the function execution context, optional -/// \return the elementwise addition of the values +/// \return the elementwise product of the factors ARROW_EXPORT Result Mul(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index acb3b6ce4b5..fea0e735854 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -17,7 +17,6 @@ #include "arrow/compute/kernels/common.h" #include "arrow/util/int_util.h" -#include "iostream" namespace arrow { namespace compute { From 9f34b32c2957a9121ce105e070373ebfb44061b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Thu, 4 Jun 2020 22:24:04 +0200 Subject: [PATCH 16/23] renames --- cpp/src/arrow/compute/api_scalar.cc | 4 +- cpp/src/arrow/compute/api_scalar.h | 8 +-- .../compute/kernels/scalar_arithmetic.cc | 8 +-- .../compute/kernels/scalar_arithmetic_test.cc | 53 ++++++++++--------- 4 files changed, 37 insertions(+), 36 deletions(-) diff --git a/cpp/src/arrow/compute/api_scalar.cc b/cpp/src/arrow/compute/api_scalar.cc index 842aed2bf76..9cdce7c1f16 100644 --- a/cpp/src/arrow/compute/api_scalar.cc +++ b/cpp/src/arrow/compute/api_scalar.cc @@ -42,8 +42,8 @@ namespace compute { // Arithmetic SCALAR_EAGER_BINARY(Add, "add") -SCALAR_EAGER_BINARY(Sub, "sub") -SCALAR_EAGER_BINARY(Mul, "mul") +SCALAR_EAGER_BINARY(Subtract, "subtract") +SCALAR_EAGER_BINARY(Multiply, "multiply") // ---------------------------------------------------------------------- // Set-related operations diff --git a/cpp/src/arrow/compute/api_scalar.h b/cpp/src/arrow/compute/api_scalar.h index ae4e65d4f06..d109ecead54 100644 --- a/cpp/src/arrow/compute/api_scalar.h +++ b/cpp/src/arrow/compute/api_scalar.h @@ -48,12 +48,12 @@ Result Add(const Datum& left, const Datum& right, ExecContext* ctx = NULL /// \brief Subtract two values. Array values must be the same length. If the /// minuend or minuend is null the result will be null. /// -/// \param[in] minuend the value subtracted from -/// \param[in] subtrahend the value by which the minuend is reduced +/// \param[in] left minuend, the value subtracted from +/// \param[in] right subtrahend, the value by which the minuend is reduced /// \param[in] ctx the function execution context, optional /// \return the elementwise difference of the values ARROW_EXPORT -Result Sub(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); +Result Subtract(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 is null @@ -63,7 +63,7 @@ Result Sub(const Datum& left, const Datum& right, ExecContext* ctx = NULL /// \param[in] ctx the function execution context, optional /// \return the elementwise product of the factors ARROW_EXPORT -Result Mul(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); +Result Multiply(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); enum CompareOperator { EQUAL, diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index fea0e735854..ad27a183233 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -33,14 +33,14 @@ struct Add { } }; -struct Sub { +struct Subtract { template > static constexpr T Call(KernelContext*, T left, T right) { return left - right; } }; -struct Mul { +struct Multiply { template > static constexpr T Call(KernelContext*, T left, T right) { return left * right; @@ -99,8 +99,8 @@ namespace internal { void RegisterScalarArithmetic(FunctionRegistry* registry) { codegen::AddBinaryFunction("add", registry); - codegen::AddBinaryFunction("sub", registry); - codegen::AddBinaryFunction("mul", registry); + codegen::AddBinaryFunction("subtract", registry); + codegen::AddBinaryFunction("multiply", 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 34dd2b2addb..ced12fb5426 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -64,6 +64,7 @@ class TestBinaryArithmetics> : public TestBase { return out; } + // (Array, Array) virtual void AssertBinop(BinaryFunction func, const std::shared_ptr& lhs, const std::shared_ptr& rhs, const std::shared_ptr& expected) { @@ -134,27 +135,27 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { } TYPED_TEST(TestBinaryArithmeticsIntegral, Sub) { - this->AssertBinop(arrow::compute::Sub, "[]", "[]", "[]"); - this->AssertBinop(arrow::compute::Sub, "[null]", "[null]", "[null]"); - this->AssertBinop(arrow::compute::Sub, "[3, 2, 6]", "[1, 0, 2]", "[2, 2, 4]"); + 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::Sub, "[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, "[1, 2, 3, 4, 5, 6, 7]", + "[0, 1, 2, 3, 4, 5, 6]", "[1, 1, 1, 1, 1, 1, 1]"); } TYPED_TEST(TestBinaryArithmeticsIntegral, Mul) { - this->AssertBinop(arrow::compute::Mul, "[]", "[]", "[]"); - this->AssertBinop(arrow::compute::Mul, "[null]", "[null]", "[null]"); - this->AssertBinop(arrow::compute::Mul, "[3, 2, 6]", "[1, 0, 2]", "[3, 0, 12]"); + 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::Mul, "[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, "[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::Mul, "[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, "[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::Mul, "[null, 1, 3, null, 2, 5]", "[1, 4, 2, 5, 0, 3]", - "[null, 4, 6, null, 0, 15]"); + this->AssertBinop(arrow::compute::Multiply, "[null, 1, 3, null, 2, 5]", + "[1, 4, 2, 5, 0, 3]", "[null, 4, 6, null, 0, 15]"); } TYPED_TEST(TestBinaryArithmeticsSigned, Add) { @@ -179,14 +180,14 @@ TYPED_TEST(TestBinaryArithmeticsUnsigned, OverflowWraps) { auto left = this->MakeInputArray({min, max}); auto right = this->MakeInputArray({1, max}); auto expected = this->MakeOutputArray({max, min}); - this->AssertBinop(arrow::compute::Sub, left, right, expected); + this->AssertBinop(arrow::compute::Subtract, left, right, expected); } { // Multiplication auto left = this->MakeInputArray({min, max, max}); auto right = this->MakeInputArray({max, 2, max}); auto expected = this->MakeOutputArray({min, static_cast(max - 1), 1}); - this->AssertBinop(arrow::compute::Mul, left, right, expected); + this->AssertBinop(arrow::compute::Multiply, left, right, expected); } } @@ -207,33 +208,33 @@ TYPED_TEST(TestBinaryArithmeticsSigned, OverflowWraps) { auto left = this->MakeInputArray({min, max}); auto right = this->MakeInputArray({1, -1}); auto expected = this->MakeOutputArray({max, min}); - this->AssertBinop(arrow::compute::Sub, left, right, expected); + this->AssertBinop(arrow::compute::Subtract, left, right, expected); } { // Multiplication auto left = this->MakeInputArray({min, max}); auto right = this->MakeInputArray({-1, 2}); auto expected = this->MakeOutputArray({min, -2}); - this->AssertBinop(arrow::compute::Mul, left, right, expected); + this->AssertBinop(arrow::compute::Multiply, left, right, expected); } } TYPED_TEST(TestBinaryArithmeticsSigned, Sub) { - this->AssertBinop(arrow::compute::Sub, "[0, 1, 2, 3, 4, 5, 6]", "[1, 2, 3, 4, 5, 6, 7]", - "[-1, -1, -1, -1, -1, -1, -1]"); + this->AssertBinop(arrow::compute::Subtract, "[0, 1, 2, 3, 4, 5, 6]", + "[1, 2, 3, 4, 5, 6, 7]", "[-1, -1, -1, -1, -1, -1, -1]"); - this->AssertBinop(arrow::compute::Sub, "[0, 0, 0, 0, 0, 0, 0]", "[6, 5, 4, 3, 2, 1, 0]", - "[-6, -5, -4, -3, -2, -1, 0]"); + this->AssertBinop(arrow::compute::Subtract, "[0, 0, 0, 0, 0, 0, 0]", + "[6, 5, 4, 3, 2, 1, 0]", "[-6, -5, -4, -3, -2, -1, 0]"); - this->AssertBinop(arrow::compute::Sub, "[10, 12, 4, 50, 50, 32, 11]", + this->AssertBinop(arrow::compute::Subtract, "[10, 12, 4, 50, 50, 32, 11]", "[2, 0, 6, 1, 5, 3, 4]", "[8, 12, -2, 49, 45, 29, 7]"); - this->AssertBinop(arrow::compute::Sub, "[null, 1, 3, null, 2, 5]", "[1, 4, 2, 5, 0, 3]", - "[null, -3, 1, null, 2, 2]"); + this->AssertBinop(arrow::compute::Subtract, "[null, 1, 3, null, 2, 5]", + "[1, 4, 2, 5, 0, 3]", "[null, -3, 1, null, 2, 2]"); } TYPED_TEST(TestBinaryArithmeticsSigned, Mul) { - this->AssertBinop(arrow::compute::Mul, "[-10, 12, 4, 50, -5, 32, 11]", + this->AssertBinop(arrow::compute::Multiply, "[-10, 12, 4, 50, -5, 32, 11]", "[-2, 0, -6, 1, 5, 3, 4]", "[20, 0, -24, 50, -25, 96, 44]"); } From 85e98f9358860cb7de23d6b3ff9a83c141eec688 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Fri, 5 Jun 2020 00:07:03 +0200 Subject: [PATCH 17/23] make uint16 an exceptional case for multiplication --- .../compute/kernels/scalar_arithmetic.cc | 18 +++++++++++++--- .../compute/kernels/scalar_arithmetic_test.cc | 21 ++++++++++++++++++- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index ad27a183233..ec156604842 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -23,8 +23,12 @@ namespace compute { // explicitly disallow signed integers as input arguments template -using enable_if_unsigned_or_floating = - enable_if_t::value || std::is_floating_point::value>; +using is_unsigned_or_floating = + std::integral_constant::value || + std::is_floating_point::value>; + +template +using enable_if_unsigned_or_floating = enable_if_t::value>; struct Add { template > @@ -41,10 +45,18 @@ struct Subtract { }; struct Multiply { - template > + template ::value && + is_unsigned_or_floating::value>> static constexpr T Call(KernelContext*, T left, T right) { return left * right; } + + template ::value>> + static constexpr uint16_t Call(KernelContext*, T left, T right) { + // exception because multiplying to uint16 values involves implicit promotion + // to signed int32 type which can trigger undefined behaviour by signed overflow + return static_cast(left) * static_cast(right); + } }; namespace codegen { diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index ced12fb5426..8a6193370a3 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -16,7 +16,6 @@ // under the License. #include -#include #include #include #include @@ -64,6 +63,23 @@ class TestBinaryArithmetics> : public TestBase { return out; } + // (Scalar, Array) + // virtual void AssertBinop(BinaryFunction func, InputCType lhs, + // const std::string& rhs, + // const std::string& expected) { + // auto input_type = TypeTraits::type_singleton(); + // auto output_type = TypeTraits::type_singleton(); + + // auto left = Datum(MakeScalar(input_type, lhs).ValueOrDie()); + // auto right = Datum(ArrayFromJSON(input_type, rhs)); + // auto exp = ArrayFromJSON(output_type, expected); + + // ASSERT_OK_AND_ASSIGN(Datum result, func(left, right, nullptr)); + // std::shared_ptr out = result.make_array(); + // ASSERT_OK(out->ValidateFull()); + // AssertArraysEqual(*exp, *out); + // } + // (Array, Array) virtual void AssertBinop(BinaryFunction func, const std::shared_ptr& lhs, const std::shared_ptr& rhs, @@ -132,6 +148,9 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { 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]"); } TYPED_TEST(TestBinaryArithmeticsIntegral, Sub) { From 83c442b9c9f76d9de3de8beeae3e61610c2100bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Fri, 5 Jun 2020 02:26:44 +0200 Subject: [PATCH 18/23] fix arithmetics with scalar operands --- .../compute/kernels/scalar_arithmetic.cc | 80 ++++++++++++++----- .../compute/kernels/scalar_arithmetic_test.cc | 76 +++++++++++++----- 2 files changed, 116 insertions(+), 40 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index ec156604842..164b75546f6 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -21,42 +21,86 @@ namespace arrow { namespace compute { -// explicitly disallow signed integers as input arguments template -using is_unsigned_or_floating = - std::integral_constant::value || - std::is_floating_point::value>; +using is_unsigned_integer = std::integral_constant::value && + std::is_unsigned::value>; template -using enable_if_unsigned_or_floating = enable_if_t::value>; +using is_signed_integer = + std::integral_constant::value && std::is_signed::value>; + +template +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_floating_point = enable_if_t::value, T>; struct Add { - template > - static constexpr T Call(KernelContext*, T left, T right) { + template + static constexpr enable_if_floating_point Call(KernelContext*, T left, T right) { return left + right; } + + template + static constexpr enable_if_unsigned_integer Call(KernelContext*, T left, T right) { + return left + right; + } + + template + static constexpr enable_if_signed_integer Call(KernelContext*, T left, T right) { + using Unsigned = typename std::make_unsigned::type; + return static_cast(left) + static_cast(right); + } }; struct Subtract { - template > - static constexpr T Call(KernelContext*, T left, T right) { + template + static constexpr enable_if_floating_point Call(KernelContext*, T left, T right) { return left - right; } + + template + static constexpr enable_if_unsigned_integer Call(KernelContext*, T left, T right) { + return left - right; + } + + template + static constexpr enable_if_signed_integer Call(KernelContext*, T left, T right) { + using Unsigned = typename std::make_unsigned::type; + return static_cast(left) - static_cast(right); + } }; struct Multiply { - template ::value && - is_unsigned_or_floating::value>> - static constexpr T Call(KernelContext*, T left, T right) { + template + static constexpr enable_if_floating_point Call(KernelContext*, T left, T right) { return left * right; } - template ::value>> - static constexpr uint16_t Call(KernelContext*, T left, T right) { + template + static constexpr enable_if_t< + is_unsigned_integer::value && !std::is_same::value, T> + Call(KernelContext*, T left, T right) { + return left * right; + } + + template + static constexpr enable_if_t::value, T> Call(KernelContext*, + T left, + T right) { // exception because multiplying to uint16 values involves implicit promotion // to signed int32 type which can trigger undefined behaviour by signed overflow return static_cast(left) * static_cast(right); } + + template + static constexpr enable_if_signed_integer Call(KernelContext*, T left, T right) { + using Unsigned = typename std::make_unsigned::type; + return static_cast(left) * static_cast(right); + } }; namespace codegen { @@ -70,19 +114,19 @@ template ArrayKernelExec NumericEqualTypesBinary(detail::GetTypeId get_id) { switch (get_id.id) { case Type::INT8: - return ScalarBinaryEqualTypes::Exec; + return ScalarBinaryEqualTypes::Exec; case Type::UINT8: return ScalarBinaryEqualTypes::Exec; case Type::INT16: - return ScalarBinaryEqualTypes::Exec; + return ScalarBinaryEqualTypes::Exec; case Type::UINT16: return ScalarBinaryEqualTypes::Exec; case Type::INT32: - return ScalarBinaryEqualTypes::Exec; + return ScalarBinaryEqualTypes::Exec; case Type::UINT32: return ScalarBinaryEqualTypes::Exec; case Type::INT64: - return ScalarBinaryEqualTypes::Exec; + return ScalarBinaryEqualTypes::Exec; case Type::UINT64: return ScalarBinaryEqualTypes::Exec; case Type::FLOAT: diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 8a6193370a3..f7e94971144 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -64,34 +64,33 @@ class TestBinaryArithmetics> : public TestBase { } // (Scalar, Array) - // virtual void AssertBinop(BinaryFunction func, InputCType lhs, - // const std::string& rhs, - // const std::string& expected) { - // auto input_type = TypeTraits::type_singleton(); - // auto output_type = TypeTraits::type_singleton(); - - // auto left = Datum(MakeScalar(input_type, lhs).ValueOrDie()); - // auto right = Datum(ArrayFromJSON(input_type, rhs)); - // auto exp = ArrayFromJSON(output_type, expected); - - // ASSERT_OK_AND_ASSIGN(Datum result, func(left, right, nullptr)); - // std::shared_ptr out = result.make_array(); - // ASSERT_OK(out->ValidateFull()); - // AssertArraysEqual(*exp, *out); - // } + void AssertBinop(BinaryFunction func, InputCType lhs, const std::string& rhs, + const std::string& expected) { + auto input_type = TypeTraits::type_singleton(); + auto output_type = TypeTraits::type_singleton(); + + ASSERT_OK_AND_ASSIGN(auto left, MakeScalar(input_type, lhs)); + auto right = ArrayFromJSON(input_type, rhs); + auto exp = ArrayFromJSON(output_type, expected); + + ASSERT_OK_AND_ASSIGN(auto result, func(left, right, nullptr)); + std::shared_ptr out = result.make_array(); + ASSERT_OK(out->ValidateFull()); + AssertArraysEqual(*exp, *out); + } // (Array, Array) - virtual void AssertBinop(BinaryFunction func, const std::shared_ptr& lhs, - const std::shared_ptr& rhs, - const std::shared_ptr& expected) { + void AssertBinop(BinaryFunction func, const std::shared_ptr& lhs, + const std::shared_ptr& rhs, + const std::shared_ptr& expected) { ASSERT_OK_AND_ASSIGN(Datum result, func(lhs, rhs, nullptr)); std::shared_ptr out = result.make_array(); ASSERT_OK(out->ValidateFull()); AssertArraysEqual(*expected, *out); } - virtual void AssertBinop(BinaryFunction func, const std::string& lhs, - const std::string& rhs, const std::string& expected) { + void AssertBinop(BinaryFunction func, const std::string& lhs, const std::string& rhs, + const std::string& expected) { auto input_type = TypeTraits::type_singleton(); auto output_type = TypeTraits::type_singleton(); AssertBinop(func, ArrayFromJSON(input_type, lhs), ArrayFromJSON(input_type, rhs), @@ -149,8 +148,8 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { 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, 10, "[null, 1, 3, null, 2, 5]", + "[null, 11, 13, null, 12, 15]"); } TYPED_TEST(TestBinaryArithmeticsIntegral, Sub) { @@ -160,6 +159,9 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Sub) { 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]"); } TYPED_TEST(TestBinaryArithmeticsIntegral, Mul) { @@ -175,6 +177,9 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Mul) { 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]"); } TYPED_TEST(TestBinaryArithmeticsSigned, Add) { @@ -274,6 +279,33 @@ TYPED_TEST(TestBinaryArithmeticsFloating, Add) { this->AssertBinop(arrow::compute::Add, "[null, 1, 3.3, null, 2, 5.3]", "[1, 4, 2, 5, 0, 3]", "[null, 5, 5.3, null, 2, 8.3]"); + + this->AssertBinop(arrow::compute::Add, 1.1, "[null, 1, 3.3, null, 2, 5.3]", + "[null, 2.1, 4.4, null, 3.1, 6.4]"); +} + +TYPED_TEST(TestBinaryArithmeticsFloating, Sub) { + this->AssertBinop(arrow::compute::Subtract, "[]", "[]", "[]"); + + this->AssertBinop(arrow::compute::Subtract, "[3.4, 2.6, 6.3]", "[1, 0, 2]", + "[2.4, 2.6, 4.3]"); + + // this->AssertBinop(arrow::compute::Subtract, + // "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]", + // "[0.1, 1.2, 2.3, 3.4, 4.5, 5.6, 6.7]", + // "[1.0, 1.2, 1.2, 0.9, 0.6, 1.2, 0.6]"); + + this->AssertBinop(arrow::compute::Subtract, "[7, 6, 5, 4, 3, 2, 1]", + "[6, 5, 4, 3, 2, 1, 0]", "[1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0]"); + + // this->AssertBinop(arrow::compute::Subtract, "[10.4, 12, 4.2, 50, 50.3, 32, 11]", + // "[2, 0, 6, 1, 5, 3, 4]", "[8.4, 12, -1.8, 49, 45.3, 29, 7]"); + + // this->AssertBinop(arrow::compute::Subtract, "[null, 1, 3.3, null, 2, 5.3]", + // "[1, 4, 2, 5, 0, 3]", "[null, -3, 1.3, null, 2, 2.3]"); + + // this->AssertBinop(arrow::compute::Subtract, 0.1, "[null, 1, 3.3, null, 2, 5.3]", + // "[null, -0.9, -3.2, null, -1.9, -5.2]"); } } // namespace compute From 3d7e5eb093cbaaed926fdf7b92baae12aba29bf4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Fri, 5 Jun 2020 02:33:51 +0200 Subject: [PATCH 19/23] ubsan --- cpp/src/arrow/compute/kernels/scalar_arithmetic.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 164b75546f6..98e616be8d3 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -88,16 +88,18 @@ struct Multiply { } template - static constexpr enable_if_t::value, T> Call(KernelContext*, - T left, - T right) { + static constexpr enable_if_t< + std::is_same::value || std::is_same::value, T> + Call(KernelContext*, T left, T right) { // exception because multiplying to uint16 values involves implicit promotion // to signed int32 type which can trigger undefined behaviour by signed overflow return static_cast(left) * static_cast(right); } template - static constexpr enable_if_signed_integer Call(KernelContext*, T left, T right) { + static constexpr enable_if_t< + is_signed_integer::value && !std::is_same::value, T> + Call(KernelContext*, T left, T right) { using Unsigned = typename std::make_unsigned::type; return static_cast(left) * static_cast(right); } From 19ad8caf44cf76ca4212c2931050f5ac9de9201a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Fri, 5 Jun 2020 15:55:08 +0200 Subject: [PATCH 20/23] compare as string workaround until we have AssertArraysAlmostEqual --- .../compute/kernels/scalar_arithmetic_test.cc | 42 ++++++++++++------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index f7e94971144..a57fd5a764d 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -65,7 +65,7 @@ class TestBinaryArithmetics> : public TestBase { // (Scalar, Array) void AssertBinop(BinaryFunction func, InputCType lhs, const std::string& rhs, - const std::string& expected) { + const std::string& expected, bool compare_as_string = false) { auto input_type = TypeTraits::type_singleton(); auto output_type = TypeTraits::type_singleton(); @@ -76,25 +76,34 @@ class TestBinaryArithmetics> : public TestBase { ASSERT_OK_AND_ASSIGN(auto result, func(left, right, nullptr)); std::shared_ptr out = result.make_array(); ASSERT_OK(out->ValidateFull()); - AssertArraysEqual(*exp, *out); + if (compare_as_string) { + ASSERT_EQ(exp->ToString(), out->ToString()); + } else { + AssertArraysEqual(*exp, *out); + } } // (Array, Array) void AssertBinop(BinaryFunction func, const std::shared_ptr& lhs, const std::shared_ptr& rhs, - const std::shared_ptr& expected) { + const std::shared_ptr& expected, + bool compare_as_string = false) { ASSERT_OK_AND_ASSIGN(Datum result, func(lhs, rhs, nullptr)); std::shared_ptr out = result.make_array(); ASSERT_OK(out->ValidateFull()); - AssertArraysEqual(*expected, *out); + if (compare_as_string) { + ASSERT_EQ(expected->ToString(), out->ToString()); + } else { + AssertArraysEqual(*expected, *out); + } } void AssertBinop(BinaryFunction func, const std::string& lhs, const std::string& rhs, - const std::string& expected) { + const std::string& expected, bool compare_as_string = false) { auto input_type = TypeTraits::type_singleton(); auto output_type = TypeTraits::type_singleton(); AssertBinop(func, ArrayFromJSON(input_type, lhs), ArrayFromJSON(input_type, rhs), - ArrayFromJSON(output_type, expected)); + ArrayFromJSON(output_type, expected), compare_as_string); } }; @@ -290,22 +299,23 @@ TYPED_TEST(TestBinaryArithmeticsFloating, Sub) { this->AssertBinop(arrow::compute::Subtract, "[3.4, 2.6, 6.3]", "[1, 0, 2]", "[2.4, 2.6, 4.3]"); - // this->AssertBinop(arrow::compute::Subtract, - // "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]", - // "[0.1, 1.2, 2.3, 3.4, 4.5, 5.6, 6.7]", - // "[1.0, 1.2, 1.2, 0.9, 0.6, 1.2, 0.6]"); + this->AssertBinop(arrow::compute::Subtract, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]", + "[0.1, 1.2, 2.3, 3.4, 4.5, 5.6, 6.7]", + "[1.0, 1.2, 1.2, 0.9, 0.6, 1.2, 0.6]", /*compare_as_string=*/true); this->AssertBinop(arrow::compute::Subtract, "[7, 6, 5, 4, 3, 2, 1]", "[6, 5, 4, 3, 2, 1, 0]", "[1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0]"); - // this->AssertBinop(arrow::compute::Subtract, "[10.4, 12, 4.2, 50, 50.3, 32, 11]", - // "[2, 0, 6, 1, 5, 3, 4]", "[8.4, 12, -1.8, 49, 45.3, 29, 7]"); + this->AssertBinop(arrow::compute::Subtract, "[10.4, 12, 4.2, 50, 50.3, 32, 11]", + "[2, 0, 6, 1, 5, 3, 4]", "[8.4, 12, -1.8, 49, 45.3, 29, 7]", + /*compare_as_string=*/true); - // this->AssertBinop(arrow::compute::Subtract, "[null, 1, 3.3, null, 2, 5.3]", - // "[1, 4, 2, 5, 0, 3]", "[null, -3, 1.3, null, 2, 2.3]"); + this->AssertBinop(arrow::compute::Subtract, "[null, 1, 3.3, null, 2, 5.3]", + "[1, 4, 2, 5, 0, 3]", "[null, -3, 1.3, null, 2, 2.3]", + /*compare_as_string=*/true); - // this->AssertBinop(arrow::compute::Subtract, 0.1, "[null, 1, 3.3, null, 2, 5.3]", - // "[null, -0.9, -3.2, null, -1.9, -5.2]"); + this->AssertBinop(arrow::compute::Subtract, 0.1, "[null, 1, 3.3, null, 2, 5.3]", + "[null, -0.9, -3.2, null, -1.9, -5.2]", /*compare_as_string=*/true); } } // namespace compute From d81d00f7f5f885963bde629d8fa1a37ea05e2c1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kriszti=C3=A1n=20Sz=C5=B1cs?= Date: Fri, 5 Jun 2020 16:05:01 +0200 Subject: [PATCH 21/23] scalar - scalar tests --- .../compute/kernels/scalar_arithmetic_test.cc | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index a57fd5a764d..5049c65727c 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -63,6 +63,22 @@ class TestBinaryArithmetics> : public TestBase { return out; } + // (Scalar, Scalar) + void AssertBinop(BinaryFunction func, InputCType lhs, InputCType rhs, + OutputCType expected) { + auto input_type = TypeTraits::type_singleton(); + auto output_type = TypeTraits::type_singleton(); + + ASSERT_OK_AND_ASSIGN(auto left, MakeScalar(input_type, lhs)); + ASSERT_OK_AND_ASSIGN(auto right, MakeScalar(input_type, rhs)); + ASSERT_OK_AND_ASSIGN(auto exp, MakeScalar(output_type, expected)); + + ASSERT_OK_AND_ASSIGN(auto result, func(left, right, nullptr)); + std::shared_ptr out = result.scalar(); + + AssertScalarsEqual(*exp, *out, true); + } + // (Scalar, Array) void AssertBinop(BinaryFunction func, InputCType lhs, const std::string& rhs, const std::string& expected, bool compare_as_string = false) { @@ -159,6 +175,8 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { 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) { @@ -171,6 +189,8 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Sub) { 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) { @@ -189,11 +209,16 @@ TYPED_TEST(TestBinaryArithmeticsIntegral, Mul) { 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) { this->AssertBinop(arrow::compute::Add, "[-7, 6, 5, 4, 3, 2, 1]", "[-6, 5, -4, 3, -2, 1, 0]", "[-13, 11, 1, 7, 1, 3, 1]"); + this->AssertBinop(arrow::compute::Add, -1, "[-6, 5, -4, 3, -2, 1, 0]", + "[-7, 4, -5, 2, -3, 0, -1]"); + this->AssertBinop(arrow::compute::Add, -10, 5, -5); } TYPED_TEST(TestBinaryArithmeticsUnsigned, OverflowWraps) { @@ -269,6 +294,9 @@ TYPED_TEST(TestBinaryArithmeticsSigned, Sub) { TYPED_TEST(TestBinaryArithmeticsSigned, Mul) { this->AssertBinop(arrow::compute::Multiply, "[-10, 12, 4, 50, -5, 32, 11]", "[-2, 0, -6, 1, 5, 3, 4]", "[20, 0, -24, 50, -25, 96, 44]"); + this->AssertBinop(arrow::compute::Multiply, -2, "[-10, 12, 4, 50, -5, 32, 11]", + "[20, -24, -8, -100, 10, -64, -22]"); + this->AssertBinop(arrow::compute::Multiply, -5, -5, 25); } TYPED_TEST(TestBinaryArithmeticsFloating, Add) { From 9c2921a3867effb4473cb05fa6cc349e3fa14e2c Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Mon, 8 Jun 2020 16:42:08 -0400 Subject: [PATCH 22/23] cleanup tests --- cpp/src/arrow/compute/api_scalar.h | 22 +- .../compute/kernels/scalar_arithmetic.cc | 51 ++-- .../compute/kernels/scalar_arithmetic_test.cc | 246 +++++++----------- cpp/src/arrow/testing/gtest_util.cc | 15 ++ cpp/src/arrow/testing/gtest_util.h | 9 +- 5 files changed, 155 insertions(+), 188 deletions(-) diff --git a/cpp/src/arrow/compute/api_scalar.h b/cpp/src/arrow/compute/api_scalar.h index d109ecead54..bc502f7bcb9 100644 --- a/cpp/src/arrow/compute/api_scalar.h +++ b/cpp/src/arrow/compute/api_scalar.h @@ -35,33 +35,33 @@ namespace compute { // ---------------------------------------------------------------------- -/// \brief Add two values together. Array values must be the same length. If a -/// value is null in either addend, the result is null +/// \brief Add two values together. Array values must be the same length. If +/// either addend is null the result will be null. /// -/// \param[in] left the first value -/// \param[in] right the second value +/// \param[in] left the first addend +/// \param[in] right the second addend /// \param[in] ctx the function execution context, optional -/// \return the elementwise addition of the values +/// \return the elementwise sum ARROW_EXPORT Result Add(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); /// \brief Subtract two values. Array values must be the same length. If the -/// minuend or minuend is null the result will be null. +/// minuend or subtrahend is null the result will be null. /// -/// \param[in] left minuend, the value subtracted from -/// \param[in] right subtrahend, the value by which the minuend is reduced +/// \param[in] left the value subtracted from (minuend) +/// \param[in] right the value by which the minuend is reduced (subtrahend) /// \param[in] ctx the function execution context, optional -/// \return the elementwise difference of the values +/// \return the elementwise difference ARROW_EXPORT Result Subtract(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 is null +/// factor is null the result will be null. /// /// \param[in] left the first factor /// \param[in] right the second factor /// \param[in] ctx the function execution context, optional -/// \return the elementwise product of the factors +/// \return the elementwise product ARROW_EXPORT Result Multiply(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 98e616be8d3..de05900f7c1 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -38,6 +38,11 @@ using enable_if_unsigned_integer = enable_if_t::value, T> template using enable_if_floating_point = enable_if_t::value, T>; +template ::type> +constexpr Unsigned to_unsigned(T signed_) { + return static_cast(signed_); +} + struct Add { template static constexpr enable_if_floating_point Call(KernelContext*, T left, T right) { @@ -51,8 +56,7 @@ struct Add { template static constexpr enable_if_signed_integer Call(KernelContext*, T left, T right) { - using Unsigned = typename std::make_unsigned::type; - return static_cast(left) + static_cast(right); + return to_unsigned(left) + to_unsigned(right); } }; @@ -69,39 +73,48 @@ struct Subtract { template static constexpr enable_if_signed_integer Call(KernelContext*, T left, T right) { - using Unsigned = typename std::make_unsigned::type; - return static_cast(left) - static_cast(right); + return to_unsigned(left) - to_unsigned(right); } }; struct Multiply { + static_assert(std::is_same::value, ""); + static_assert(std::is_same::value, ""); + static_assert(std::is_same::value, ""); + static_assert(std::is_same::value, ""); + static_assert(std::is_same::value, ""); + + static_assert(std::is_same::value, ""); + + static_assert(std::is_same::value, ""); + static_assert(std::is_same::value, ""); + template static constexpr enable_if_floating_point Call(KernelContext*, T left, T right) { return left * right; } template - static constexpr enable_if_t< - is_unsigned_integer::value && !std::is_same::value, T> - Call(KernelContext*, T left, T right) { + static constexpr enable_if_unsigned_integer Call(KernelContext*, T left, T right) { return left * right; } template - static constexpr enable_if_t< - std::is_same::value || std::is_same::value, T> - Call(KernelContext*, T left, T right) { - // exception because multiplying to uint16 values involves implicit promotion - // to signed int32 type which can trigger undefined behaviour by signed overflow - return static_cast(left) * static_cast(right); + static constexpr enable_if_signed_integer Call(KernelContext*, T left, T right) { + return to_unsigned(left) * to_unsigned(right); } - template - static constexpr enable_if_t< - is_signed_integer::value && !std::is_same::value, T> - Call(KernelContext*, T left, T right) { - using Unsigned = typename std::make_unsigned::type; - return static_cast(left) * static_cast(right); + // Multiplication of 16 bit integer types implicitly promotes to signed 32 bit + // integer. However, some inputs may nevertheless overflow (which triggers undefined + // behaviour). Therefore we first cast to 32 bit unsigned integers where overflow is + // well defined. + template + static constexpr int16_t Call(KernelContext*, int16_t left, int16_t right) { + return static_cast(left) * static_cast(right); + } + template + static constexpr uint16_t Call(KernelContext*, uint16_t left, uint16_t right) { + return static_cast(left) * static_cast(right); } }; diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 5049c65727c..ae52cff056c 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -29,6 +29,7 @@ #include "arrow/type.h" #include "arrow/type_traits.h" #include "arrow/util/checked_cast.h" +#include "arrow/util/string.h" #include "arrow/testing/gtest_common.h" #include "arrow/testing/gtest_util.h" @@ -37,127 +38,96 @@ namespace arrow { namespace compute { -template -class TestBinaryArithmetics; - -template -class TestBinaryArithmetics> : public TestBase { +template +class TestBinaryArithmetics : public TestBase { protected: - using InputType = I; - using OutputType = O; - using InputCType = typename I::c_type; - using OutputCType = typename O::c_type; + using CType = typename ArrowType::c_type; - using BinaryFunction = - std::function(const Datum&, const Datum&, ExecContext*)>; - - std::shared_ptr MakeInputArray(const std::vector& values) { - std::shared_ptr out; - ArrayFromVector(values, &out); - return out; + static std::shared_ptr type_singleton() { + return TypeTraits::type_singleton(); } - std::shared_ptr MakeOutputArray(const std::vector& values) { - std::shared_ptr out; - ArrayFromVector(values, &out); - return out; - } + using BinaryFunction = + std::function(const Datum&, const Datum&, ExecContext*)>; // (Scalar, Scalar) - void AssertBinop(BinaryFunction func, InputCType lhs, InputCType rhs, - OutputCType expected) { - auto input_type = TypeTraits::type_singleton(); - auto output_type = TypeTraits::type_singleton(); - - ASSERT_OK_AND_ASSIGN(auto left, MakeScalar(input_type, lhs)); - ASSERT_OK_AND_ASSIGN(auto right, MakeScalar(input_type, rhs)); - ASSERT_OK_AND_ASSIGN(auto exp, MakeScalar(output_type, expected)); - - ASSERT_OK_AND_ASSIGN(auto result, func(left, right, nullptr)); - std::shared_ptr out = result.scalar(); + void AssertBinop(BinaryFunction func, CType lhs, CType rhs, CType expected) { + ASSERT_OK_AND_ASSIGN(auto left, MakeScalar(type_singleton(), lhs)); + ASSERT_OK_AND_ASSIGN(auto right, MakeScalar(type_singleton(), rhs)); + ASSERT_OK_AND_ASSIGN(auto exp, MakeScalar(type_singleton(), expected)); - AssertScalarsEqual(*exp, *out, true); + ASSERT_OK_AND_ASSIGN(auto actual, func(left, right, nullptr)); + AssertScalarsEqual(*exp, *actual.scalar(), true); } // (Scalar, Array) - void AssertBinop(BinaryFunction func, InputCType lhs, const std::string& rhs, - const std::string& expected, bool compare_as_string = false) { - auto input_type = TypeTraits::type_singleton(); - auto output_type = TypeTraits::type_singleton(); - - ASSERT_OK_AND_ASSIGN(auto left, MakeScalar(input_type, lhs)); - auto right = ArrayFromJSON(input_type, rhs); - auto exp = ArrayFromJSON(output_type, expected); - - ASSERT_OK_AND_ASSIGN(auto result, func(left, right, nullptr)); - std::shared_ptr out = result.make_array(); - ASSERT_OK(out->ValidateFull()); - if (compare_as_string) { - ASSERT_EQ(exp->ToString(), out->ToString()); - } else { - AssertArraysEqual(*exp, *out); - } + void AssertBinop(BinaryFunction func, CType lhs, const std::string& rhs, + const std::string& expected) { + ASSERT_OK_AND_ASSIGN(auto left, MakeScalar(type_singleton(), lhs)); + auto right = ArrayFromJSON(type_singleton(), rhs); + auto exp = ArrayFromJSON(type_singleton(), expected); + + ASSERT_OK_AND_ASSIGN(auto actual, func(left, right, nullptr)); + ValidateAndAssertApproxEqual(actual.make_array(), expected); } // (Array, Array) - void AssertBinop(BinaryFunction func, const std::shared_ptr& lhs, - const std::shared_ptr& rhs, - const std::shared_ptr& expected, - bool compare_as_string = false) { - ASSERT_OK_AND_ASSIGN(Datum result, func(lhs, rhs, nullptr)); - std::shared_ptr out = result.make_array(); - ASSERT_OK(out->ValidateFull()); - if (compare_as_string) { - ASSERT_EQ(expected->ToString(), out->ToString()); - } else { - AssertArraysEqual(*expected, *out); - } + void AssertBinop(BinaryFunction func, const std::string& lhs, const std::string& rhs, + const std::string& expected) { + auto left = ArrayFromJSON(type_singleton(), lhs); + auto right = ArrayFromJSON(type_singleton(), rhs); + + ASSERT_OK_AND_ASSIGN(Datum actual, func(left, right, nullptr)); + ValidateAndAssertApproxEqual(actual.make_array(), expected); } - void AssertBinop(BinaryFunction func, const std::string& lhs, const std::string& rhs, - const std::string& expected, bool compare_as_string = false) { - auto input_type = TypeTraits::type_singleton(); - auto output_type = TypeTraits::type_singleton(); - AssertBinop(func, ArrayFromJSON(input_type, lhs), ArrayFromJSON(input_type, rhs), - ArrayFromJSON(output_type, expected), compare_as_string); + void ValidateAndAssertApproxEqual(std::shared_ptr actual, + const std::string& expected) { + auto exp = ArrayFromJSON(type_singleton(), expected); + ASSERT_OK(actual->ValidateFull()); + AssertArraysApproxEqual(*exp, *actual); } }; -template -class TestBinaryArithmeticsIntegral : public TestBinaryArithmetics {}; +template +std::string MakeArray(Elements... elements) { + std::vector elements_as_strings = {std::to_string(elements)...}; + + std::vector elements_as_views(sizeof...(Elements)); + std::copy(elements_as_strings.begin(), elements_as_strings.end(), + elements_as_views.begin()); -template -class TestBinaryArithmeticsSigned : public TestBinaryArithmeticsIntegral {}; + return "[" + internal::JoinStrings(elements_as_views, ",") + "]"; +} + +template +class TestBinaryArithmeticsIntegral : public TestBinaryArithmetics {}; -template -class TestBinaryArithmeticsUnsigned : public TestBinaryArithmeticsIntegral {}; +template +class TestBinaryArithmeticsSigned : public TestBinaryArithmeticsIntegral {}; -template -class TestBinaryArithmeticsFloating : public TestBinaryArithmetics {}; +template +class TestBinaryArithmeticsUnsigned : public TestBinaryArithmeticsIntegral {}; + +template +class TestBinaryArithmeticsFloating : public TestBinaryArithmetics {}; // InputType - OutputType pairs -using IntegralPairs = - testing::Types, std::pair, - std::pair, std::pair, - std::pair, std::pair, - std::pair, std::pair>; +using IntegralTypes = testing::Types; -using SignedIntegerPairs = - testing::Types, std::pair, - std::pair, std::pair>; +using SignedIntegerTypes = testing::Types; -using UnsignedIntegerPairs = - testing::Types, std::pair, - std::pair, std::pair>; +using UnsignedIntegerTypes = + testing::Types; // TODO(kszucs): add half-float -using FloatingPairs = - testing::Types, std::pair>; +using FloatingTypes = testing::Types; -TYPED_TEST_SUITE(TestBinaryArithmeticsIntegral, IntegralPairs); -TYPED_TEST_SUITE(TestBinaryArithmeticsSigned, SignedIntegerPairs); -TYPED_TEST_SUITE(TestBinaryArithmeticsUnsigned, UnsignedIntegerPairs); -TYPED_TEST_SUITE(TestBinaryArithmeticsFloating, FloatingPairs); +TYPED_TEST_SUITE(TestBinaryArithmeticsIntegral, IntegralTypes); +TYPED_TEST_SUITE(TestBinaryArithmeticsSigned, SignedIntegerTypes); +TYPED_TEST_SUITE(TestBinaryArithmeticsUnsigned, UnsignedIntegerTypes); +TYPED_TEST_SUITE(TestBinaryArithmeticsFloating, FloatingTypes); TYPED_TEST(TestBinaryArithmeticsIntegral, Add) { this->AssertBinop(arrow::compute::Add, "[]", "[]", "[]"); @@ -221,60 +191,36 @@ TYPED_TEST(TestBinaryArithmeticsSigned, Add) { this->AssertBinop(arrow::compute::Add, -10, 5, -5); } -TYPED_TEST(TestBinaryArithmeticsUnsigned, OverflowWraps) { - using InputCType = typename TestFixture::InputCType; - - auto min = std::numeric_limits::min(); - auto max = std::numeric_limits::max(); - { - // Addition - auto left = this->MakeInputArray({max, min, max}); - auto right = this->MakeInputArray({1, 1, max}); - auto expected = this->MakeOutputArray({0, 1, static_cast(max - 1)}); - this->AssertBinop(arrow::compute::Add, left, right, expected); - } - { - // Subtraction - auto left = this->MakeInputArray({min, max}); - auto right = this->MakeInputArray({1, max}); - auto expected = this->MakeOutputArray({max, min}); - this->AssertBinop(arrow::compute::Subtract, left, right, expected); - } - { - // Multiplication - auto left = this->MakeInputArray({min, max, max}); - auto right = this->MakeInputArray({max, 2, max}); - auto expected = this->MakeOutputArray({min, static_cast(max - 1), 1}); - this->AssertBinop(arrow::compute::Multiply, left, right, expected); - } +TYPED_TEST(TestBinaryArithmeticsSigned, OverflowWraps) { + using CType = typename TestFixture::CType; + + auto min = std::numeric_limits::lowest(); + auto max = std::numeric_limits::max(); + + this->AssertBinop(arrow::compute::Add, MakeArray(min, max, max), + MakeArray(CType(-1), 1, max), MakeArray(max, min, CType(-2))); + + 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, OverflowWraps) { - using InputCType = typename TestFixture::InputCType; - - auto min = std::numeric_limits::min(); - auto max = std::numeric_limits::max(); - { - // Addition - auto left = this->MakeInputArray({max, min}); - auto right = this->MakeInputArray({1, -1}); - auto expected = this->MakeOutputArray({min, max}); - this->AssertBinop(arrow::compute::Add, left, right, expected); - } - { - // Subtraction - auto left = this->MakeInputArray({min, max}); - auto right = this->MakeInputArray({1, -1}); - auto expected = this->MakeOutputArray({max, min}); - this->AssertBinop(arrow::compute::Subtract, left, right, expected); - } - { - // Multiplication - auto left = this->MakeInputArray({min, max}); - auto right = this->MakeInputArray({-1, 2}); - auto expected = this->MakeOutputArray({min, -2}); - this->AssertBinop(arrow::compute::Multiply, left, right, expected); - } +TYPED_TEST(TestBinaryArithmeticsUnsigned, OverflowWraps) { + using CType = typename TestFixture::CType; + + auto min = std::numeric_limits::lowest(); + auto max = std::numeric_limits::max(); + + this->AssertBinop(arrow::compute::Add, MakeArray(min, max, max), + MakeArray(CType(-1), 1, max), MakeArray(max, min, CType(-2))); + + 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, Sub) { @@ -329,21 +275,19 @@ TYPED_TEST(TestBinaryArithmeticsFloating, Sub) { this->AssertBinop(arrow::compute::Subtract, "[1.1, 2.4, 3.5, 4.3, 5.1, 6.8, 7.3]", "[0.1, 1.2, 2.3, 3.4, 4.5, 5.6, 6.7]", - "[1.0, 1.2, 1.2, 0.9, 0.6, 1.2, 0.6]", /*compare_as_string=*/true); + "[1.0, 1.2, 1.2, 0.9, 0.6, 1.2, 0.6]"); this->AssertBinop(arrow::compute::Subtract, "[7, 6, 5, 4, 3, 2, 1]", "[6, 5, 4, 3, 2, 1, 0]", "[1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0]"); this->AssertBinop(arrow::compute::Subtract, "[10.4, 12, 4.2, 50, 50.3, 32, 11]", - "[2, 0, 6, 1, 5, 3, 4]", "[8.4, 12, -1.8, 49, 45.3, 29, 7]", - /*compare_as_string=*/true); + "[2, 0, 6, 1, 5, 3, 4]", "[8.4, 12, -1.8, 49, 45.3, 29, 7]"); this->AssertBinop(arrow::compute::Subtract, "[null, 1, 3.3, null, 2, 5.3]", - "[1, 4, 2, 5, 0, 3]", "[null, -3, 1.3, null, 2, 2.3]", - /*compare_as_string=*/true); + "[1, 4, 2, 5, 0, 3]", "[null, -3, 1.3, null, 2, 2.3]"); this->AssertBinop(arrow::compute::Subtract, 0.1, "[null, 1, 3.3, null, 2, 5.3]", - "[null, -0.9, -3.2, null, -1.9, -5.2]", /*compare_as_string=*/true); + "[null, -0.9, -3.2, null, -1.9, -5.2]"); } } // namespace compute diff --git a/cpp/src/arrow/testing/gtest_util.cc b/cpp/src/arrow/testing/gtest_util.cc index a91fc834a64..280a6dd56a2 100644 --- a/cpp/src/arrow/testing/gtest_util.cc +++ b/cpp/src/arrow/testing/gtest_util.cc @@ -82,6 +82,21 @@ void AssertArraysEqual(const Array& expected, const Array& actual, bool verbose) } } +void AssertArraysApproxEqual(const Array& expected, const Array& actual, bool verbose) { + std::stringstream diff; + if (!expected.ApproxEquals(actual, EqualOptions().diff_sink(&diff))) { + if (verbose) { + ::arrow::PrettyPrintOptions options(/*indent=*/2); + options.window = 50; + diff << "Expected:\n"; + ARROW_EXPECT_OK(PrettyPrint(expected, options, &diff)); + diff << "\nActual:\n"; + ARROW_EXPECT_OK(PrettyPrint(actual, options, &diff)); + } + FAIL() << diff.str(); + } +} + void AssertScalarsEqual(const Scalar& expected, const Scalar& actual, bool verbose) { std::stringstream diff; // ARROW-8956, ScalarEquals returns false when both are null diff --git a/cpp/src/arrow/testing/gtest_util.h b/cpp/src/arrow/testing/gtest_util.h index d84db73fdd4..32c338ab538 100644 --- a/cpp/src/arrow/testing/gtest_util.h +++ b/cpp/src/arrow/testing/gtest_util.h @@ -41,13 +41,6 @@ #include "arrow/util/macros.h" #include "arrow/util/visibility.h" -namespace arrow { - -template -class Result; - -} // namespace arrow - // NOTE: failing must be inline in the macros below, to get correct file / line number // reporting on test failures. @@ -167,6 +160,8 @@ struct Datum; // If verbose is true, then the arrays will be pretty printed ARROW_EXPORT void AssertArraysEqual(const Array& expected, const Array& actual, bool verbose = false); +ARROW_EXPORT void AssertArraysApproxEqual(const Array& expected, const Array& actual, + bool verbose = false); // Returns true when values are both null ARROW_EXPORT void AssertScalarsEqual(const Scalar& expected, const Scalar& actual, bool verbose = false); From 870bcc1bcdf14b63557167ab51faa08e3eb44f75 Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Mon, 8 Jun 2020 17:36:53 -0400 Subject: [PATCH 23/23] silence conversion warning error for MSVC --- cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index ae52cff056c..2f2159ef642 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -263,7 +263,7 @@ TYPED_TEST(TestBinaryArithmeticsFloating, Add) { this->AssertBinop(arrow::compute::Add, "[null, 1, 3.3, null, 2, 5.3]", "[1, 4, 2, 5, 0, 3]", "[null, 5, 5.3, null, 2, 8.3]"); - this->AssertBinop(arrow::compute::Add, 1.1, "[null, 1, 3.3, null, 2, 5.3]", + this->AssertBinop(arrow::compute::Add, 1.1F, "[null, 1, 3.3, null, 2, 5.3]", "[null, 2.1, 4.4, null, 3.1, 6.4]"); } @@ -286,7 +286,7 @@ TYPED_TEST(TestBinaryArithmeticsFloating, Sub) { this->AssertBinop(arrow::compute::Subtract, "[null, 1, 3.3, null, 2, 5.3]", "[1, 4, 2, 5, 0, 3]", "[null, -3, 1.3, null, 2, 2.3]"); - this->AssertBinop(arrow::compute::Subtract, 0.1, "[null, 1, 3.3, null, 2, 5.3]", + this->AssertBinop(arrow::compute::Subtract, 0.1F, "[null, 1, 3.3, null, 2, 5.3]", "[null, -0.9, -3.2, null, -1.9, -5.2]"); }