[GitHub] [arrow] lidavidm commented on a change in pull request #10390: ARROW-12751: [C++] Implement minimum/maximum kernels

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

lidavidm commented on a change in pull request #10390:
URL: https://github.com/apache/arrow/pull/10390#discussion_r638312730



##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -42,6 +42,11 @@ struct ArithmeticOptions : public FunctionOptions {
   bool check_overflow;
 };
 
+struct ARROW_EXPORT MinMaxOptions : public FunctionOptions {

Review comment:
       Do we want to name this something else?

##########
File path: cpp/src/arrow/compute/api_scalar.h
##########
@@ -250,6 +255,24 @@ Result<Datum> Power(const Datum& left, const Datum& right,
                     ArithmeticOptions options = ArithmeticOptions(),
                     ExecContext* ctx = NULLPTR);
 
+/// \brief

Review comment:
       I forgot to fill this in, will fix.

##########
File path: cpp/src/arrow/ipc/json_simple.cc
##########
@@ -911,6 +912,26 @@ Status DictArrayFromJSON(const std::shared_ptr<DataType>& type,
       .Value(out);
 }
 
+Status ScalarFromJSON(const std::shared_ptr<DataType>& type,

Review comment:
       I'm going to port this into ARROW-12859.

##########
File path: cpp/src/arrow/testing/gtest_util.cc
##########
@@ -360,6 +360,30 @@ void AssertDatumsEqual(const Datum& expected, const Datum& actual, bool verbose)
       break;
   }
 }
+ARROW_TESTING_EXPORT void AssertDatumsApproxEqual(const Datum& expected,

Review comment:
       I'm going to port this to ARROW-12859 (and fix the unimplemented case).

##########
File path: cpp/src/arrow/compute/kernels/scalar_arithmetic.cc
##########
@@ -428,6 +454,45 @@ ArrayKernelExec ArithmeticExecFromOp(detail::GetTypeId get_id) {
   }
 }
 
+template <template <typename... Args> class KernelGenerator, typename Op>

Review comment:
       This is rather a lot of code being generated; it might be nice if I could figure out the right template setup to consolidate the temporal types with their respective equivalent integral type.




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

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