GH-38589: [C++][Gandiva] Support registering external C functions #38632

niyue · 2023-11-08T06:00:42Z

Rationale for this change

This PR tries to enhance Gandiva by supporting registering external C functions to its function registry, so that developers can author third party functions with complex dependency and expose them as C functions to be used in Gandiva expression. See more details in GH-38589.

What changes are included in this PR?

This PR primarily adds a new API to the FunctionRegistry so that developers can use it to register external C functions:

arrow::Status Register(
      NativeFunction func, void* c_function_ptr,
      std::optional<FunctionHolderMaker> function_holder_maker = std::nullopt);

Are these changes tested?

The changes are tested via unit tests in this PR, and the unit tests include several C functions written using C++ and we confirm this kind of functions can be used by Gandiva after registration using the above mentioned new API.
Additionally, locally I wrote some Rust based functions, and integrate the Rust based functions into a C++ program by using the new registration API and verified this approach did work, but this piece of work is not included in the PR.

Are there any user-facing changes?

There are several new APIs added to FunctionRegistry class:

/// \brief register a C function into the function registry
  /// @param func the registered function's metadata
  /// @param c_function_ptr the function pointer to the
  /// registered function's implementation
  /// @param function_holder_maker this will be used as the function holder if the
  /// function requires a function holder
  arrow::Status Register(
      NativeFunction func, void* c_function_ptr,
      std::optional<FunctionHolderMaker> function_holder_maker = std::nullopt);

  /// \brief get a list of C functions saved in the registry
  const std::vector<std::pair<NativeFunction, void*>>& GetCFunctions() const;

  const FunctionHolderMakerRegistry& GetFunctionHolderMakerRegistry() const;

Closes: [C++][Gandiva] Allow registering external C functions #38589

Notes

This PR is related with GH-37753: [C++][Gandiva] Add external function registry support #38116, which adds the initial support for registering LLVM IR based external functions into Gandiva.

github-actions · 2023-11-08T06:01:17Z

⚠️ GitHub issue #38589 has been automatically assigned in GitHub to PR creator.

niyue · 2023-11-08T06:11:18Z

cpp/src/gandiva/external_c_interface_functions.cc

