This is an automated email from the ASF dual-hosted git repository.
kou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new cca5eec5fd GH-39156: [C++][Compute] Fix negative duration division
(#39158)
cca5eec5fd is described below
commit cca5eec5fd853d4593dfe1b6c158e9543d32619f
Author: Jin Shang <[email protected]>
AuthorDate: Sun Dec 10 09:57:56 2023 +0800
GH-39156: [C++][Compute] Fix negative duration division (#39158)
### Rationale for this change
I forgot to cast durations to doubles in the current `division(duration,
duration)` kernel. So they were essentially `reinterpret_cast`ed to double.
Because I only tested small positive ints but not large ints or negative ints,
I missed this bug.
### What changes are included in this PR?
Add a `FloatingDivide` operator that casts ints to doubles and do floating
division. Replace the `division(duration, duration)` with this op.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
No.
* Closes: #39156
Authored-by: Jin Shang <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
---
.../compute/kernels/base_arithmetic_internal.h | 38 ++++++++++++++++++++++
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc | 7 ++--
.../arrow/compute/kernels/scalar_temporal_test.cc | 8 ++---
3 files changed, 46 insertions(+), 7 deletions(-)
diff --git a/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h
b/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h
index 7798c61577..d59320d270 100644
--- a/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h
+++ b/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h
@@ -426,6 +426,44 @@ struct DivideChecked {
}
};
+struct FloatingDivide {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_floating_value<Arg0> Call(KernelContext*, Arg0 left, Arg1
right,
+ Status*) {
+ return left / right;
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer_value<Arg0, double> Call(KernelContext* ctx, Arg0
left,
+ Arg1 right, Status* st) {
+ static_assert(std::is_same<Arg0, Arg1>::value);
+ return Call<double>(ctx, static_cast<double>(left),
static_cast<double>(right), st);
+ }
+
+ // TODO: Add decimal
+};
+
+struct FloatingDivideChecked {
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_floating_value<Arg0> Call(KernelContext*, Arg0 left, Arg1
right,
+ Status* st) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<T,
Arg1>::value);
+ if (ARROW_PREDICT_FALSE(right == 0)) {
+ *st = Status::Invalid("divide by zero");
+ return 0;
+ }
+ return left / right;
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static enable_if_integer_value<Arg0, double> Call(KernelContext* ctx, Arg0
left,
+ Arg1 right, Status* st) {
+ static_assert(std::is_same<Arg0, Arg1>::value);
+ return Call<double>(ctx, static_cast<double>(left),
static_cast<double>(right), st);
+ }
+ // TODO: Add decimal
+};
+
struct Negate {
template <typename T, typename Arg>
static constexpr enable_if_floating_value<T> 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 c305028be1..ad33d7f895 100644
--- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
+++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
@@ -1513,7 +1513,8 @@ void RegisterScalarArithmetic(FunctionRegistry* registry)
{
// Add divide(duration, duration) -> float64
for (auto unit : TimeUnit::values()) {
- auto exec = ScalarBinaryNotNull<DoubleType, DoubleType, DoubleType,
Divide>::Exec;
+ auto exec =
+ ScalarBinaryNotNull<DoubleType, Int64Type, Int64Type,
FloatingDivide>::Exec;
DCHECK_OK(
divide->AddKernel({duration(unit), duration(unit)}, float64(),
std::move(exec)));
}
@@ -1533,8 +1534,8 @@ void RegisterScalarArithmetic(FunctionRegistry* registry)
{
// Add divide_checked(duration, duration) -> float64
for (auto unit : TimeUnit::values()) {
- auto exec =
- ScalarBinaryNotNull<DoubleType, DoubleType, DoubleType,
DivideChecked>::Exec;
+ auto exec = ScalarBinaryNotNull<DoubleType, Int64Type, Int64Type,
+ FloatingDivideChecked>::Exec;
DCHECK_OK(divide_checked->AddKernel({duration(unit), duration(unit)},
float64(),
std::move(exec)));
}
diff --git a/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
b/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
index 4c7975add0..d8bbe5ca8a 100644
--- a/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
+++ b/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
@@ -1722,12 +1722,12 @@ TEST_F(ScalarTemporalTest, TestTemporalDivideDuration) {
}
// div(duration, duration) -> float64
- auto left = ArrayFromJSON(duration(TimeUnit::SECOND), "[1, 2, 3, 4]");
- auto right = ArrayFromJSON(duration(TimeUnit::MILLI), "[4000, 300, 20, 1]");
+ auto left = ArrayFromJSON(duration(TimeUnit::SECOND), "[1, 2, -3, 4]");
+ auto right = ArrayFromJSON(duration(TimeUnit::MILLI), "[4000, -300, 20, 1]");
auto expected_left_by_right =
- ArrayFromJSON(float64(), "[0.25, 6.666666666666667, 150, 4000]");
+ ArrayFromJSON(float64(), "[0.25, -6.666666666666667, -150, 4000]");
auto expected_right_by_left =
- ArrayFromJSON(float64(), "[4, 0.15, 0.006666666666666667, 0.00025]");
+ ArrayFromJSON(float64(), "[4, -0.15, -0.006666666666666667, 0.00025]");
CheckScalarBinary("divide", left, right, expected_left_by_right);
CheckScalarBinary("divide_checked", left, right, expected_left_by_right);
CheckScalarBinary("divide", right, left, expected_right_by_left);
