pitrou commented on code in PR #38116:
URL: https://github.com/apache/arrow/pull/38116#discussion_r1368687570
##########
cpp/src/gandiva/engine.h:
##########
@@ -93,6 +93,9 @@ class GANDIVA_EXPORT Engine {
/// the main module.
Status LoadPreCompiledIR();
+ // load external pre-compiled IR modules from LLVMIRStore
Review Comment:
`LLVMIRStore` doesn't exist anymore, does it? Can you update this docstring?
##########
cpp/src/gandiva/engine.cc:
##########
@@ -236,7 +238,38 @@ static void SetDataLayout(llvm::Module* module) {
module->setDataLayout(machine->createDataLayout());
}
-// end of the mofified method from MLIR
+// end of the modified method from MLIR
+
+static arrow::Result<std::unique_ptr<llvm::Module>> GetModule(
+ llvm::Expected<std::unique_ptr<llvm::Module>>& module_or_error) {
Review Comment:
We can probably make a more general function to convert a
`llvm::Expected<T>` into a `arrow::Result<T>`, no?
##########
cpp/src/gandiva/expr_decomposer_test.cc:
##########
@@ -32,7 +32,7 @@ using arrow::int32;
class TestExprDecomposer : public ::testing::Test {
protected:
- FunctionRegistry registry_;
+ FunctionRegistry& registry_ = *default_function_registry();
Review Comment:
We prefer either const-refs or non-const pointers, so here:
```suggestion
FunctionRegistry* registry_ = default_function_registry();
```
##########
cpp/src/gandiva/function_registry.h:
##########
@@ -30,18 +35,41 @@ class GANDIVA_EXPORT FunctionRegistry {
public:
using iterator = const NativeFunction*;
+ FunctionRegistry();
+ FunctionRegistry(const FunctionRegistry&) = delete;
+ FunctionRegistry& operator=(const FunctionRegistry&) = delete;
+
/// Lookup a pre-compiled function by its signature.
const NativeFunction* LookupSignature(const FunctionSignature& signature)
const;
+ /// \brief register a set of functions into the function registry from a
given bitcode
+ /// file
+ arrow::Status Register(const std::vector<NativeFunction>& funcs,
+ const std::string& bitcode_path);
+
+ /// \brief register a set of functions into the function registry from a
given bitcode
+ /// buffer
+ arrow::Status Register(const std::vector<NativeFunction>& funcs,
+ std::unique_ptr<arrow::Buffer> bitcode_buffer);
Review Comment:
This would be more flexible if it took a `std::shared_ptr<arrow::Buffer>`
##########
cpp/src/gandiva/expression_registry.h:
##########
@@ -37,7 +38,8 @@ class GANDIVA_EXPORT ExpressionRegistry {
public:
using native_func_iterator_type = const NativeFunction*;
using func_sig_iterator_type = const FunctionSignature*;
- ExpressionRegistry();
+ ExpressionRegistry(
+ FunctionRegistry* function_registry =
gandiva::default_function_registry());
Review Comment:
Perhaps make this explicit:
```suggestion
explicit ExpressionRegistry(
FunctionRegistry* function_registry =
gandiva::default_function_registry());
```
##########
cpp/src/gandiva/engine.cc:
##########
@@ -256,23 +289,26 @@ Status Engine::LoadPreCompiledIR() {
/// Parse the IR module.
llvm::Expected<std::unique_ptr<llvm::Module>> module_or_error =
llvm::getOwningLazyBitcodeModule(std::move(buffer), *context());
- if (!module_or_error) {
- // NOTE: llvm::handleAllErrors() fails linking with RTTI-disabled LLVM
builds
- // (ARROW-5148)
- std::string str;
- llvm::raw_string_ostream stream(str);
- stream << module_or_error.takeError();
- return Status::CodeGenError(stream.str());
- }
- std::unique_ptr<llvm::Module> ir_module = std::move(module_or_error.get());
+ // NOTE: llvm::handleAllErrors() fails linking with RTTI-disabled LLVM builds
+ // (ARROW-5148)
+ ARROW_RETURN_NOT_OK(VerifyAndLinkModule(module_,
std::move(module_or_error)));
+ return Status::OK();
+}
- // set dataLayout
- SetDataLayout(ir_module.get());
+llvm::MemoryBufferRef AsLLVMMemoryBuffer(
+ const std::unique_ptr<arrow::Buffer>& arrow_buffer) {
Review Comment:
Two things:
1) a simple `const Buffer&` seems sufficient here
2) please make sure that all private functions are made `static`, or moved
into the anonymous namespace
##########
cpp/src/gandiva/expression_registry.h:
##########
@@ -62,7 +64,7 @@ class GANDIVA_EXPORT ExpressionRegistry {
private:
static DataTypeVector supported_types_;
- std::unique_ptr<FunctionRegistry> function_registry_;
Review Comment:
Apropos ownership, do we want to carry raw pointers? Using `std::shared_ptr`
may be safer and more future-proof...
##########
cpp/src/gandiva/tests/test_util.h:
##########
@@ -16,6 +16,7 @@
// under the License.
#include <chrono>
+#include <filesystem>
Review Comment:
It does not seem this include is used here, is it?
##########
cpp/src/gandiva/function_registry.h:
##########
@@ -17,7 +17,12 @@
#pragma once
+#include <memory>
+#include <string>
#include <vector>
+
+#include <arrow/buffer.h>
+#include <arrow/status.h>
Review Comment:
Should use regular quotes for Arrow includes:
```suggestion
#include "arrow/buffer.h"
#include "arrow/status.h"
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
