niyue commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1434711551
##########
cpp/src/gandiva/engine.h:
##########
@@ -30,23 +35,34 @@
#include "gandiva/llvm_types.h"
#include "gandiva/visibility.h"
+namespace llvm::orc {
+class LLJIT;
+} // namespace llvm::orc
+
namespace gandiva {
/// \brief LLVM Execution engine wrapper.
class GANDIVA_EXPORT Engine {
public:
+ ~Engine();
llvm::LLVMContext* context() { return context_.get(); }
llvm::IRBuilder<>* ir_builder() { return ir_builder_.get(); }
LLVMTypes* types() { return &types_; }
- llvm::Module* module() { return module_; }
+
+ /// Retrieve LLVM module in the engine.
+ /// This should only be called before `FinalizeModule` is called
+ llvm::Module* module();
/// Factory method to create and initialize the engine object.
///
/// \param[in] config the engine configuration
/// \param[in] cached flag to mark if the module is already compiled and
cached
- /// \param[out] engine the created engine
- static Status Make(const std::shared_ptr<Configuration>& config, bool cached,
- std::unique_ptr<Engine>* engine);
+ /// \param[in] object_cache an optional object_cache used for building the
module
+ /// \return arrow::Result containing the created engine
+ static Result<std::unique_ptr<Engine>> Make(
Review Comment:
I commented here about this change
(https://github.com/apache/arrow/pull/39098#discussion_r1417035796)
LLJIT requires cache related parameter to be passed during JIT instance
construction (you can find the details here,
https://github.com/apache/arrow/pull/39098/files/b484dfbc293de5d2bca5cf4d294bc5765ff51194#diff-0fbbced7a02cae757d015b7dec201c949bab76bab3c97d12d0a4bdd9712b4e70R212-R224)
So we have to change the `LLVMGenerator`/`Engine` constructor to add the
`GandivaCache` as a parameter in order to use LLJIT, because previously there
is an `out` parameter in their constructors, it is not practical to add an
optional parameter as the last parameter in the constructor, which means we
have to change the constructor anyway in this PR, so I change it in a more
radical way to take advantage of `arrow::Result`.
I think Gandiva users typically uses `Projector`/`Filter` but do not use
`LLVMGenerator` and `Engine` API directly, so it may be okay to consider this
is an internal API change. What do you think?
--
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]
