[GitHub] [arrow] pitrou commented on a change in pull request #12215: ARROW-14101: [C++] multiply(duration, integer) -> duration kernel

[GitBox <gi...@apache.org>]

pitrou commented on a change in pull request #12215:
URL: https://github.com/apache/arrow/pull/12215#discussion_r811968318



##########
File path: cpp/src/arrow/compute/kernels/codegen_internal_test.cc
##########
@@ -202,6 +202,8 @@ TEST(TestDispatchBest, CommonTemporalResolution) {
   ASSERT_EQ(TimeUnit::MILLI, CommonTemporalResolution(args.data(), args.size()));
   args = {time64(TimeUnit::MICRO), duration(TimeUnit::NANO)};
   ASSERT_EQ(TimeUnit::NANO, CommonTemporalResolution(args.data(), args.size()));
+  args = {duration(TimeUnit::SECOND), int64()};
+  ASSERT_EQ(TimeUnit::SECOND, CommonTemporalResolution(args.data(), args.size()));

Review comment:
       This looks unexpected to me since `int64` is not a temporal at all. @lidavidm What do you think?

##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
##########
@@ -1455,6 +1455,36 @@ TEST_F(ScalarTemporalTest, TestTemporalSubtractDuration) {
   }
 }
 
+TEST_F(ScalarTemporalTest, TestTemporalMultiplyDuration) {
+  std::shared_ptr<Array> max_array;
+  auto max = std::numeric_limits<int64_t>::max();
+  ArrayFromVector<Int64Type, int64_t>({max, max, max, max}, &max_array);
+
+  for (auto u : TimeUnit::values()) {
+    auto unit = duration(u);
+    auto durations = ArrayFromJSON(unit, R"([0, 1, 6, null])");
+    auto multipliers = ArrayFromJSON(int64(), R"([0, 3, 7, null])");
+    auto durations_multiplied = ArrayFromJSON(unit, R"([0, 3, 42, null])");

Review comment:
       Can you some negative value(s) into the mix?

##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -2802,23 +2802,55 @@ void RegisterScalarArithmetic(FunctionRegistry* registry) {
   // ----------------------------------------------------------------------
   auto multiply = MakeArithmeticFunction<Multiply>("multiply", &mul_doc);
   AddDecimalBinaryKernels<Multiply>("multiply", multiply.get());
+
+  // Add multiply(duration, int64) -> duration
+  for (auto unit : TimeUnit::values()) {
+    auto exec = ArithmeticExecFromOp<ScalarBinaryEqualTypes, Multiply>(Type::DURATION);
+    DCHECK_OK(
+        multiply->AddKernel({duration(unit), int64()}, duration(unit), std::move(exec)));

Review comment:
       Is it worth also supporting `multiply(int64, duration)` or would commutativity be handled at an upper level? @lidavidm 

##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -464,7 +465,6 @@ struct DivideChecked {
   template <typename T, typename Arg0, typename Arg1>
   static enable_if_integer_value<T> Call(KernelContext*, Arg0 left, Arg1 right,
                                          Status* st) {
-    static_assert(std::is_same<T, Arg0>::value && std::is_same<T, Arg1>::value, "");

Review comment:
       For which reason was this failing? Shouldn't it receive `int64_t` for all arguments in this PR?

##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
##########
@@ -1455,6 +1455,36 @@ TEST_F(ScalarTemporalTest, TestTemporalSubtractDuration) {
   }
 }
 
+TEST_F(ScalarTemporalTest, TestTemporalMultiplyDuration) {
+  std::shared_ptr<Array> max_array;
+  auto max = std::numeric_limits<int64_t>::max();
+  ArrayFromVector<Int64Type, int64_t>({max, max, max, max}, &max_array);
+
+  for (auto u : TimeUnit::values()) {
+    auto unit = duration(u);
+    auto durations = ArrayFromJSON(unit, R"([0, 1, 6, null])");
+    auto multipliers = ArrayFromJSON(int64(), R"([0, 3, 7, null])");
+    auto durations_multiplied = ArrayFromJSON(unit, R"([0, 3, 42, null])");
+
+    CheckScalarBinary("multiply", durations, multipliers, durations_multiplied);
+    CheckScalarBinary("multiply_checked", durations, multipliers, durations_multiplied);
+    EXPECT_RAISES_WITH_MESSAGE_THAT(
+        Invalid, ::testing::HasSubstr("Invalid: overflow"),
+        CallFunction("multiply_checked", {durations, max_array}));
+  }
+}
+
+TEST_F(ScalarTemporalTest, TestTemporalDivideDuration) {
+  for (auto u : TimeUnit::values()) {
+    auto unit = duration(u);
+    auto divided_durations = ArrayFromJSON(unit, R"([0, 1, 6, null])");
+    auto divisors = ArrayFromJSON(int64(), R"([3, 3, 7, null])");
+    auto durations = ArrayFromJSON(unit, R"([1, 3, 42, null])");
+    CheckScalarBinary("divide", durations, divisors, divided_durations);
+    CheckScalarBinary("divide_checked", durations, divisors, divided_durations);

Review comment:
       Also check that division by zero raises?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org