Change LLVMGenerator and Engine's Make factory method to return arrow…

… Result instead of using out parameter so that it is simpler and optional parameter can be used as well.
apache · Dec 6, 2023 · f06a8cf · f06a8cf
1 parent 54f530b
commit f06a8cf
Show file tree

Hide file tree

Showing 8 changed files with 24 additions and 35 deletions.
diff --git a/cpp/src/gandiva/engine.cc b/cpp/src/gandiva/engine.cc
@@ -237,10 +237,9 @@ Status Engine::LoadFunctionIRs() {
 }
 
 /// factory method to construct the engine.
-Status Engine::Make(
+Result<std::unique_ptr<Engine>> Engine::Make(
     const std::shared_ptr<Configuration>& conf, bool cached,
-    std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache,
-    std::unique_ptr<Engine>* out) {
+    std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache) {
   std::call_once(llvm_init_once_flag, InitOnce);
 
   ARROW_ASSIGN_OR_RAISE(auto jtmb, GetTargetMachineBuilder(*conf));
@@ -253,8 +252,7 @@ Status Engine::Make(
       new Engine(conf, std::move(jit), std::move(target_machine), cached)};
 
   ARROW_RETURN_NOT_OK(engine->Init());
-  *out = std::move(engine);
-  return Status::OK();
+  return engine;
 }
 
 static arrow::Status VerifyAndLinkModule(

diff --git a/cpp/src/gandiva/engine.h b/cpp/src/gandiva/engine.h
@@ -53,11 +53,10 @@ class GANDIVA_EXPORT Engine {
   /// \param[in] config the engine configuration
   /// \param[in] cached flag to mark if the module is already compiled and cached
   /// \param[in] object_cache an optional object_cache used for building the module
-  /// \param[out] engine the created engine
-  static Status Make(
+  /// \return arrow::Result containing the created engine
+  static Result<std::unique_ptr<Engine>> Make(
       const std::shared_ptr<Configuration>& config, bool cached,
-      std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache,
-      std::unique_ptr<Engine>* engine);
+      std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache = std::nullopt);
 
   /// Add the function to the list of IR functions that need to be compiled.
   /// Compiling only the functions that are used by the module saves time.

diff --git a/cpp/src/gandiva/engine_llvm_test.cc b/cpp/src/gandiva/engine_llvm_test.cc
@@ -100,7 +100,7 @@ class TestEngine : public ::testing::Test {
   }
 
   void BuildEngine() {
-    ASSERT_OK(Engine::Make(TestConfiguration(), false, std::nullopt, &engine));
+    ASSERT_OK_AND_ASSIGN(engine, Engine::Make(TestConfiguration(), false));
   }
 
   std::unique_ptr<Engine> engine;

diff --git a/cpp/src/gandiva/filter.cc b/cpp/src/gandiva/filter.cc
@@ -65,9 +65,8 @@ Status Filter::Make(SchemaPtr schema, ConditionPtr condition,
   GandivaObjectCache obj_cache(cache, cache_key);
 
   // Build LLVM generator, and generate code for the specified expression
-  std::unique_ptr<LLVMGenerator> llvm_gen;
-  ARROW_RETURN_NOT_OK(
-      LLVMGenerator::Make(configuration, is_cached, obj_cache, &llvm_gen));
+  ARROW_ASSIGN_OR_RAISE(auto llvm_gen,
+                        LLVMGenerator::Make(configuration, is_cached, obj_cache));
 
   if (!is_cached) {
     // Run the validation on the expression.

diff --git a/cpp/src/gandiva/llvm_generator.cc b/cpp/src/gandiva/llvm_generator.cc
@@ -42,18 +42,15 @@ LLVMGenerator::LLVMGenerator(bool cached,
       function_registry_(std::move(function_registry)),
       enable_ir_traces_(false) {}
 
-Status LLVMGenerator::Make(
+Result<std::unique_ptr<LLVMGenerator>> LLVMGenerator::Make(
     const std::shared_ptr<Configuration>& config, bool cached,
-    std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache,
-    std::unique_ptr<LLVMGenerator>* llvm_generator) {
-  std::unique_ptr<LLVMGenerator> llvmgen_obj(
+    std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache) {
+  std::unique_ptr<LLVMGenerator> llvm_generator(
       new LLVMGenerator(cached, config->function_registry()));
 
-  ARROW_RETURN_NOT_OK(
-      Engine::Make(config, cached, object_cache, &(llvmgen_obj->engine_)));
-  *llvm_generator = std::move(llvmgen_obj);
-
-  return Status::OK();
+  ARROW_ASSIGN_OR_RAISE(auto engine, Engine::Make(config, cached, object_cache));
+  llvm_generator->engine_ = std::move(engine);
+  return llvm_generator;
 }
 
 std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>

diff --git a/cpp/src/gandiva/llvm_generator.h b/cpp/src/gandiva/llvm_generator.h
@@ -49,10 +49,10 @@ class FunctionHolder;
 class GANDIVA_EXPORT LLVMGenerator {
  public:
   /// \brief Factory method to initialize the generator.
-  static Status Make(
+  static Result<std::unique_ptr<LLVMGenerator>> Make(
       const std::shared_ptr<Configuration>& config, bool cached,
-      std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache,
-      std::unique_ptr<LLVMGenerator>* llvm_generator);
+      std::optional<std::reference_wrapper<GandivaObjectCache>> object_cache =
+          std::nullopt);
 
   /// \brief Get the cache to be used for LLVM ObjectCache.
   static std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>

diff --git a/cpp/src/gandiva/llvm_generator_test.cc b/cpp/src/gandiva/llvm_generator_test.cc
@@ -47,8 +47,7 @@ class TestLLVMGenerator : public ::testing::Test {
     auto external_registry = std::make_shared<FunctionRegistry>();
     auto config = config_factory(std::move(external_registry));
 
-    std::unique_ptr<LLVMGenerator> generator;
-    ASSERT_OK(LLVMGenerator::Make(config, false, std::nullopt, &generator));
+    ASSERT_OK_AND_ASSIGN(auto generator, LLVMGenerator::Make(config, false));
 
     auto module = generator->module();
     ASSERT_OK(generator->engine_->LoadFunctionIRs());
@@ -58,8 +57,7 @@ class TestLLVMGenerator : public ::testing::Test {
 
 // Verify that a valid pc function exists for every function in the registry.
 TEST_F(TestLLVMGenerator, VerifyPCFunctions) {
-  std::unique_ptr<LLVMGenerator> generator;
-  ASSERT_OK(LLVMGenerator::Make(TestConfiguration(), false, std::nullopt, &generator));
+  ASSERT_OK_AND_ASSIGN(auto generator, LLVMGenerator::Make(TestConfiguration(), false));
 
   llvm::Module* module = generator->module();
   ASSERT_OK(generator->engine_->LoadFunctionIRs());
@@ -70,9 +68,8 @@ TEST_F(TestLLVMGenerator, VerifyPCFunctions) {
 
 TEST_F(TestLLVMGenerator, TestAdd) {
   // Setup LLVM generator to do an arithmetic add of two vectors
-  std::unique_ptr<LLVMGenerator> generator;
-  ASSERT_OK(
-      LLVMGenerator::Make(TestConfigWithIrDumping(), false, std::nullopt, &generator));
+  ASSERT_OK_AND_ASSIGN(auto generator,
+                       LLVMGenerator::Make(TestConfigWithIrDumping(), false));
   Annotator annotator;
 
   auto field0 = std::make_shared<arrow::Field>("f0", arrow::int32());

diff --git a/cpp/src/gandiva/projector.cc b/cpp/src/gandiva/projector.cc
@@ -80,9 +80,8 @@ Status Projector::Make(SchemaPtr schema, const ExpressionVector& exprs,
   GandivaObjectCache obj_cache(cache, cache_key);
 
   // Build LLVM generator, and generate code for the specified expressions
-  std::unique_ptr<LLVMGenerator> llvm_gen;
-  ARROW_RETURN_NOT_OK(
-      LLVMGenerator::Make(configuration, is_cached, obj_cache, &llvm_gen));
+  ARROW_ASSIGN_OR_RAISE(auto llvm_gen,
+                        LLVMGenerator::Make(configuration, is_cached, obj_cache));
 
   // Run the validation on the expressions.
   // Return if any of the expression is invalid since