[GitHub] [arrow] bkietz commented on a diff in pull request #14043: ARROW-17613: [C++] Add function execution API for a preconfigured kernel

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

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


##########
cpp/src/arrow/compute/function.cc:
##########
@@ -167,6 +180,109 @@ const Kernel* DispatchExactImpl(const Function* func,
   return nullptr;
 }
 
+struct FunctionExecutorImpl : public FunctionExecutor {
+  FunctionExecutorImpl(std::vector<TypeHolder> in_types, const Kernel* kernel,
+                       std::unique_ptr<detail::KernelExecutor> executor,
+                       const Function& func)
+      : in_types(std::move(in_types)),
+        kernel(kernel),
+        kernel_ctx(),
+        executor(std::move(executor)),
+        func(func),
+        state(),
+        options(NULLPTR) {}
+  virtual ~FunctionExecutorImpl() {}
+
+  Status KernelInit(const FunctionOptions* options) {
+    if (!kernel_ctx) {
+      return Status::Invalid("function executor with null kernel context");
+    }
+    if (options == nullptr) {
+      RETURN_NOT_OK(CheckOptions(func, options));
+      options = func.default_options();
+    }

Review Comment:
   Could we fold this into 
   ```suggestion
       RETURN_NOT_OK(CheckOptions(func, &options));
   ```



##########
cpp/src/arrow/compute/function.cc:
##########
@@ -167,6 +180,109 @@ const Kernel* DispatchExactImpl(const Function* func,
   return nullptr;
 }
 
+struct FunctionExecutorImpl : public FunctionExecutor {
+  FunctionExecutorImpl(std::vector<TypeHolder> in_types, const Kernel* kernel,
+                       std::unique_ptr<detail::KernelExecutor> executor,
+                       const Function& func)
+      : in_types(std::move(in_types)),
+        kernel(kernel),
+        kernel_ctx(),
+        executor(std::move(executor)),
+        func(func),
+        state(),
+        options(NULLPTR) {}
+  virtual ~FunctionExecutorImpl() {}
+
+  Status KernelInit(const FunctionOptions* options) {
+    if (!kernel_ctx) {

Review Comment:
   KernelContext is a simple wrapper around an ExecContext, KernelState, and a Kernel. I think it's not worthwhile to keep it by pointer and it'd be preferable validate the members of the KernelContext instead- like checking if KernelContext::exec_context is non null (or coercing to default_exec_context)



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