From 0e7fabaf369187996f9c87648ddce7a4acbd3068 Mon Sep 17 00:00:00 2001
From: Rok <rok@mihevc.org>
Date: Fri, 11 Mar 2022 21:02:13 +0100
Subject: [PATCH 1/4] Temporal add/add_checked should be commutative.

---
 .../compute/kernels/codegen_internal_test.cc  |   4 +
 .../compute/kernels/scalar_arithmetic.cc      | 104 ++++++++++++++----
 .../compute/kernels/scalar_temporal_test.cc   |  14 +++
 3 files changed, 101 insertions(+), 21 deletions(-)

diff --git a/cpp/src/arrow/compute/kernels/codegen_internal_test.cc b/cpp/src/arrow/compute/kernels/codegen_internal_test.cc
index ba9493ea875..46d31c8ae4c 100644
--- a/cpp/src/arrow/compute/kernels/codegen_internal_test.cc
+++ b/cpp/src/arrow/compute/kernels/codegen_internal_test.cc
@@ -182,6 +182,8 @@ TEST(TestDispatchBest, CommonTemporalResolution) {
   ASSERT_EQ(TimeUnit::MILLI, CommonTemporalResolution(args.data(), args.size()));
   args = {duration(TimeUnit::SECOND), time32(TimeUnit::SECOND)};
   ASSERT_EQ(TimeUnit::SECOND, CommonTemporalResolution(args.data(), args.size()));
+  args = {duration(TimeUnit::SECOND), time64(TimeUnit::NANO)};
+  ASSERT_EQ(TimeUnit::NANO, CommonTemporalResolution(args.data(), args.size()));
   args = {time64(TimeUnit::MICRO), duration(TimeUnit::NANO)};
   ASSERT_EQ(TimeUnit::NANO, CommonTemporalResolution(args.data(), args.size()));
   args = {timestamp(TimeUnit::SECOND, tz), timestamp(TimeUnit::MICRO)};
@@ -204,6 +206,8 @@ TEST(TestDispatchBest, CommonTemporalResolution) {
   ASSERT_EQ(TimeUnit::NANO, CommonTemporalResolution(args.data(), args.size()));
   args = {duration(TimeUnit::SECOND), int64()};
   ASSERT_EQ(TimeUnit::SECOND, CommonTemporalResolution(args.data(), args.size()));
+  args = {duration(TimeUnit::MILLI), timestamp(TimeUnit::SECOND, tz)};
+  ASSERT_EQ(TimeUnit::MILLI, CommonTemporalResolution(args.data(), args.size()));
 }
 
 TEST(TestDispatchBest, ReplaceTemporalTypes) {
diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
index 2d543e23266..8b8b0046f4a 100644
--- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
+++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
@@ -240,11 +240,11 @@ struct SubtractCheckedDate32 {
   }
 };
 
-template <bool is_32bit, int64_t multiple>
+template <bool left_is_32bit, int64_t multiple>
 struct AddTimeDuration {
   template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
-                                       Status* st) {
+  static enable_if_t<(!std::is_same<Arg0, Arg1>::value && left_is_32bit), T> Call(
+      KernelContext*, Arg0 left, Arg1 right, Status* st) {
     T result = arrow::internal::SafeSignedAdd(left, static_cast<T>(right));
     if (result < 0 || multiple <= result) {
       *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
@@ -254,8 +254,19 @@ struct AddTimeDuration {
   }
 
   template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<!is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
-                                        Status* st) {
+  static enable_if_t<(!std::is_same<Arg0, Arg1>::value && !left_is_32bit), T> Call(
+      KernelContext*, Arg0 left, Arg1 right, Status* st) {
+    T result = arrow::internal::SafeSignedAdd(static_cast<T>(left), right);
+    if (result < 0 || multiple <= result) {
+      *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
+                            multiple, ") s");
+    }
+    return result;
+  }
+
+  template <typename T, typename Arg0, typename Arg1>
+  static enable_if_t<(std::is_same<Arg0, Arg1>::value), T> Call(KernelContext*, Arg0 left,
+                                                                Arg1 right, Status* st) {
     T result = arrow::internal::SafeSignedAdd(left, right);
     if (result < 0 || multiple <= result) {
       *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
@@ -265,11 +276,11 @@ struct AddTimeDuration {
   }
 };
 
-template <bool is_32bit, int64_t multiple>
+template <bool left_is_32bit, int64_t multiple>
 struct AddTimeDurationChecked {
   template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
-                                       Status* st) {
+  static enable_if_t<(!std::is_same<Arg0, Arg1>::value && left_is_32bit), T> Call(
+      KernelContext*, Arg0 left, Arg1 right, Status* st) {
     T result = 0;
     if (ARROW_PREDICT_FALSE(AddWithOverflow(left, static_cast<T>(right), &result))) {
       *st = Status::Invalid("overflow");
@@ -282,10 +293,24 @@ struct AddTimeDurationChecked {
   }
 
   template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<!is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
-                                        Status* st) {
+  static enable_if_t<(!std::is_same<Arg0, Arg1>::value && !left_is_32bit), T> Call(
+      KernelContext*, Arg0 left, Arg1 right, Status* st) {
     T result = 0;
-    if (ARROW_PREDICT_FALSE(AddWithOverflow(left, static_cast<T>(right), &result))) {
+    if (ARROW_PREDICT_FALSE(AddWithOverflow(static_cast<T>(left), right, &result))) {
+      *st = Status::Invalid("overflow");
+    }
+    if (result < 0 || multiple <= result) {
+      *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
+                            multiple, ") s");
+    }
+    return result;
+  }
+
+  template <typename T, typename Arg0, typename Arg1>
+  static enable_if_t<(std::is_same<Arg0, Arg1>::value), T> Call(KernelContext*, Arg0 left,
+                                                                Arg1 right, Status* st) {
+    T result = 0;
+    if (ARROW_PREDICT_FALSE(AddWithOverflow(left, right, &result))) {
       *st = Status::Invalid("overflow");
     }
     if (result < 0 || multiple <= result) {
@@ -341,7 +366,7 @@ struct SubtractTimeDurationChecked {
   static enable_if_t<!is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
                                         Status* st) {
     T result = 0;
-    if (ARROW_PREDICT_FALSE(SubtractWithOverflow(left, static_cast<T>(right), &result))) {
+    if (ARROW_PREDICT_FALSE(SubtractWithOverflow(left, right, &result))) {
       *st = Status::Invalid("overflow");
     }
     if (result < 0 || multiple <= result) {
@@ -2270,7 +2295,7 @@ std::shared_ptr<ScalarFunction> MakeArithmeticFunctionFloatingPointNotNull(
 }
 
 template <template <bool, int64_t> class Op>
-void AddArithmeticFunctionTimeDurations(std::shared_ptr<ScalarFunction> func) {
+void AddArithmeticFunctionTimeDuration(std::shared_ptr<ScalarFunction> func) {
   // Add Op(time32, duration) -> time32
   TimeUnit::type unit = TimeUnit::SECOND;
   auto exec_1 = ScalarBinary<Time32Type, Time32Type, DurationType, Op<true, 86400>>::Exec;
@@ -2297,6 +2322,35 @@ void AddArithmeticFunctionTimeDurations(std::shared_ptr<ScalarFunction> func) {
                             std::move(exec_4)));
 }
 
+template <template <bool, int64_t> class Op>
+void AddArithmeticFunctionDurationTime(std::shared_ptr<ScalarFunction> func) {
+  // Add Op(time32, duration) -> time32
+  TimeUnit::type unit = TimeUnit::SECOND;
+  auto exec_1 =
+      ScalarBinary<Time32Type, DurationType, Time32Type, Op<false, 86400>>::Exec;
+  DCHECK_OK(func->AddKernel({duration(unit), time32(unit)}, OutputType(LastType),
+                            std::move(exec_1)));
+
+  unit = TimeUnit::MILLI;
+  auto exec_2 =
+      ScalarBinary<Time32Type, DurationType, Time32Type, Op<false, 86400000>>::Exec;
+  DCHECK_OK(func->AddKernel({duration(unit), time32(unit)}, OutputType(LastType),
+                            std::move(exec_2)));
+
+  // Add Op(time64, duration) -> time64
+  unit = TimeUnit::MICRO;
+  auto exec_3 =
+      ScalarBinary<Time64Type, DurationType, Time64Type, Op<false, 86400000000>>::Exec;
+  DCHECK_OK(func->AddKernel({duration(unit), time64(unit)}, OutputType(LastType),
+                            std::move(exec_3)));
+
+  unit = TimeUnit::NANO;
+  auto exec_4 =
+      ScalarBinary<Time64Type, DurationType, Time64Type, Op<false, 86400000000000>>::Exec;
+  DCHECK_OK(func->AddKernel({duration(unit), time64(unit)}, OutputType(LastType),
+                            std::move(exec_4)));
+}
+
 const FunctionDoc absolute_value_doc{
     "Calculate the absolute value of the argument element-wise",
     ("Results will wrap around on integer overflow.\n"
@@ -2637,9 +2691,12 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
   // Add add(timestamp, duration) -> timestamp
   for (auto unit : TimeUnit::values()) {
     InputType in_type(match::TimestampTypeUnit(unit));
-    auto exec = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
+    auto exec1 = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
     DCHECK_OK(add->AddKernel({in_type, duration(unit)}, OutputType(FirstType),
-                             std::move(exec)));
+                             std::move(exec1)));
+    auto exec2 = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
+    DCHECK_OK(add->AddKernel({duration(unit), in_type}, OutputType(LastType),
+                             std::move(exec2)));
   }
 
   // Add add(duration, duration) -> duration
@@ -2649,7 +2706,8 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
     DCHECK_OK(add->AddKernel({in_type, in_type}, duration(unit), std::move(exec)));
   }
 
-  AddArithmeticFunctionTimeDurations<AddTimeDuration>(add);
+  AddArithmeticFunctionTimeDuration<AddTimeDuration>(add);
+  AddArithmeticFunctionDurationTime<AddTimeDuration>(add);
 
   DCHECK_OK(registry->AddFunction(std::move(add)));
 
@@ -2661,9 +2719,12 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
   // Add add_checked(timestamp, duration) -> timestamp
   for (auto unit : TimeUnit::values()) {
     InputType in_type(match::TimestampTypeUnit(unit));
-    auto exec = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
+    auto exec1 = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
     DCHECK_OK(add_checked->AddKernel({in_type, duration(unit)}, OutputType(FirstType),
-                                     std::move(exec)));
+                                     std::move(exec1)));
+    auto exec2 = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
+    DCHECK_OK(add_checked->AddKernel({duration(unit), in_type}, OutputType(LastType),
+                                     std::move(exec2)));
   }
 
   // Add add(duration, duration) -> duration
@@ -2674,7 +2735,8 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
         add_checked->AddKernel({in_type, in_type}, duration(unit), std::move(exec)));
   }
 
-  AddArithmeticFunctionTimeDurations<AddTimeDurationChecked>(add_checked);
+  AddArithmeticFunctionTimeDuration<AddTimeDurationChecked>(add_checked);
+  AddArithmeticFunctionDurationTime<AddTimeDurationChecked>(add_checked);
 
   DCHECK_OK(registry->AddFunction(std::move(add_checked)));
 
@@ -2732,7 +2794,7 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
   DCHECK_OK(subtract->AddKernel({in_type_date_64, in_type_date_64},
                                 duration(TimeUnit::MILLI), std::move(exec_date_64)));
 
-  AddArithmeticFunctionTimeDurations<SubtractTimeDuration>(subtract);
+  AddArithmeticFunctionTimeDuration<SubtractTimeDuration>(subtract);
 
   DCHECK_OK(registry->AddFunction(std::move(subtract)));
 
@@ -2798,7 +2860,7 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
         subtract_checked->AddKernel({in_type, in_type}, duration(unit), std::move(exec)));
   }
 
-  AddArithmeticFunctionTimeDurations<SubtractTimeDurationChecked>(subtract_checked);
+  AddArithmeticFunctionTimeDuration<SubtractTimeDurationChecked>(subtract_checked);
 
   DCHECK_OK(registry->AddFunction(std::move(subtract_checked)));
 
diff --git a/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc b/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
index 38b810902ba..8acb7d9c9a6 100644
--- a/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
+++ b/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
@@ -1070,6 +1070,9 @@ TEST_F(ScalarTemporalTest, TestTemporalAddDateAndDuration) {
         ArrayFromJSON(timestamp(TimeUnit::MICRO), times_seconds_precision);
     CheckScalarBinary(op, dates32, durations_us, timestamps_us);
     CheckScalarBinary(op, dates64, durations_us, timestamps_us);
+
+    CheckScalarBinary(op, durations_us, dates32, timestamps_us);
+    CheckScalarBinary(op, durations_us, dates64, timestamps_us);
   }
 }
 
@@ -1154,6 +1157,10 @@ TEST_F(ScalarTemporalTest, TestTemporalAddTimestampAndDuration) {
       CheckScalarBinary(op, ArrayFromJSON(timestamp_unit_ns, times_seconds_precision),
                         ArrayFromJSON(duration_unit_ns, nanoseconds_between),
                         ArrayFromJSON(timestamp_unit_ns, times_seconds_precision2));
+
+      CheckScalarBinary(op, ArrayFromJSON(duration_unit_ns, nanoseconds_between),
+                        ArrayFromJSON(timestamp_unit_ns, times_seconds_precision),
+                        ArrayFromJSON(timestamp_unit_ns, times_seconds_precision2));
     }
 
     auto seconds_1 = ArrayFromJSON(timestamp(TimeUnit::SECOND), R"([1, null])");
@@ -1341,6 +1348,9 @@ TEST_F(ScalarTemporalTest, TestTemporalAddTimeAndDuration) {
     CheckScalarBinary(op, arr_ns,
                       ArrayFromJSON(duration(TimeUnit::NANO), nanoseconds_between_time),
                       arr_ns2);
+    CheckScalarBinary(op,
+                      ArrayFromJSON(duration(TimeUnit::NANO), nanoseconds_between_time),
+                      arr_ns, arr_ns2);
 
     auto seconds_1 = ArrayFromJSON(time32(TimeUnit::SECOND), R"([1, null])");
     auto milliseconds_2k = ArrayFromJSON(duration(TimeUnit::MILLI), R"([2000, null])");
@@ -1353,6 +1363,10 @@ TEST_F(ScalarTemporalTest, TestTemporalAddTimeAndDuration) {
     CheckScalarBinary(op, nanoseconds_1G, microseconds_2M, nanoseconds_3M);
     CheckScalarBinary(op, seconds_1, microseconds_2M, microseconds_3M);
 
+    CheckScalarBinary(op, milliseconds_2k, seconds_1, milliseconds_3k);
+    CheckScalarBinary(op, microseconds_2M, nanoseconds_1G, nanoseconds_3M);
+    CheckScalarBinary(op, microseconds_2M, seconds_1, microseconds_3M);
+
     EXPECT_RAISES_WITH_MESSAGE_THAT(
         Invalid,
         ::testing::HasSubstr("-1 is not within the acceptable range of [0, 86400)"),

From 2c13914f7e48a45520a66974a83fa3fff9f5a0d2 Mon Sep 17 00:00:00 2001
From: Rok <rok@mihevc.org>
Date: Wed, 16 Mar 2022 23:39:55 +0100
Subject: [PATCH 2/4] Review feedback

---
 .../compute/kernels/scalar_arithmetic.cc      | 62 ++-----------------
 1 file changed, 6 insertions(+), 56 deletions(-)

diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
index 8b8b0046f4a..bc6b82b3e5b 100644
--- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
+++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
@@ -243,31 +243,9 @@ struct SubtractCheckedDate32 {
 template <bool left_is_32bit, int64_t multiple>
 struct AddTimeDuration {
   template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<(!std::is_same<Arg0, Arg1>::value && left_is_32bit), T> Call(
-      KernelContext*, Arg0 left, Arg1 right, Status* st) {
-    T result = arrow::internal::SafeSignedAdd(left, static_cast<T>(right));
-    if (result < 0 || multiple <= result) {
-      *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
-                            multiple, ") s");
-    }
-    return result;
-  }
-
-  template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<(!std::is_same<Arg0, Arg1>::value && !left_is_32bit), T> Call(
-      KernelContext*, Arg0 left, Arg1 right, Status* st) {
-    T result = arrow::internal::SafeSignedAdd(static_cast<T>(left), right);
-    if (result < 0 || multiple <= result) {
-      *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
-                            multiple, ") s");
-    }
-    return result;
-  }
-
-  template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<(std::is_same<Arg0, Arg1>::value), T> Call(KernelContext*, Arg0 left,
-                                                                Arg1 right, Status* st) {
-    T result = arrow::internal::SafeSignedAdd(left, right);
+  static T Call(KernelContext*, Arg0 left, Arg1 right, Status* st) {
+    T result =
+        arrow::internal::SafeSignedAdd(static_cast<T>(left), static_cast<T>(right));
     if (result < 0 || multiple <= result) {
       *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
                             multiple, ") s");
@@ -279,38 +257,10 @@ struct AddTimeDuration {
 template <bool left_is_32bit, int64_t multiple>
 struct AddTimeDurationChecked {
   template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<(!std::is_same<Arg0, Arg1>::value && left_is_32bit), T> Call(
-      KernelContext*, Arg0 left, Arg1 right, Status* st) {
-    T result = 0;
-    if (ARROW_PREDICT_FALSE(AddWithOverflow(left, static_cast<T>(right), &result))) {
-      *st = Status::Invalid("overflow");
-    }
-    if (result < 0 || multiple <= result) {
-      *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
-                            multiple, ") s");
-    }
-    return result;
-  }
-
-  template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<(!std::is_same<Arg0, Arg1>::value && !left_is_32bit), T> Call(
-      KernelContext*, Arg0 left, Arg1 right, Status* st) {
-    T result = 0;
-    if (ARROW_PREDICT_FALSE(AddWithOverflow(static_cast<T>(left), right, &result))) {
-      *st = Status::Invalid("overflow");
-    }
-    if (result < 0 || multiple <= result) {
-      *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
-                            multiple, ") s");
-    }
-    return result;
-  }
-
-  template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<(std::is_same<Arg0, Arg1>::value), T> Call(KernelContext*, Arg0 left,
-                                                                Arg1 right, Status* st) {
+  static T Call(KernelContext*, Arg0 left, Arg1 right, Status* st) {
     T result = 0;
-    if (ARROW_PREDICT_FALSE(AddWithOverflow(left, right, &result))) {
+    if (ARROW_PREDICT_FALSE(
+            AddWithOverflow(static_cast<T>(left), static_cast<T>(right), &result))) {
       *st = Status::Invalid("overflow");
     }
     if (result < 0 || multiple <= result) {

From 0d424da6e70dd4581967c249d8ad8ddf6593c028 Mon Sep 17 00:00:00 2001
From: Rok <rok@mihevc.org>
Date: Thu, 17 Mar 2022 12:19:22 +0100
Subject: [PATCH 3/4] Review feedback

---
 .../compute/kernels/scalar_arithmetic.cc      | 68 +++++--------------
 1 file changed, 18 insertions(+), 50 deletions(-)

diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
index bc6b82b3e5b..38486bc59e0 100644
--- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
+++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
@@ -240,7 +240,7 @@ struct SubtractCheckedDate32 {
   }
 };
 
-template <bool left_is_32bit, int64_t multiple>
+template <int64_t multiple>
 struct AddTimeDuration {
   template <typename T, typename Arg0, typename Arg1>
   static T Call(KernelContext*, Arg0 left, Arg1 right, Status* st) {
@@ -254,7 +254,7 @@ struct AddTimeDuration {
   }
 };
 
-template <bool left_is_32bit, int64_t multiple>
+template <int64_t multiple>
 struct AddTimeDurationChecked {
   template <typename T, typename Arg0, typename Arg1>
   static T Call(KernelContext*, Arg0 left, Arg1 right, Status* st) {
@@ -271,11 +271,10 @@ struct AddTimeDurationChecked {
   }
 };
 
-template <bool is_32bit, int64_t multiple>
+template <int64_t multiple>
 struct SubtractTimeDuration {
   template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
-                                       Status* st) {
+  static T Call(KernelContext*, Arg0 left, Arg1 right, Status* st) {
     T result = arrow::internal::SafeSignedSubtract(left, static_cast<T>(right));
     if (result < 0 || multiple <= result) {
       *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
@@ -283,24 +282,12 @@ struct SubtractTimeDuration {
     }
     return result;
   }
-
-  template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<!is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
-                                        Status* st) {
-    T result = arrow::internal::SafeSignedSubtract(left, right);
-    if (result < 0 || multiple <= result) {
-      *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
-                            multiple, ") s");
-    }
-    return result;
-  }
 };
 
-template <bool is_32bit, int64_t multiple>
+template <int64_t multiple>
 struct SubtractTimeDurationChecked {
   template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
-                                       Status* st) {
+  static T Call(KernelContext*, Arg0 left, Arg1 right, Status* st) {
     T result = 0;
     if (ARROW_PREDICT_FALSE(SubtractWithOverflow(left, static_cast<T>(right), &result))) {
       *st = Status::Invalid("overflow");
@@ -311,20 +298,6 @@ struct SubtractTimeDurationChecked {
     }
     return result;
   }
-
-  template <typename T, typename Arg0, typename Arg1>
-  static enable_if_t<!is_32bit, T> Call(KernelContext*, Arg0 left, Arg1 right,
-                                        Status* st) {
-    T result = 0;
-    if (ARROW_PREDICT_FALSE(SubtractWithOverflow(left, right, &result))) {
-      *st = Status::Invalid("overflow");
-    }
-    if (result < 0 || multiple <= result) {
-      *st = Status::Invalid(result, " is not within the acceptable range of ", "[0, ",
-                            multiple, ") s");
-    }
-    return result;
-  }
 };
 
 struct Multiply {
@@ -2244,59 +2217,54 @@ std::shared_ptr<ScalarFunction> MakeArithmeticFunctionFloatingPointNotNull(
   return func;
 }
 
-template <template <bool, int64_t> class Op>
+template <template <int64_t> class Op>
 void AddArithmeticFunctionTimeDuration(std::shared_ptr<ScalarFunction> func) {
   // Add Op(time32, duration) -> time32
   TimeUnit::type unit = TimeUnit::SECOND;
-  auto exec_1 = ScalarBinary<Time32Type, Time32Type, DurationType, Op<true, 86400>>::Exec;
+  auto exec_1 = ScalarBinary<Time32Type, Time32Type, DurationType, Op<86400>>::Exec;
   DCHECK_OK(func->AddKernel({time32(unit), duration(unit)}, OutputType(FirstType),
                             std::move(exec_1)));
 
   unit = TimeUnit::MILLI;
-  auto exec_2 =
-      ScalarBinary<Time32Type, Time32Type, DurationType, Op<true, 86400000>>::Exec;
+  auto exec_2 = ScalarBinary<Time32Type, Time32Type, DurationType, Op<86400000>>::Exec;
   DCHECK_OK(func->AddKernel({time32(unit), duration(unit)}, OutputType(FirstType),
                             std::move(exec_2)));
 
   // Add Op(time64, duration) -> time64
   unit = TimeUnit::MICRO;
-  auto exec_3 =
-      ScalarBinary<Time64Type, Time64Type, DurationType, Op<false, 86400000000>>::Exec;
+  auto exec_3 = ScalarBinary<Time64Type, Time64Type, DurationType, Op<86400000000>>::Exec;
   DCHECK_OK(func->AddKernel({time64(unit), duration(unit)}, OutputType(FirstType),
                             std::move(exec_3)));
 
   unit = TimeUnit::NANO;
   auto exec_4 =
-      ScalarBinary<Time64Type, Time64Type, DurationType, Op<false, 86400000000000>>::Exec;
+      ScalarBinary<Time64Type, Time64Type, DurationType, Op<86400000000000>>::Exec;
   DCHECK_OK(func->AddKernel({time64(unit), duration(unit)}, OutputType(FirstType),
                             std::move(exec_4)));
 }
 
-template <template <bool, int64_t> class Op>
+template <template <int64_t> class Op>
 void AddArithmeticFunctionDurationTime(std::shared_ptr<ScalarFunction> func) {
-  // Add Op(time32, duration) -> time32
+  // Add Op(duration, time32) -> time32
   TimeUnit::type unit = TimeUnit::SECOND;
-  auto exec_1 =
-      ScalarBinary<Time32Type, DurationType, Time32Type, Op<false, 86400>>::Exec;
+  auto exec_1 = ScalarBinary<Time32Type, DurationType, Time32Type, Op<86400>>::Exec;
   DCHECK_OK(func->AddKernel({duration(unit), time32(unit)}, OutputType(LastType),
                             std::move(exec_1)));
 
   unit = TimeUnit::MILLI;
-  auto exec_2 =
-      ScalarBinary<Time32Type, DurationType, Time32Type, Op<false, 86400000>>::Exec;
+  auto exec_2 = ScalarBinary<Time32Type, DurationType, Time32Type, Op<86400000>>::Exec;
   DCHECK_OK(func->AddKernel({duration(unit), time32(unit)}, OutputType(LastType),
                             std::move(exec_2)));
 
-  // Add Op(time64, duration) -> time64
+  // Add Op(duration, time64) -> time64
   unit = TimeUnit::MICRO;
-  auto exec_3 =
-      ScalarBinary<Time64Type, DurationType, Time64Type, Op<false, 86400000000>>::Exec;
+  auto exec_3 = ScalarBinary<Time64Type, DurationType, Time64Type, Op<86400000000>>::Exec;
   DCHECK_OK(func->AddKernel({duration(unit), time64(unit)}, OutputType(LastType),
                             std::move(exec_3)));
 
   unit = TimeUnit::NANO;
   auto exec_4 =
-      ScalarBinary<Time64Type, DurationType, Time64Type, Op<false, 86400000000000>>::Exec;
+      ScalarBinary<Time64Type, DurationType, Time64Type, Op<86400000000000>>::Exec;
   DCHECK_OK(func->AddKernel({duration(unit), time64(unit)}, OutputType(LastType),
                             std::move(exec_4)));
 }

From acabb81a8a39ba76f7459d9661fdac7621b046f0 Mon Sep 17 00:00:00 2001
From: Rok <rok@mihevc.org>
Date: Thu, 17 Mar 2022 19:13:47 +0100
Subject: [PATCH 4/4] Review feedback

---
 .../compute/kernels/scalar_arithmetic.cc      | 41 ++++-----
 .../compute/kernels/scalar_temporal_test.cc   | 92 +++++++++----------
 cpp/src/arrow/compute/kernels/test_util.cc    |  7 ++
 cpp/src/arrow/compute/kernels/test_util.h     |  4 +
 4 files changed, 68 insertions(+), 76 deletions(-)

diff --git a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
index 38486bc59e0..bfafb6fcc19 100644
--- a/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
+++ b/cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
@@ -2609,12 +2609,9 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
   // Add add(timestamp, duration) -> timestamp
   for (auto unit : TimeUnit::values()) {
     InputType in_type(match::TimestampTypeUnit(unit));
-    auto exec1 = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
-    DCHECK_OK(add->AddKernel({in_type, duration(unit)}, OutputType(FirstType),
-                             std::move(exec1)));
-    auto exec2 = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
-    DCHECK_OK(add->AddKernel({duration(unit), in_type}, OutputType(LastType),
-                             std::move(exec2)));
+    auto exec = ScalarBinary<Int64Type, Int64Type, Int64Type, Add>::Exec;
+    DCHECK_OK(add->AddKernel({in_type, duration(unit)}, OutputType(FirstType), exec));
+    DCHECK_OK(add->AddKernel({duration(unit), in_type}, OutputType(LastType), exec));
   }
 
   // Add add(duration, duration) -> duration
@@ -2637,12 +2634,11 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
   // Add add_checked(timestamp, duration) -> timestamp
   for (auto unit : TimeUnit::values()) {
     InputType in_type(match::TimestampTypeUnit(unit));
-    auto exec1 = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
-    DCHECK_OK(add_checked->AddKernel({in_type, duration(unit)}, OutputType(FirstType),
-                                     std::move(exec1)));
-    auto exec2 = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
-    DCHECK_OK(add_checked->AddKernel({duration(unit), in_type}, OutputType(LastType),
-                                     std::move(exec2)));
+    auto exec = ScalarBinary<Int64Type, Int64Type, Int64Type, AddChecked>::Exec;
+    DCHECK_OK(
+        add_checked->AddKernel({in_type, duration(unit)}, OutputType(FirstType), exec));
+    DCHECK_OK(
+        add_checked->AddKernel({duration(unit), in_type}, OutputType(LastType), exec));
   }
 
   // Add add(duration, duration) -> duration
@@ -2788,12 +2784,9 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
 
   // Add multiply(duration, int64) -> duration
   for (auto unit : TimeUnit::values()) {
-    auto exec1 = ArithmeticExecFromOp<ScalarBinaryEqualTypes, Multiply>(Type::DURATION);
-    DCHECK_OK(
-        multiply->AddKernel({duration(unit), int64()}, duration(unit), std::move(exec1)));
-    auto exec2 = ArithmeticExecFromOp<ScalarBinaryEqualTypes, Multiply>(Type::DURATION);
-    DCHECK_OK(
-        multiply->AddKernel({int64(), duration(unit)}, duration(unit), std::move(exec2)));
+    auto exec = ArithmeticExecFromOp<ScalarBinaryEqualTypes, Multiply>(Type::DURATION);
+    DCHECK_OK(multiply->AddKernel({duration(unit), int64()}, duration(unit), exec));
+    DCHECK_OK(multiply->AddKernel({int64(), duration(unit)}, duration(unit), exec));
   }
 
   DCHECK_OK(registry->AddFunction(std::move(multiply)));
@@ -2805,14 +2798,12 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
 
   // Add multiply_checked(duration, int64) -> duration
   for (auto unit : TimeUnit::values()) {
-    auto exec1 =
-        ArithmeticExecFromOp<ScalarBinaryEqualTypes, MultiplyChecked>(Type::DURATION);
-    DCHECK_OK(multiply_checked->AddKernel({duration(unit), int64()}, duration(unit),
-                                          std::move(exec1)));
-    auto exec2 =
+    auto exec =
         ArithmeticExecFromOp<ScalarBinaryEqualTypes, MultiplyChecked>(Type::DURATION);
-    DCHECK_OK(multiply_checked->AddKernel({int64(), duration(unit)}, duration(unit),
-                                          std::move(exec2)));
+    DCHECK_OK(
+        multiply_checked->AddKernel({duration(unit), int64()}, duration(unit), exec));
+    DCHECK_OK(
+        multiply_checked->AddKernel({int64(), duration(unit)}, duration(unit), exec));
   }
 
   DCHECK_OK(registry->AddFunction(std::move(multiply_checked)));
diff --git a/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc b/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
index 8acb7d9c9a6..5ed7c911ca8 100644
--- a/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
+++ b/cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
@@ -1061,18 +1061,15 @@ TEST_F(ScalarTemporalTest, TestTemporalAddDateAndDuration) {
         ArrayFromJSON(duration(TimeUnit::MILLI), milliseconds_between_date_and_time);
     auto timestamps_ms =
         ArrayFromJSON(timestamp(TimeUnit::MILLI), times_seconds_precision);
-    CheckScalarBinary(op, dates32, durations_ms, timestamps_ms);
-    CheckScalarBinary(op, dates64, durations_ms, timestamps_ms);
+    CheckScalarBinaryCommutative(op, dates32, durations_ms, timestamps_ms);
+    CheckScalarBinaryCommutative(op, dates64, durations_ms, timestamps_ms);
 
     auto durations_us =
         ArrayFromJSON(duration(TimeUnit::MICRO), microseconds_between_date_and_time);
     auto timestamps_us =
         ArrayFromJSON(timestamp(TimeUnit::MICRO), times_seconds_precision);
-    CheckScalarBinary(op, dates32, durations_us, timestamps_us);
-    CheckScalarBinary(op, dates64, durations_us, timestamps_us);
-
-    CheckScalarBinary(op, durations_us, dates32, timestamps_us);
-    CheckScalarBinary(op, durations_us, dates64, timestamps_us);
+    CheckScalarBinaryCommutative(op, dates32, durations_us, timestamps_us);
+    CheckScalarBinaryCommutative(op, dates64, durations_us, timestamps_us);
   }
 }
 
@@ -1145,33 +1142,33 @@ TEST_F(ScalarTemporalTest, TestTemporalAddTimestampAndDuration) {
       auto timestamp_unit_ns = timestamp(TimeUnit::NANO, tz);
       auto duration_unit_ns = duration(TimeUnit::NANO);
 
-      CheckScalarBinary(op, ArrayFromJSON(timestamp_unit_s, times_seconds_precision),
-                        ArrayFromJSON(duration_unit_s, seconds_between),
-                        ArrayFromJSON(timestamp_unit_s, times_seconds_precision2));
-      CheckScalarBinary(op, ArrayFromJSON(timestamp_unit_ms, times_seconds_precision),
-                        ArrayFromJSON(duration_unit_ms, milliseconds_between),
-                        ArrayFromJSON(timestamp_unit_ms, times_seconds_precision2));
-      CheckScalarBinary(op, ArrayFromJSON(timestamp_unit_us, times_seconds_precision),
-                        ArrayFromJSON(duration_unit_us, microseconds_between),
-                        ArrayFromJSON(timestamp_unit_us, times_seconds_precision2));
-      CheckScalarBinary(op, ArrayFromJSON(timestamp_unit_ns, times_seconds_precision),
-                        ArrayFromJSON(duration_unit_ns, nanoseconds_between),
-                        ArrayFromJSON(timestamp_unit_ns, times_seconds_precision2));
-
-      CheckScalarBinary(op, ArrayFromJSON(duration_unit_ns, nanoseconds_between),
-                        ArrayFromJSON(timestamp_unit_ns, times_seconds_precision),
-                        ArrayFromJSON(timestamp_unit_ns, times_seconds_precision2));
+      CheckScalarBinaryCommutative(
+          op, ArrayFromJSON(timestamp_unit_s, times_seconds_precision),
+          ArrayFromJSON(duration_unit_s, seconds_between),
+          ArrayFromJSON(timestamp_unit_s, times_seconds_precision2));
+      CheckScalarBinaryCommutative(
+          op, ArrayFromJSON(timestamp_unit_ms, times_seconds_precision),
+          ArrayFromJSON(duration_unit_ms, milliseconds_between),
+          ArrayFromJSON(timestamp_unit_ms, times_seconds_precision2));
+      CheckScalarBinaryCommutative(
+          op, ArrayFromJSON(timestamp_unit_us, times_seconds_precision),
+          ArrayFromJSON(duration_unit_us, microseconds_between),
+          ArrayFromJSON(timestamp_unit_us, times_seconds_precision2));
+      CheckScalarBinaryCommutative(
+          op, ArrayFromJSON(timestamp_unit_ns, times_seconds_precision),
+          ArrayFromJSON(duration_unit_ns, nanoseconds_between),
+          ArrayFromJSON(timestamp_unit_ns, times_seconds_precision2));
     }
 
     auto seconds_1 = ArrayFromJSON(timestamp(TimeUnit::SECOND), R"([1, null])");
     auto milliseconds_2k = ArrayFromJSON(duration(TimeUnit::MILLI), R"([2000, null])");
     auto milliseconds_3k = ArrayFromJSON(timestamp(TimeUnit::MILLI), R"([3000, null])");
-    CheckScalarBinary(op, seconds_1, milliseconds_2k, milliseconds_3k);
+    CheckScalarBinaryCommutative(op, seconds_1, milliseconds_2k, milliseconds_3k);
 
     auto seconds_1_tz = ArrayFromJSON(timestamp(TimeUnit::SECOND, "UTC"), R"([1, null])");
     auto milliseconds_3k_tz =
         ArrayFromJSON(timestamp(TimeUnit::MILLI, "UTC"), R"([3000, null])");
-    CheckScalarBinary(op, seconds_1_tz, milliseconds_2k, milliseconds_3k_tz);
+    CheckScalarBinaryCommutative(op, seconds_1_tz, milliseconds_2k, milliseconds_3k_tz);
   }
 }
 
@@ -1336,21 +1333,18 @@ TEST_F(ScalarTemporalTest, TestTemporalAddTimeAndDuration) {
     auto arr_ns = ArrayFromJSON(time64(TimeUnit::NANO), times_ns);
     auto arr_ns2 = ArrayFromJSON(time64(TimeUnit::NANO), times_ns2);
 
-    CheckScalarBinary(op, arr_s,
-                      ArrayFromJSON(duration(TimeUnit::SECOND), seconds_between_time),
-                      arr_s2);
-    CheckScalarBinary(op, arr_ms,
-                      ArrayFromJSON(duration(TimeUnit::MILLI), milliseconds_between_time),
-                      arr_ms2);
-    CheckScalarBinary(op, arr_us,
-                      ArrayFromJSON(duration(TimeUnit::MICRO), microseconds_between_time),
-                      arr_us2);
-    CheckScalarBinary(op, arr_ns,
-                      ArrayFromJSON(duration(TimeUnit::NANO), nanoseconds_between_time),
-                      arr_ns2);
-    CheckScalarBinary(op,
-                      ArrayFromJSON(duration(TimeUnit::NANO), nanoseconds_between_time),
-                      arr_ns, arr_ns2);
+    CheckScalarBinaryCommutative(
+        op, arr_s, ArrayFromJSON(duration(TimeUnit::SECOND), seconds_between_time),
+        arr_s2);
+    CheckScalarBinaryCommutative(
+        op, arr_ms, ArrayFromJSON(duration(TimeUnit::MILLI), milliseconds_between_time),
+        arr_ms2);
+    CheckScalarBinaryCommutative(
+        op, arr_us, ArrayFromJSON(duration(TimeUnit::MICRO), microseconds_between_time),
+        arr_us2);
+    CheckScalarBinaryCommutative(
+        op, arr_ns, ArrayFromJSON(duration(TimeUnit::NANO), nanoseconds_between_time),
+        arr_ns2);
 
     auto seconds_1 = ArrayFromJSON(time32(TimeUnit::SECOND), R"([1, null])");
     auto milliseconds_2k = ArrayFromJSON(duration(TimeUnit::MILLI), R"([2000, null])");
@@ -1359,13 +1353,9 @@ TEST_F(ScalarTemporalTest, TestTemporalAddTimeAndDuration) {
     auto microseconds_2M = ArrayFromJSON(duration(TimeUnit::MICRO), R"([2000000, null])");
     auto nanoseconds_3M = ArrayFromJSON(time64(TimeUnit::NANO), R"([3000000000, null])");
     auto microseconds_3M = ArrayFromJSON(time64(TimeUnit::MICRO), R"([3000000, null])");
-    CheckScalarBinary(op, seconds_1, milliseconds_2k, milliseconds_3k);
-    CheckScalarBinary(op, nanoseconds_1G, microseconds_2M, nanoseconds_3M);
-    CheckScalarBinary(op, seconds_1, microseconds_2M, microseconds_3M);
-
-    CheckScalarBinary(op, milliseconds_2k, seconds_1, milliseconds_3k);
-    CheckScalarBinary(op, microseconds_2M, nanoseconds_1G, nanoseconds_3M);
-    CheckScalarBinary(op, microseconds_2M, seconds_1, microseconds_3M);
+    CheckScalarBinaryCommutative(op, seconds_1, milliseconds_2k, milliseconds_3k);
+    CheckScalarBinaryCommutative(op, nanoseconds_1G, microseconds_2M, nanoseconds_3M);
+    CheckScalarBinaryCommutative(op, seconds_1, microseconds_2M, microseconds_3M);
 
     EXPECT_RAISES_WITH_MESSAGE_THAT(
         Invalid,
@@ -1486,10 +1476,10 @@ TEST_F(ScalarTemporalTest, TestTemporalMultiplyDuration) {
     auto multipliers = ArrayFromJSON(int64(), R"([0, 3, 2, 7, null])");
     auto durations_multiplied = ArrayFromJSON(unit, R"([0, -3, 4, 42, null])");
 
-    CheckScalarBinary("multiply", durations, multipliers, durations_multiplied);
-    CheckScalarBinary("multiply", multipliers, durations, durations_multiplied);
-    CheckScalarBinary("multiply_checked", durations, multipliers, durations_multiplied);
-    CheckScalarBinary("multiply_checked", multipliers, durations, durations_multiplied);
+    CheckScalarBinaryCommutative("multiply", durations, multipliers,
+                                 durations_multiplied);
+    CheckScalarBinaryCommutative("multiply_checked", durations, multipliers,
+                                 durations_multiplied);
 
     EXPECT_RAISES_WITH_MESSAGE_THAT(
         Invalid, ::testing::HasSubstr("Invalid: overflow"),
diff --git a/cpp/src/arrow/compute/kernels/test_util.cc b/cpp/src/arrow/compute/kernels/test_util.cc
index 1c6a51d09b7..169d8d6935e 100644
--- a/cpp/src/arrow/compute/kernels/test_util.cc
+++ b/cpp/src/arrow/compute/kernels/test_util.cc
@@ -273,6 +273,13 @@ void CheckScalarBinary(std::string func_name, Datum left_input, Datum right_inpu
   CheckScalar(std::move(func_name), {left_input, right_input}, expected, options);
 }
 
+void CheckScalarBinaryCommutative(std::string func_name, Datum left_input,
+                                  Datum right_input, Datum expected,
+                                  const FunctionOptions* options) {
+  CheckScalar(func_name, {left_input, right_input}, expected, options);
+  CheckScalar(func_name, {right_input, left_input}, expected, options);
+}
+
 namespace {
 
 void ValidateOutput(const ArrayData& output) {
diff --git a/cpp/src/arrow/compute/kernels/test_util.h b/cpp/src/arrow/compute/kernels/test_util.h
index c2d5a8f12e8..81c468958c0 100644
--- a/cpp/src/arrow/compute/kernels/test_util.h
+++ b/cpp/src/arrow/compute/kernels/test_util.h
@@ -124,6 +124,10 @@ void CheckScalarUnary(std::string func_name, Datum input, Datum expected,
 void CheckScalarBinary(std::string func_name, Datum left_input, Datum right_input,
                        Datum expected, const FunctionOptions* options = nullptr);
 
+void CheckScalarBinaryCommutative(std::string func_name, Datum left_input,
+                                  Datum right_input, Datum expected,
+                                  const FunctionOptions* options = nullptr);
+
 void CheckVectorUnary(std::string func_name, Datum input, Datum expected,
                       const FunctionOptions* options = nullptr);