[GitHub] [arrow] bkietz commented on a diff in pull request #36231: GH-36128: [C++][Compute] Allow multiplication between duration and all integer types

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #36231:
URL: https://github.com/apache/arrow/pull/36231#discussion_r1238901471


##########
cpp/src/arrow/compute/kernels/scalar_arithmetic.cc:
##########
@@ -640,6 +642,17 @@ struct ArithmeticFunction : ScalarFunction {
           ReplaceTypes(type, types);
         }
       }
+
+      if (name_ == "multiply_checked") {
+        // MultiplyChecked requires the same type for both operands, therefore we can't
+        // easily make kernels for integers other than int64, so we just cast integers to
+        // int64
+        if ((*types)[0].id() == Type::DURATION && is_integer((*types)[1].id())) {
+          ReplaceTypes(int64(), &(*types)[1], 1);
+        } else if ((*types)[1].id() == Type::DURATION && is_integer((*types)[0].id())) {
+          ReplaceTypes(int64(), &(*types)[0], 1);
+        }
+      }

Review Comment:
   Personally, I'd prefer to see a uniform approach to adding this support to `multiply` and `multiply_checked`: either both should have kernels matching each type or both should just use the numeric promotion. (Adding kernels to multiply_checked would require adding Call overloads to the MultiplyChecked op, but is doable.) Of those two options, I'd prefer to see promotion for consistency; multiply(int8, int64) requires an implicit cast to int64 so it seems odd to specifically add direct support for multiply(int8, duration) but not for any of the other arithmetic kernels.
   
   More directly in answer to your question, I would also like to see this logic extracted to a helper function. It may only be used in this DispatchBest, but just in case another Function requires this logic it'd be best to try to avoid copypasta. Furthermore it'll keep this DispatchBest from getting too unreadable. Something like:
   
   ```c++
   if (name_ == "multiply" || name_ == "multiply_checked" ||
       name_ == "divide" || name_ == "divide_checked") {
     PromoteIntegerForDurationArithmetic(types);
   }
   
   void PromoteIntegerForDurationArithmetic(std::vector<TypeHolder>* types) {
     bool has_duration = false;
     for (const auto& type : *types) {
       if (type.id() == Type::DURATION) {
         has_duration = true;
       }
     }
     if (!has_duration) return;
   
     // Require implicit casts to int64 to match duration's bit width
     for (auto& type : *types) {
       if (is_integer(type.id())) {
         type = int64();
       }
     }
   }
   ```
   
   It should be fine to let this function do the replacement `(int8, duration) -> (int64, duration)` even for divide; this just defers the failed kernel lookup to the following DispatchExact call.



-- 
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