From 79cb5807e1cca5462acbc4054f383c552ff9bce0 Mon Sep 17 00:00:00 2001 From: Eduardo Ponce Date: Thu, 9 Sep 2021 06:08:40 -0400 Subject: [PATCH 1/4] add floor_divide and remainder compute functions --- cpp/src/arrow/compute/api_scalar.cc | 2 + cpp/src/arrow/compute/api_scalar.h | 27 ++++++ .../kernels/base_arithmetic_internal.h | 84 +++++++++++++++++++ .../compute/kernels/scalar_arithmetic.cc | 51 +++++++++++ .../compute/kernels/scalar_arithmetic_test.cc | 16 ++++ 5 files changed, 180 insertions(+) diff --git a/cpp/src/arrow/compute/api_scalar.cc b/cpp/src/arrow/compute/api_scalar.cc index 425274043ed..064b947ecee 100644 --- a/cpp/src/arrow/compute/api_scalar.cc +++ b/cpp/src/arrow/compute/api_scalar.cc @@ -699,6 +699,8 @@ SCALAR_ARITHMETIC_BINARY(Add, "add", "add_checked") SCALAR_ARITHMETIC_BINARY(Divide, "divide", "divide_checked") SCALAR_ARITHMETIC_BINARY(Logb, "logb", "logb_checked") SCALAR_ARITHMETIC_BINARY(Multiply, "multiply", "multiply_checked") +SCALAR_ARITHMETIC_BINARY(FloorDivide, "floor_divide", "floor_divide_checked") +SCALAR_ARITHMETIC_BINARY(Remainder, "remainder", "remainder_checked") SCALAR_ARITHMETIC_BINARY(Power, "power", "power_checked") SCALAR_ARITHMETIC_BINARY(ShiftLeft, "shift_left", "shift_left_checked") SCALAR_ARITHMETIC_BINARY(ShiftRight, "shift_right", "shift_right_checked") diff --git a/cpp/src/arrow/compute/api_scalar.h b/cpp/src/arrow/compute/api_scalar.h index 1c27757fcfc..3e7826babba 100644 --- a/cpp/src/arrow/compute/api_scalar.h +++ b/cpp/src/arrow/compute/api_scalar.h @@ -605,6 +605,33 @@ Result Divide(const Datum& left, const Datum& right, ArithmeticOptions options = ArithmeticOptions(), ExecContext* ctx = NULLPTR); +/// \brief Divide two values and return the largest integer smaller or equal to the +/// quotient. Array values must be the same length. If either argument is null the result +/// will be null. For integer types, if there is a zero divisor, an error will be raised. +/// +/// \param[in] left the dividend +/// \param[in] right the divisor +/// \param[in] options arithmetic options (enable/disable overflow checking), optional +/// \param[in] ctx the function execution context, optional +/// \return the elementwise quotient without the fractional part +ARROW_EXPORT +Result FloorDivide(const Datum& left, const Datum& right, + ArithmeticOptions options = ArithmeticOptions(), + ExecContext* ctx = NULLPTR); + +/// \brief Calculate the remainder of dividing two values. Array values must be the same +/// length. If either argument is null the result will be null. +/// +/// \param[in] left the dividend +/// \param[in] right the divisor +/// \param[in] options arithmetic options (enable/disable overflow checking), optional +/// \param[in] ctx the function execution context, optional +/// \return the elementwise remainder +ARROW_EXPORT +Result Remainder(const Datum& left, const Datum& right, + ArithmeticOptions options = ArithmeticOptions(), + ExecContext* ctx = NULLPTR); + /// \brief Negate values. /// /// If argument is null the result will be null. diff --git a/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h b/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h index 1cccdca1481..802ef843221 100644 --- a/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h +++ b/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h @@ -425,6 +425,90 @@ struct DivideChecked { } }; +struct FloorDivide { + template + static enable_if_floating_point Call(KernelContext*, Arg0 left, Arg1 right, + Status*) { + return std::floor(left / right); + } + + template + static enable_if_c_integer Call(KernelContext*, Arg0 left, Arg1 right, Status* st) { + T result; + if (ARROW_PREDICT_FALSE(DivideWithOverflow(left, right, &result))) { + if (ARROW_PREDICT_FALSE(right == 0)) { + *st = Status::Invalid("divide by zero"); + result = 0; + } else { + result = 0; + } + } + return result; + } +}; + +struct FloorDivideChecked { + template + static enable_if_c_integer Call(KernelContext*, Arg0 left, Arg1 right, Status* st) { + static_assert(std::is_same::value && std::is_same::value, ""); + T result; + if (ARROW_PREDICT_FALSE(DivideWithOverflow(left, right, &result))) { + if (ARROW_PREDICT_FALSE(right == 0)) { + *st = Status::Invalid("divide by zero"); + result = 0; + } else { + *st = Status::Invalid("overflow"); + } + } + return result; + } + + template + static enable_if_floating_point Call(KernelContext*, Arg0 left, Arg1 right, + Status* st) { + static_assert(std::is_same::value && std::is_same::value, ""); + if (ARROW_PREDICT_FALSE(right == 0)) { + *st = Status::Invalid("divide by zero"); + return 0; + } + return std::floor(left / right); + } +}; + +struct Remainder { + template + static enable_if_floating_point Call(KernelContext*, Arg0 left, Arg1 right, + Status*) { + return std::fmod(left, right); + } + + template + static enable_if_c_integer Call(KernelContext*, Arg0 left, Arg1 right, Status* st) { + if (ARROW_PREDICT_FALSE(right == 0)) { + *st = Status::Invalid("divide by zero"); + return 0; + } + return left % right; + } +}; + +struct RemainderChecked { + template + static enable_if_floating_point Call(KernelContext*, Arg0 left, Arg1 right, + Status*) { + return std::fmod(left, right); + } + + template + static enable_if_c_integer Call(KernelContext*, Arg0 left, Arg1 right, Status* st) { + if (ARROW_PREDICT_FALSE(right == 0)) { + *st = Status::Invalid("divide by zero"); + return 0; + } + return left % right; + } +}; + struct Negate { template static constexpr enable_if_floating_value Call(KernelContext*, Arg arg, Status*) { diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 79e7d0fee1a..6f62e84029a 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -1699,6 +1699,34 @@ const FunctionDoc div_checked_doc{ "integer overflow is encountered."), {"dividend", "divisor"}}; +const FunctionDoc floor_div_doc{ + "Calculate the quotient without the fractional part", + ("Integer division by zero returns an error. However, integer overflow\n" + "wraps around, and floating-point division by zero returns an infinite.\n" + "Use function \"floor_divide_checked\" if you want to get an error\n" + "in all the aforementioned cases."), + {"dividend", "divisor"}}; + +const FunctionDoc floor_div_checked_doc{ + "Calculate the quotient without the fractional part", + ("An error is returned when trying to divide by zero, or when\n" + "integer overflow is encountered."), + {"dividend", "divisor"}}; + +const FunctionDoc remainder_doc{ + "Calculate the remainder of dividing two values", + ("Integer division by zero returns an error. However, integer overflow\n" + "wraps around, and floating-point division by zero returns an infinite.\n" + "Use function \"remainder_checked\" if you want to get an error\n" + "in all the aforementioned cases."), + {"dividend", "divisor"}}; + +const FunctionDoc remainder_checked_doc{ + "Calculate the remainder of dividing two values", + ("An error is returned when trying to divide by zero, or when\n" + "integer overflow is encountered."), + {"dividend", "divisor"}}; + const FunctionDoc negate_doc{"Negate the argument element-wise", ("Results will wrap around on integer overflow.\n" "Use function \"negate_checked\" if you want overflow\n" @@ -2207,8 +2235,31 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) { DCHECK_OK(registry->AddFunction(std::move(divide_checked))); // ---------------------------------------------------------------------- +<<<<<<< HEAD auto negate = MakeUnaryArithmeticFunction("negate", negate_doc); AddDecimalUnaryKernels(negate.get()); +======= + auto floor_divide = + MakeArithmeticFunctionNotNull("floor_divide", &floor_div_doc); + DCHECK_OK(registry->AddFunction(std::move(floor_divide))); + + // ---------------------------------------------------------------------- + auto floor_divide_checked = MakeArithmeticFunctionNotNull( + "floor_divide_checked", &floor_div_checked_doc); + DCHECK_OK(registry->AddFunction(std::move(floor_divide_checked))); + + // ---------------------------------------------------------------------- + auto remainder = MakeArithmeticFunctionNotNull("remainder", &remainder_doc); + DCHECK_OK(registry->AddFunction(std::move(remainder))); + + // ---------------------------------------------------------------------- + auto remainder_checked = MakeArithmeticFunctionNotNull( + "remainder_checked", &remainder_checked_doc); + DCHECK_OK(registry->AddFunction(std::move(remainder_checked))); + + // ---------------------------------------------------------------------- + auto negate = MakeUnaryArithmeticFunction("negate", &negate_doc); +>>>>>>> 853bfca88 (add floor_divide and remainder compute functions) DCHECK_OK(registry->AddFunction(std::move(negate))); // ---------------------------------------------------------------------- diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc index fc0d5eb2e15..8c8fa16a249 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic_test.cc @@ -937,6 +937,22 @@ TYPED_TEST(TestBinaryArithmeticSigned, DivideOverflowRaises) { this->AssertBinop(Divide, MakeArray(min), MakeArray(-1), "[0]"); } +TYPED_TEST(TestBinaryArithmeticIntegral, Remainder) { + // Empty arrays + this->AssertBinop(Remainder, "[]", "[]", "[]"); + // Ordinary arrays + this->AssertBinop(Remainder, "[3, 2, 6]", "[1, 1, 2]", "[0, 0, 0]"); + // Array with nulls + this->AssertBinop(Remainder, "[null, 10, 30, null, 20]", "[1, 4, 2, 5, 10]", + "[null, 2, 0, null, 0]"); + // Scalar divides by array + this->AssertBinop(Remainder, 33, "[null, 1, 3, null, 2]", "[null, 0, 0, null, 1]"); + // Array divides by scalar + this->AssertBinop(Remainder, "[null, 10, 30, null, 2]", 3, "[null, 1, 0, null, 2]"); + // Scalar divides by scalar + this->AssertBinop(Remainder, 16, 7, 2); +} + TYPED_TEST(TestBinaryArithmeticFloating, Power) { using CType = typename TestFixture::CType; auto max = std::numeric_limits::max(); From 11538fa20bdcf53430d59837fc8a77b299f5e43a Mon Sep 17 00:00:00 2001 From: Eduardo Ponce Date: Tue, 19 Jul 2022 12:01:08 -0400 Subject: [PATCH 2/4] minor change to API comments --- cpp/src/arrow/compute/api_scalar.cc | 2 +- cpp/src/arrow/compute/api_scalar.h | 17 +++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/cpp/src/arrow/compute/api_scalar.cc b/cpp/src/arrow/compute/api_scalar.cc index 064b947ecee..dd399c69c9e 100644 --- a/cpp/src/arrow/compute/api_scalar.cc +++ b/cpp/src/arrow/compute/api_scalar.cc @@ -700,7 +700,7 @@ SCALAR_ARITHMETIC_BINARY(Divide, "divide", "divide_checked") SCALAR_ARITHMETIC_BINARY(Logb, "logb", "logb_checked") SCALAR_ARITHMETIC_BINARY(Multiply, "multiply", "multiply_checked") SCALAR_ARITHMETIC_BINARY(FloorDivide, "floor_divide", "floor_divide_checked") -SCALAR_ARITHMETIC_BINARY(Remainder, "remainder", "remainder_checked") +SCALAR_ARITHMETIC_BINARY(Modulo, "modulo", "modulo_checked") SCALAR_ARITHMETIC_BINARY(Power, "power", "power_checked") SCALAR_ARITHMETIC_BINARY(ShiftLeft, "shift_left", "shift_left_checked") SCALAR_ARITHMETIC_BINARY(ShiftRight, "shift_right", "shift_right_checked") diff --git a/cpp/src/arrow/compute/api_scalar.h b/cpp/src/arrow/compute/api_scalar.h index 3e7826babba..adfaedc2e39 100644 --- a/cpp/src/arrow/compute/api_scalar.h +++ b/cpp/src/arrow/compute/api_scalar.h @@ -606,11 +606,13 @@ Result Divide(const Datum& left, const Datum& right, ExecContext* ctx = NULLPTR); /// \brief Divide two values and return the largest integer smaller or equal to the -/// quotient. Array values must be the same length. If either argument is null the result +/// quotient. +/// +/// Array values must be the same length. If either argument is null the result /// will be null. For integer types, if there is a zero divisor, an error will be raised. /// -/// \param[in] left the dividend -/// \param[in] right the divisor +/// \param[in] dividend the dividend +/// \param[in] divisor the divisor /// \param[in] options arithmetic options (enable/disable overflow checking), optional /// \param[in] ctx the function execution context, optional /// \return the elementwise quotient without the fractional part @@ -619,8 +621,11 @@ Result FloorDivide(const Datum& left, const Datum& right, ArithmeticOptions options = ArithmeticOptions(), ExecContext* ctx = NULLPTR); -/// \brief Calculate the remainder of dividing two values. Array values must be the same -/// length. If either argument is null the result will be null. +/// \brief Calculate the remainder of dividing two values. +/// +/// Array values must be the same length. +/// If either argument is null the result will be null. +/// If the divisor is zero, an error will be raised. /// /// \param[in] left the dividend /// \param[in] right the divisor @@ -628,7 +633,7 @@ Result FloorDivide(const Datum& left, const Datum& right, /// \param[in] ctx the function execution context, optional /// \return the elementwise remainder ARROW_EXPORT -Result Remainder(const Datum& left, const Datum& right, +Result Modulo(const Datum& left, const Datum& right, ArithmeticOptions options = ArithmeticOptions(), ExecContext* ctx = NULLPTR); From 44d44452480fc5092ea90ff3b53aaeaa5caa4cb7 Mon Sep 17 00:00:00 2001 From: Eduardo Ponce Date: Mon, 5 Sep 2022 16:59:11 -0400 Subject: [PATCH 3/4] resolve conflicts from rebase --- cpp/src/arrow/compute/kernels/scalar_arithmetic.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 6f62e84029a..828050c2451 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -2235,10 +2235,6 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) { DCHECK_OK(registry->AddFunction(std::move(divide_checked))); // ---------------------------------------------------------------------- -<<<<<<< HEAD - auto negate = MakeUnaryArithmeticFunction("negate", negate_doc); - AddDecimalUnaryKernels(negate.get()); -======= auto floor_divide = MakeArithmeticFunctionNotNull("floor_divide", &floor_div_doc); DCHECK_OK(registry->AddFunction(std::move(floor_divide))); @@ -2259,7 +2255,7 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) { // ---------------------------------------------------------------------- auto negate = MakeUnaryArithmeticFunction("negate", &negate_doc); ->>>>>>> 853bfca88 (add floor_divide and remainder compute functions) + AddDecimalUnaryKernels(negate.get()); DCHECK_OK(registry->AddFunction(std::move(negate))); // ---------------------------------------------------------------------- From dcad18f30e7ea14279edadfe8da18d6283cf91b3 Mon Sep 17 00:00:00 2001 From: Eduardo Ponce Date: Thu, 15 Dec 2022 09:22:51 -0500 Subject: [PATCH 4/4] update type traits and passing of FunctionDoc --- .../arrow/compute/kernels/base_arithmetic_internal.h | 12 ++++++++---- cpp/src/arrow/compute/kernels/scalar_arithmetic.cc | 10 +++++----- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h b/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h index 802ef843221..c445baccc3b 100644 --- a/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h +++ b/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h @@ -433,7 +433,8 @@ struct FloorDivide { } template - static enable_if_c_integer Call(KernelContext*, Arg0 left, Arg1 right, Status* st) { + static enable_if_integer_value Call(KernelContext*, Arg0 left, Arg1 right, + Status* st) { T result; if (ARROW_PREDICT_FALSE(DivideWithOverflow(left, right, &result))) { if (ARROW_PREDICT_FALSE(right == 0)) { @@ -449,7 +450,8 @@ struct FloorDivide { struct FloorDivideChecked { template - static enable_if_c_integer Call(KernelContext*, Arg0 left, Arg1 right, Status* st) { + static enable_if_integer_value Call(KernelContext*, Arg0 left, Arg1 right, + Status* st) { static_assert(std::is_same::value && std::is_same::value, ""); T result; if (ARROW_PREDICT_FALSE(DivideWithOverflow(left, right, &result))) { @@ -483,7 +485,8 @@ struct Remainder { } template - static enable_if_c_integer Call(KernelContext*, Arg0 left, Arg1 right, Status* st) { + static enable_if_integer_value Call(KernelContext*, Arg0 left, Arg1 right, + Status* st) { if (ARROW_PREDICT_FALSE(right == 0)) { *st = Status::Invalid("divide by zero"); return 0; @@ -500,7 +503,8 @@ struct RemainderChecked { } template - static enable_if_c_integer Call(KernelContext*, Arg0 left, Arg1 right, Status* st) { + static enable_if_integer_value Call(KernelContext*, Arg0 left, Arg1 right, + Status* st) { if (ARROW_PREDICT_FALSE(right == 0)) { *st = Status::Invalid("divide by zero"); return 0; diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc index 828050c2451..dade23cba13 100644 --- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc +++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc @@ -2236,25 +2236,25 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) { // ---------------------------------------------------------------------- auto floor_divide = - MakeArithmeticFunctionNotNull("floor_divide", &floor_div_doc); + MakeArithmeticFunctionNotNull("floor_divide", floor_div_doc); DCHECK_OK(registry->AddFunction(std::move(floor_divide))); // ---------------------------------------------------------------------- auto floor_divide_checked = MakeArithmeticFunctionNotNull( - "floor_divide_checked", &floor_div_checked_doc); + "floor_divide_checked", floor_div_checked_doc); DCHECK_OK(registry->AddFunction(std::move(floor_divide_checked))); // ---------------------------------------------------------------------- - auto remainder = MakeArithmeticFunctionNotNull("remainder", &remainder_doc); + auto remainder = MakeArithmeticFunctionNotNull("remainder", remainder_doc); DCHECK_OK(registry->AddFunction(std::move(remainder))); // ---------------------------------------------------------------------- auto remainder_checked = MakeArithmeticFunctionNotNull( - "remainder_checked", &remainder_checked_doc); + "remainder_checked", remainder_checked_doc); DCHECK_OK(registry->AddFunction(std::move(remainder_checked))); // ---------------------------------------------------------------------- - auto negate = MakeUnaryArithmeticFunction("negate", &negate_doc); + auto negate = MakeUnaryArithmeticFunction("negate", negate_doc); AddDecimalUnaryKernels(negate.get()); DCHECK_OK(registry->AddFunction(std::move(negate)));