From 04d46f4e58586864f075854eb15a6b3b531ad478 Mon Sep 17 00:00:00 2001 From: David Li Date: Thu, 8 Jul 2021 14:49:15 -0400 Subject: [PATCH 1/3] ARROW-13289: [C++] Accept integer args in trig/log functions via promotion to double --- .../compute/kernels/scalar_arithmetic.cc | 41 +++++++++++++++++-- .../compute/kernels/scalar_arithmetic_test.cc | 20 +++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index f0eabf1b40e..3115a7ad248 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -1076,6 +1076,38 @@ struct ArithmeticFunction : ScalarFunction { } }; +/// An ArithmeticFunction that promotes integer arguments to double. +struct ArithmeticFloatingPointFunction : public ArithmeticFunction { + using ArithmeticFunction::ArithmeticFunction; + + Result DispatchBest(std::vector* values) const override { + RETURN_NOT_OK(CheckArity(*values)); + RETURN_NOT_OK(CheckDecimals(values)); + + using arrow::compute::detail::DispatchExactImpl; + if (auto kernel = DispatchExactImpl(this, *values)) return kernel; + + EnsureDictionaryDecoded(values); + + // Only promote types for binary functions + if (values->size() == 2) { + ReplaceNullWithOtherType(values); + } + + for (auto& descr : *values) { + if (is_integer(descr.type->id())) { + descr.type = float64(); + } + } + if (auto type = CommonNumeric(*values)) { + ReplaceTypes(type, values); + } + + if (auto kernel = DispatchExactImpl(this, *values)) return kernel; + return arrow::compute::detail::NoMatchingKernel(this, *values); + } +}; + template std::shared_ptr MakeArithmeticFunction(std::string name, const FunctionDoc* doc) { @@ -1164,7 +1196,8 @@ std::shared_ptr MakeShiftFunctionNotNull(std::string name, template std::shared_ptr MakeUnaryArithmeticFunctionFloatingPoint( std::string name, const FunctionDoc* doc) { - auto func = std::make_shared(name, Arity::Unary(), doc); + auto func = + std::make_shared(name, Arity::Unary(), doc); for (const auto& ty : FloatingPointTypes()) { auto output = is_integer(ty->id()) ? float64() : ty; auto exec = GenerateArithmeticFloatingPoint(ty); @@ -1176,7 +1209,8 @@ std::shared_ptr MakeUnaryArithmeticFunctionFloatingPoint( template std::shared_ptr MakeUnaryArithmeticFunctionFloatingPointNotNull( std::string name, const FunctionDoc* doc) { - auto func = std::make_shared(name, Arity::Unary(), doc); + auto func = + std::make_shared(name, Arity::Unary(), doc); for (const auto& ty : FloatingPointTypes()) { auto output = is_integer(ty->id()) ? float64() : ty; auto exec = GenerateArithmeticFloatingPoint(ty); @@ -1188,7 +1222,8 @@ std::shared_ptr MakeUnaryArithmeticFunctionFloatingPointNotNull( template std::shared_ptr MakeArithmeticFunctionFloatingPoint( std::string name, const FunctionDoc* doc) { - auto func = std::make_shared(name, Arity::Binary(), doc); + auto func = + std::make_shared(name, Arity::Binary(), doc); for (const auto& ty : FloatingPointTypes()) { auto output = is_integer(ty->id()) ? float64() : ty; auto exec = GenerateArithmeticFloatingPoint(ty); diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 877b6f31160..1dfdb12ce86 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -1042,6 +1042,26 @@ TEST(TestUnaryArithmetic, DispatchBest) { for (std::string name : {"negate", "negate_checked", "abs", "abs_checked"}) { CheckDispatchFails(name, {null()}); } + + for (std::string name : + {"ln", "log2", "log10", "log1p", "sin", "cos", "tan", "asin", "acos"}) { + for (std::string suffix : {"", "_checked"}) { + name += suffix; + + CheckDispatchBest(name, {int32()}, {float64()}); + CheckDispatchBest(name, {uint8()}, {float64()}); + + CheckDispatchBest(name, {dictionary(int8(), int64())}, {float64()}); + } + } + + CheckDispatchBest("atan", {int32()}, {float64()}); + CheckDispatchBest("atan2", {int32(), float64()}, {float64(), float64()}); + CheckDispatchBest("atan2", {int32(), uint8()}, {float64(), float64()}); + CheckDispatchBest("atan2", {int32(), null()}, {float64(), float64()}); + CheckDispatchBest("atan2", {float32(), float64()}, {float64(), float64()}); + // Integer always promotes to double + CheckDispatchBest("atan2", {float32(), int8()}, {float64(), float64()}); } TYPED_TEST(TestUnaryArithmeticSigned, Negate) { From 3a0791574d2654bb42e1ca1329166f7d5f2291ff Mon Sep 17 00:00:00 2001 From: David Li Date: Mon, 12 Jul 2021 02:31:37 -0400 Subject: [PATCH 2/3] ARROW-13289: [C++] Add sanity check tests for log(int) --- .../compute/kernels/scalar_arithmetic.cc | 1 - .../compute/kernels/scalar_arithmetic_test.cc | 39 ++++++++++++++++++- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 3115a7ad248..db73294e1fa 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -1089,7 +1089,6 @@ struct ArithmeticFloatingPointFunction : public ArithmeticFunction { EnsureDictionaryDecoded(values); - // Only promote types for binary functions if (values->size() == 2) { ReplaceNullWithOtherType(values); } diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 1dfdb12ce86..44bc0ac0cf6 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -1843,7 +1843,6 @@ TYPED_TEST(TestBinaryArithmeticFloating, TrigAtan2) { TYPED_TEST(TestUnaryArithmeticFloating, Log) { using CType = typename TestFixture::CType; - auto ty = this->type_singleton(); this->SetNansEqual(true); auto min_val = std::numeric_limits::min(); auto max_val = std::numeric_limits::max(); @@ -1901,5 +1900,43 @@ TYPED_TEST(TestUnaryArithmeticFloating, Log) { Log1p(lowest_val, this->options_)); } +TYPED_TEST(TestUnaryArithmeticIntegral, Log) { + auto ty = this->type_singleton(); + for (auto check_overflow : {false, true}) { + this->SetOverflowCheck(check_overflow); + this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[1, null]"), + ArrayFromJSON(float64(), "[0, null]")); + this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[1, 10, null]"), + ArrayFromJSON(float64(), "[0, 1, null]")); + this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[1, 2, null]"), + ArrayFromJSON(float64(), "[0, 1, null]")); + this->AssertUnaryOp(Log1p, ArrayFromJSON(ty, "[0, null]"), + ArrayFromJSON(float64(), "[0, null]")); + } +} + +TYPED_TEST(TestUnaryArithmeticSigned, Log) { + auto ty = this->type_singleton(); + this->SetNansEqual(true); + this->SetOverflowCheck(false); + this->AssertUnaryOp(Ln, ArrayFromJSON(ty, "[-1, 0]"), + ArrayFromJSON(float64(), "[NaN, -Inf]")); + this->AssertUnaryOp(Log10, ArrayFromJSON(ty, "[-1, 0]"), + ArrayFromJSON(float64(), "[NaN, -Inf]")); + this->AssertUnaryOp(Log2, ArrayFromJSON(ty, "[-1, 0]"), + ArrayFromJSON(float64(), "[NaN, -Inf]")); + this->AssertUnaryOp(Log1p, ArrayFromJSON(ty, "[-2, -1]"), + ArrayFromJSON(float64(), "[NaN, -Inf]")); + this->SetOverflowCheck(true); + this->AssertUnaryOpRaises(Ln, "[0]", "logarithm of zero"); + this->AssertUnaryOpRaises(Ln, "[-1]", "logarithm of negative number"); + this->AssertUnaryOpRaises(Log10, "[0]", "logarithm of zero"); + this->AssertUnaryOpRaises(Log10, "[-1]", "logarithm of negative number"); + this->AssertUnaryOpRaises(Log2, "[0]", "logarithm of zero"); + this->AssertUnaryOpRaises(Log2, "[-1]", "logarithm of negative number"); + this->AssertUnaryOpRaises(Log1p, "[-1]", "logarithm of zero"); + this->AssertUnaryOpRaises(Log1p, "[-2]", "logarithm of negative number"); +} + } // namespace compute } // namespace arrow From 79bf5dde8aa2450375717f7b2a5e71be5efa9b43 Mon Sep 17 00:00:00 2001 From: David Li Date: Mon, 12 Jul 2021 02:43:08 -0400 Subject: [PATCH 3/3] ARROW-13289: [C++] Add sanity check tests for trig(int) --- .../compute/kernels/scalar_arithmetic_test.cc | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index 44bc0ac0cf6..e37fb93fac2 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -1841,6 +1841,39 @@ TYPED_TEST(TestBinaryArithmeticFloating, TrigAtan2) { -M_PI_2, 0, M_PI)); } +TYPED_TEST(TestUnaryArithmeticIntegral, Trig) { + // Integer arguments promoted to double, sanity check here + auto ty = this->type_singleton(); + auto atan = [](const Datum& arg, ArithmeticOptions, ExecContext* ctx) { + return Atan(arg, ctx); + }; + for (auto check_overflow : {false, true}) { + this->SetOverflowCheck(check_overflow); + this->AssertUnaryOp(Sin, ArrayFromJSON(ty, "[0, 1]"), + ArrayFromJSON(float64(), "[0, 0.8414709848078965]")); + this->AssertUnaryOp(Cos, ArrayFromJSON(ty, "[0, 1]"), + ArrayFromJSON(float64(), "[1, 0.5403023058681398]")); + this->AssertUnaryOp(Tan, ArrayFromJSON(ty, "[0, 1]"), + ArrayFromJSON(float64(), "[0, 1.5574077246549023]")); + this->AssertUnaryOp(Asin, ArrayFromJSON(ty, "[0, 1]"), + ArrayFromJSON(float64(), MakeArray(0, M_PI_2))); + this->AssertUnaryOp(Acos, ArrayFromJSON(ty, "[0, 1]"), + ArrayFromJSON(float64(), MakeArray(M_PI_2, 0))); + this->AssertUnaryOp(atan, ArrayFromJSON(ty, "[0, 1]"), + ArrayFromJSON(float64(), MakeArray(0, M_PI_4))); + } +} + +TYPED_TEST(TestBinaryArithmeticIntegral, Trig) { + // Integer arguments promoted to double, sanity check here + auto ty = this->type_singleton(); + auto atan2 = [](const Datum& y, const Datum& x, ArithmeticOptions, ExecContext* ctx) { + return Atan2(y, x, ctx); + }; + this->AssertBinop(atan2, ArrayFromJSON(ty, "[0, 1]"), ArrayFromJSON(ty, "[1, 0]"), + ArrayFromJSON(float64(), MakeArray(0, M_PI_2))); +} + TYPED_TEST(TestUnaryArithmeticFloating, Log) { using CType = typename TestFixture::CType; this->SetNansEqual(true); @@ -1901,6 +1934,7 @@ TYPED_TEST(TestUnaryArithmeticFloating, Log) { } TYPED_TEST(TestUnaryArithmeticIntegral, Log) { + // Integer arguments promoted to double, sanity check here auto ty = this->type_singleton(); for (auto check_overflow : {false, true}) { this->SetOverflowCheck(check_overflow); @@ -1916,6 +1950,7 @@ TYPED_TEST(TestUnaryArithmeticIntegral, Log) { } TYPED_TEST(TestUnaryArithmeticSigned, Log) { + // Integer arguments promoted to double, sanity check here auto ty = this->type_singleton(); this->SetNansEqual(true); this->SetOverflowCheck(false);