+  return std::make_pair(args, ret_llvm_type);
+}
+
+arrow::Status ExternalCInterfaceFunctions::AddMappings(Engine* engine) const {


Previously, the LLVM types of stub functions arguments are hard coded in the code base, now we automatically convert arrow data types of these function's parameter types into their LLVM equivalent types so that users don't have to specify similar info twice.

niyue · 2023-11-08T06:12:22Z

cpp/src/gandiva/function_holder_registry.h

-  }
-
- private:
-  static map_type& makers() {


Move into cc file and make this state non static by copying them into each registry's instance.

niyue · 2023-11-08T06:13:04Z

cpp/src/gandiva/function_registry.h

+  /// function requires a function holder
+  arrow::Status Register(
+      NativeFunction func, void* c_interface_function_ptr,
+      std::optional<FunctionHolderMaker> function_holder_maker = std::nullopt);


This is the new API used for registering external C interface functions. If a function requires function holder, it can use the function_holder_maker to specify it

niyue · 2023-11-08T06:18:50Z

cpp/src/gandiva/tests/projector_test.cc

@@ -3608,4 +3608,80 @@ TEST_F(TestProjector, TestExtendedFunctions) {
  EXPECT_ARROW_ARRAY_EQUALS(out, outs.at(0));
 }

+TEST_F(TestProjector, TestExtendedCInterfaceFunctions) {


This test case demonstrates registering an external C interface function (but this function doesn't use function holder/context)

niyue · 2023-11-08T06:19:10Z

cpp/src/gandiva/tests/projector_test.cc

+  EXPECT_ARROW_ARRAY_EQUALS(out, outs.at(0));
+}
+
+TEST_F(TestProjector, TestExtendedCInterfaceFunctionsWithFunctionHolder) {


This test case demonstrates registering an external C interface function that uses function holder

niyue · 2023-11-08T06:19:58Z

cpp/src/gandiva/tests/projector_test.cc

+  EXPECT_ARROW_ARRAY_EQUALS(out, outs.at(0));
+}
+
+TEST_F(TestProjector, TestExtendedCInterfaceFunctionThatNeedsContext) {


This test case demonstrates registering an external C interface function that uses context.

niyue · 2023-11-08T14:56:51Z

cpp/src/gandiva/engine.cc

+arrow::Status Engine::AddGlobalMappings() {
+  ARROW_RETURN_NOT_OK(ExportedFuncsRegistry::AddMappings(this));
+  ExternalCInterfaceFunctions c_interface_funcs(function_registry_);
+  return c_interface_funcs.AddMappings(this);


The external C interface functions are added and mapped to the JIT engine here.

niyue · 2023-11-08T14:59:38Z

cpp/src/gandiva/decimal_xlarge.cc

@@ -38,7 +38,7 @@

 namespace gandiva {

-void ExportedDecimalFunctions::AddMappings(Engine* engine) const {
+arrow::Status ExportedDecimalFunctions::AddMappings(Engine* engine) const {


For external C interface functions, they may cause some error when adding mapping for them, so I change the AddGlobalMappings and ExportedFuncsBase::AddMappings functions to return arrow::Status to represent the result

niyue · 2023-11-08T15:18:20Z

@js8544 @kou
As discussed in the recent PR #38116, I submitted a follow up PR to further enhance Gandiva's external function registration capability, allowing registering C interface functions (stub function called by Gandiva internally) in this PR. Could you please help to review? Thanks.

notes

The name C interface function is different with stub function since I think stub function may not be easily understood externally, and I am looking for something equivalent to LLVM IR based function but I am not sure if there is a better term than C interface function, please advice.
I am not sure if I need to post a new discussion thread to the mailing list since it was discussed previously for PR 38116, and this PR is a smaller enhancement to the previous one. Please let me know if I need to post a new discussion thread for this. Thanks.

kou

How about naming "stub function" as "C function" not "C interface function"?
(And how about naming LLVM IR based function as "IR function"?)
It seems that "interface" is redundant.

cpp/src/gandiva/external_c_interface_functions.cc

cpp/src/gandiva/function_holder_maker_registry.cc

cpp/src/gandiva/function_registry.cc

cpp/src/gandiva/gdv_string_function_stubs.cc

cpp/src/gandiva/tests/test_util.cc

niyue · 2023-11-09T12:16:52Z

How about naming "stub function" as "C function" not "C interface function"?

Thanks for the suggestion. I rename the PR/corresponding issue description/code to use the name C function now.

kou

+1

@js8544 You may want to review this too.

I am not sure if I need to post a new discussion thread to the mailing list since it was discussed previously for PR 38116, and this PR is a smaller enhancement to the previous one.

I think that we don't need this because we don't have a new specification here.

cpp/src/gandiva/external_c_functions.cc

kou · 2023-11-09T20:58:19Z

cpp/src/gandiva/function_holder_maker_registry.cc

+template <typename HolderType>
+static arrow::Result<FunctionHolderPtr> HolderMaker(const FunctionNode& node) {
+  std::shared_ptr<HolderType> derived_instance;
+  ARROW_RETURN_NOT_OK(HolderType::Make(node, &derived_instance));
+  return derived_instance;
+}


If we change HolderType::Make() to use Result instead of Status (e.g. Status LikeHolder::Make(...) -> Result<std::shared_ptr<LikeHolder>> LikeHolder::Make(...)), we can remove this helper template function?
If so, we can do it as a follow-up task.

Sure. If I make this change, we need to change some existing classes, like LikeHolder/ReplaceHolder/etc. They are GANDIVA_EXPORT classes, but I am not sure if they are really used externally. The code will be simpler after this refactoring. I am glad to submit another PR for this if this is desired.

cpp/src/gandiva/function_registry.cc

js8544

+1. Thanks! Just a couple of nits.

js8544 · 2023-11-10T02:51:50Z

cpp/src/gandiva/expr_decomposer.cc

 #include "gandiva/function_registry.h"
 #include "gandiva/function_signature.h"
 #include "gandiva/in_holder.h"
 #include "gandiva/node.h"
+#include "gandiva/regex_functions_holder.h"


Is this include needed?

I found LikeHolder is used below but its header is not included so I added it (it is likely included indirectly by other header files ). Let me know if this is not recommended in the project (if not included, I ran into some issues in other projects, during refactoring, a indirectly included header file was removed, and causing the other file failed to be compiled)

cpp/src/gandiva/external_c_functions.cc

cpp/src/gandiva/function_registry.cc

kou

I'll merge in a few days if nobody objects it.

cpp/src/gandiva/CMakeLists.txt

cpp/src/gandiva/external_c_functions.cc

…s parameter.

…ry can work.

…stent.

…for LLVM generator using external C interface functions.

…d with correct visibility on Windows.

…e the correct number of arguments up front.

…s into anonymous namespace.

niyue · 2023-11-17T02:53:04Z

To resolve merge conflicts, rebased onto the latest main branch.

kou · 2023-11-17T07:58:52Z

No objection. I'll merge this.

conbench-apache-arrow · 2023-11-17T11:14:26Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit c353c81.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ult (#38873) ### Rationale for this change * This PR tries to make Gandiva `FunctionHolder` classes to return `arrow::Result` instead of using output parameters, and this tries to address the follow up task mentioned in #38632 (comment) and makes the code slightly simpler ### What changes are included in this PR? * A refactoring task to return `arrow::Result` in Gandiva FunctionHolder classes ### Are these changes tested? It should be covered by existing unit tests. ### Are there any user-facing changes? No * Closes: #38920 Authored-by: Yue Ni <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

…ns (apache#38632) ### Rationale for this change This PR tries to enhance Gandiva by supporting registering external C functions to its function registry, so that developers can author third party functions with complex dependency and expose them as C functions to be used in Gandiva expression. See more details in apacheGH-38589. ### What changes are included in this PR? This PR primarily adds a new API to the `FunctionRegistry` so that developers can use it to register external C functions: ```C++ arrow::Status Register( NativeFunction func, void* c_function_ptr, std::optional<FunctionHolderMaker> function_holder_maker = std::nullopt); ``` ### Are these changes tested? * The changes are tested via unit tests in this PR, and the unit tests include several C functions written using C++ and we confirm this kind of functions can be used by Gandiva after registration using the above mentioned new API. * Additionally, locally I wrote some Rust based functions, and integrate the Rust based functions into a C++ program by using the new registration API and verified this approach did work, but this piece of work is not included in the PR. ### Are there any user-facing changes? There are several new APIs added to `FunctionRegistry` class: ```C++ /// \brief register a C function into the function registry /// @ param func the registered function's metadata /// @ param c_function_ptr the function pointer to the /// registered function's implementation /// @ param function_holder_maker this will be used as the function holder if the /// function requires a function holder arrow::Status Register( NativeFunction func, void* c_function_ptr, std::optional<FunctionHolderMaker> function_holder_maker = std::nullopt); /// \brief get a list of C functions saved in the registry const std::vector<std::pair<NativeFunction, void*>>& GetCFunctions() const; const FunctionHolderMakerRegistry& GetFunctionHolderMakerRegistry() const; ``` * Closes: apache#38589 ### Notes * This PR is related with apache#38116, which adds the initial support for registering LLVM IR based external functions into Gandiva. Authored-by: Yue Ni <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

…ow Result (apache#38873) ### Rationale for this change * This PR tries to make Gandiva `FunctionHolder` classes to return `arrow::Result` instead of using output parameters, and this tries to address the follow up task mentioned in apache#38632 (comment) and makes the code slightly simpler ### What changes are included in this PR? * A refactoring task to return `arrow::Result` in Gandiva FunctionHolder classes ### Are these changes tested? It should be covered by existing unit tests. ### Are there any user-facing changes? No * Closes: apache#38920 Authored-by: Yue Ni <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

github-actions bot added awaiting review Awaiting review Component: C++ Component: C++ - Gandiva awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 8, 2023

niyue force-pushed the feature/external-c-interface-func branch from 77a8135 to 2fcd234 Compare November 8, 2023 06:10

niyue commented Nov 8, 2023

View reviewed changes

niyue force-pushed the feature/external-c-interface-func branch from 2fcd234 to 26491ce Compare November 8, 2023 06:15

niyue commented Nov 8, 2023

View reviewed changes

kou reviewed Nov 9, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Nov 9, 2023

niyue changed the title ~~GH-38589: [C++][Gandiva] Support registering external C interface functions~~ GH-38589: [C++][Gandiva] Support registering external C functions Nov 9, 2023

niyue force-pushed the feature/external-c-interface-func branch from 33c1bb1 to c8aaa50 Compare November 9, 2023 11:40

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 9, 2023

niyue force-pushed the feature/external-c-interface-func branch from c8aaa50 to 258f613 Compare November 9, 2023 12:13

niyue mentioned this pull request Nov 9, 2023

[C++][Gandiva] Allow registering external C functions #38589

Closed

niyue force-pushed the feature/external-c-interface-func branch from 258f613 to e2c3f24 Compare November 9, 2023 13:39

kou approved these changes Nov 9, 2023

View reviewed changes

github-actions bot removed the awaiting change review Awaiting change review label Nov 9, 2023

github-actions bot added the awaiting merge Awaiting merge label Nov 9, 2023

js8544 approved these changes Nov 10, 2023

View reviewed changes

kou reviewed Nov 13, 2023

View reviewed changes

cpp/src/gandiva/CMakeLists.txt Outdated Show resolved Hide resolved

cpp/src/gandiva/external_c_functions.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting merge Awaiting merge awaiting changes Awaiting changes labels Nov 13, 2023

niyue added 9 commits November 17, 2023 10:48

Add initial support for extending gandiva stub functions.

0a86c38

Support registering gandiva functions that requires function holder a…

2eb0237

…s parameter.

Verify the external gandiva function that needs context to alloc memo…

23ae3b5

…ry can work.

Rename public API to use c interface function to make this term consi…

3871aac

…stent.

Add global mapping for external c interface functions and more tests …

93d8ac0

…for LLVM generator using external C interface functions.

Export gandiva context related function calls so that they can be use…

217e916

…d with correct visibility on Windows.

Rename C interface function to C function for simplicity.

427556d

Refactor the code to use the IRtype API in engine class, and calculat…

e9e248b

…e the correct number of arguments up front.

Change several functions in external_c_functions from static function…

ec24d23

…s into anonymous namespace.

niyue force-pushed the feature/external-c-interface-func branch from a3d0625 to ec24d23 Compare November 17, 2023 02:51

kou merged commit c353c81 into apache:main Nov 17, 2023
30 of 31 checks passed

kou removed the awaiting change review Awaiting change review label Nov 17, 2023

This was referenced Nov 24, 2023

GH-38920: [C++][Gandiva] Refactor function holder to return arrow Result #38873

Merged

[C++][Gandiva] Refactor function holder to return arrow Result #38920

Closed

niyue mentioned this pull request Dec 3, 2023

[C++][Gandiva] Refactor built-in stub functions to use FunctionRegistry for registration #39052

Open

kou mentioned this pull request Mar 22, 2024

[Gandiva] 'ilike' function does not work #40727

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38589: [C++][Gandiva] Support registering external C functions #38632

GH-38589: [C++][Gandiva] Support registering external C functions #38632

niyue commented Nov 8, 2023 •

edited

Loading

github-actions bot commented Nov 8, 2023

niyue Nov 8, 2023

niyue Nov 8, 2023

niyue Nov 8, 2023 •

edited

Loading

niyue Nov 8, 2023

niyue Nov 8, 2023

niyue Nov 8, 2023

niyue Nov 8, 2023

niyue Nov 8, 2023

niyue commented Nov 8, 2023

kou left a comment

niyue commented Nov 9, 2023

kou left a comment

kou Nov 9, 2023

niyue Nov 10, 2023

js8544 left a comment

js8544 Nov 10, 2023

niyue Nov 10, 2023 •

edited

Loading

kou left a comment

niyue commented Nov 17, 2023 •

edited

Loading

kou commented Nov 17, 2023

conbench-apache-arrow bot commented Nov 17, 2023

GH-38589: [C++][Gandiva] Support registering external C functions #38632

GH-38589: [C++][Gandiva] Support registering external C functions #38632

Conversation

niyue commented Nov 8, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Notes

github-actions bot commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niyue Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niyue commented Nov 8, 2023

notes

kou left a comment

Choose a reason for hiding this comment

niyue commented Nov 9, 2023

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

js8544 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niyue Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

kou left a comment

Choose a reason for hiding this comment

niyue commented Nov 17, 2023 • edited Loading

kou commented Nov 17, 2023

conbench-apache-arrow bot commented Nov 17, 2023

niyue commented Nov 8, 2023 •

edited

Loading

niyue Nov 8, 2023 •

edited

Loading

niyue Nov 10, 2023 •

edited

Loading

niyue commented Nov 17, 2023 •

edited

Loading