kou commented on code in PR #38116:
URL: https://github.com/apache/arrow/pull/38116#discussion_r1354124148


##########
cpp/src/gandiva/engine.cc:
##########
@@ -152,6 +153,7 @@ Status Engine::LoadFunctionIRs() {
   if (!functions_loaded_) {
     ARROW_RETURN_NOT_OK(LoadPreCompiledIR());
     ARROW_RETURN_NOT_OK(DecimalIR::AddFunctions(this));
+    ARROW_RETURN_NOT_OK(LoadExternalPreCompiledIR());

Review Comment:
   > Could you please explain with more details?
   
   Sure! Sorry for missing information...
    
   > Do you mean we add a new API called `AddPreCompiledIR` (do you mean `IR` 
instead of `ID`?) to the `Engine` class so that developers can call this API 
directly to add a third party IR?
   
   Yes. (It's a typo. Sorry...)
   
   > Currently, the `Engine` class is not used by Gandiva users directly, 
typically Gandiva users uses the `Projector` API, which internally calls the 
`LLVMGenerator` which internally calls the `Engine` API. For example:
   > 
   > ```
   > std::shared_ptr<Projector> projector;
   >   ARROW_EXPECT_OK(Projector::Make(schema, {multiply}, TestConfiguration(), 
&projector));
   > ...
   > ARROW_EXPECT_OK(projector->Evaluate(*in_batch, pool_, &outs));
   > ```
   > 
   > So if we add `AddPreCompiledIR` to the `Engine` class, this will be a new 
API surface for users as well.
   > 
   > In this PR, it is expected Gandiva users to use the new `LLVMIRStore` API 
to load third party IR into Gandiva, it is possible we move this API to 
`Engine` class, but since the `Engine` class is not a global singleton, and is 
re-instantiated every time by the `LLVMGenerator`, if we store IR in `Engine`, 
we may have to introduce similar global states in `Engine` like we do for 
`LLVMIRStore`.
   
   How about adding `FunctionRegistry::Add(NativeFunction func, const 
std::string& bitcode_path)` and using it and `Engine::AddPreCompiledIR()` in 
`LLVMGenerator::Add()`?
   
   Then, users can only use `FunctionRegistry::Add()` to add an external 
function instead of using `FunctionRegistry::AddFunction()` and LVMIRStore`.



-- 
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-unsubscr...@arrow.apache.org

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

Reply via email to