[native] Dynamically Linked Library in Presto CPP #24330

soumiiow · 2025-01-07T17:10:21Z

Description

Depends on facebookincubator/velox#11439 in the Velox space
and based off of the following PR: https://github.com/facebookincubator/velox/pull/1005/files

Motivation and Context

Having these changes will enable users to register custom functions dynamically without requiring a fork of Prestissimo.

Impact

This extends Prestissimo functionality to include dynamic loading of functions, types, connectors, etc.

Test Plan

Unit tested. and Manually end to end tested the changes.

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... :pr:`12345`
* ... :pr:`12345`

Hive Connector Changes
* ... :pr:`12345`
* ... :pr:`12345`

If release note is NOT required, use:

== NO RELEASE NOTE ==

linux-foundation-easycla · 2025-01-07T17:10:27Z

The committers listed above are authorized under a signed CLA.

✅ login: soumiiow / name: Soumya Duriseti (27bd55e)

czentgr

Please also rebase.

czentgr · 2025-02-03T19:32:42Z

presto-native-execution/presto_cpp/main/CMakeLists.txt

@@ -76,7 +76,8 @@ target_link_libraries(
  ${FOLLY_WITH_DEPENDENCIES}
  ${GLOG}
  ${GFLAGS_LIBRARIES}
-  pthread)
+  pthread
+  velox_dynamic_function_loader)


Move this before the velox_encode library.

czentgr · 2025-02-03T19:33:09Z

presto-native-execution/presto_cpp/main/CMakeLists.txt

@@ -89,6 +90,7 @@ set_property(TARGET presto_server_lib PROPERTY JOB_POOL_LINK
                                               presto_link_job_pool)

 add_executable(presto_server PrestoMain.cpp)
+target_link_options(presto_server BEFORE PUBLIC "-Wl,-export-dynamic")


Can we add a comment here why we need these flags?

czentgr · 2025-02-03T19:35:24Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

+    const fs::path path(systemConfig->pluginDir());
+    PRESTO_STARTUP_LOG(INFO) << path;
+    std::error_code
+        ec; // For using the non-throwing overloads of functions below.


We don;t need this comment here and so can fix up the odd formatting.

czentgr · 2025-02-03T19:35:52Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

+void PrestoServer::registerDynamicFunctions() {
+  auto systemConfig = SystemConfig::instance();
+  if (!systemConfig->pluginDir().empty()) {
+    // if it is a valid directory, traverse and call dynamic function loader


Please make sure the comments are full sentences beginning with capitalization etc.

czentgr · 2025-02-03T19:37:42Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

+        auto dirEntryPath = dirEntry.path();
+        if (!fs::is_directory(dirEntry, ec) &&
+            extensions.find(dirEntryPath.extension()) != extensions.end()) {
+          facebook::velox::loadDynamicLibrary(dirEntryPath.c_str());


facebook is not needed here because we are already in the facebook namespace.

steveburnett

Thanks for the doc! A few minor formatting and phrasing suggestions.

steveburnett · 2025-02-04T14:43:55Z

presto-native-execution/presto_cpp/main/dynamic_registry/README.md

@@ -0,0 +1,17 @@
+# Dynamic Loading of Presto Cpp Extensions


Suggested change

# Dynamic Loading of Presto Cpp Extensions

# Dynamic Loading of Presto CPP Extensions

steveburnett · 2025-02-04T14:44:16Z

presto-native-execution/presto_cpp/main/dynamic_registry/README.md

@@ -0,0 +1,17 @@
+# Dynamic Loading of Presto Cpp Extensions
+This library adds the ability to load User Defined Functions (UDFs), connectors, or types without having to fork and build Prestissimo, through the use of shared libraries that a Prestissimo worker can access. These are to be loaded on launch of the Presto server. The Presto server searches for any .so or .dylib files and loads them using this library.


Suggested change

This library adds the ability to load User Defined Functions (UDFs), connectors, or types without having to fork and build Prestissimo, through the use of shared libraries that a Prestissimo worker can access. These are to be loaded on launch of the Presto server. The Presto server searches for any .so or .dylib files and loads them using this library.

This library adds the ability to load User Defined Functions (UDFs), connectors, or types without having to fork and build Prestissimo, through the use of shared libraries that a Prestissimo worker can access. These are loaded on launch of the Presto server. The Presto server searches for any .so or .dylib files and loads them using this library.

steveburnett · 2025-02-04T14:45:42Z

presto-native-execution/presto_cpp/main/dynamic_registry/README.md

+This library adds the ability to load User Defined Functions (UDFs), connectors, or types without having to fork and build Prestissimo, through the use of shared libraries that a Prestissimo worker can access. These are to be loaded on launch of the Presto server. The Presto server searches for any .so or .dylib files and loads them using this library.
+## Getting started
+1. Create a cpp file for your dynamic library
+For dynamically loaded function registration, the format followed is mirrored of that of built-in function registration with some noted differences. Using [MyDynamicFunction.cpp](examples/MyDynamicFunction.cpp) as an example, the function uses the extern "C" keyword to protect against name mangling. A registry() function call is also necessary here.


Suggested change

For dynamically loaded function registration, the format followed is mirrored of that of built-in function registration with some noted differences. Using [MyDynamicFunction.cpp](examples/MyDynamicFunction.cpp) as an example, the function uses the extern "C" keyword to protect against name mangling. A registry() function call is also necessary here.

For dynamically loaded function registration, the format is similar to that of built-in function registration, with some noted differences. Using [MyDynamicFunction.cpp](examples/MyDynamicFunction.cpp) as an example, the function uses the extern "C" keyword to protect against name mangling. A registry() function call is also necessary here.

presto-native-execution/presto_cpp/main/dynamic_registry/README.md

steveburnett · 2025-02-04T14:54:55Z

presto-native-execution/presto_cpp/main/dynamic_registry/README.md

+```
+plugin.dir="User\Test\Path\plugin"
+```
+4. When the worker or the sidecar process starts, it will scan the plugin directory and attempt to dynamically load all shared libraries


Suggested change

4. When the worker or the sidecar process starts, it will scan the plugin directory and attempt to dynamically load all shared libraries

When the worker or the sidecar process starts, it scans the plugin directory and attempts to dynamically load all shared libraries.

I don't think of this as part of the steps to configure, it's what happens when things are run using the config in steps 1-3.

steveburnett

Thanks for the documentation! I really liked your including the setup steps on plugin.rst. Some minor suggestions for formatting and phrasing but looks good overall.

presto-native-execution/presto_cpp/main/dynamic_registry/README.md

presto-docs/src/main/sphinx/presto_cpp/plugin.rst

presto-docs/src/main/sphinx/presto_cpp/plugin/function_plugin.rst

soumiiow · 2025-02-11T22:24:12Z

Thanks @steveburnett please take another look, I've made the changes. I'm not sure about the tone on the intro to UDFs i have on function_plugin.rst, would appreciate another set of eyes there.

steveburnett

Thanks for the revision! Nice work, your unordered list of UDF benefits was great and the format fixes in the README look good.

I made a couple of small suggestions about the intro to UDFs, let me know what you think.

presto-docs/src/main/sphinx/presto_cpp/plugin/function_plugin.rst

steveburnett

I should have noticed this spelling nit earlier! After I found this one, I did a complete review of the doc in this PR and found no other errors so I think this is the last one.

presto-docs/src/main/sphinx/presto_cpp/plugin/function_plugin.rst

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, complete review of new content. No concerns, looks good. Thanks for working with me on this!

czentgr

Thanks, a couple of comments.

czentgr · 2025-02-13T20:14:05Z

presto-docs/src/main/sphinx/presto_cpp/plugin.rst

+
+2. Set the ``plugin.dir`` property to the path of the ``plugins`` directory in the ``config.properties`` file of each of your workers.
+
+3. Run the coordinator and workers to load your plugins.


Nit: should we say start or restart the coordinator/worker to pick up any placed libraries? Run could mean it is already running? It's a bit ambiguous to me in this context.

czentgr · 2025-02-13T22:34:27Z

presto-docs/src/main/sphinx/presto_cpp/plugin/function_plugin.rst

+Creating a Shared Library for UDFs
+----------------------------------
+User defined functions (UDFs) allow users to create custom functions without the need to rebuild the executable. 
+There are many benefits to UDFs other than shorter compile times, such as:


"other than shorter compile times". An user wouldn't care about compile times. I would just leave the "other than ..." out and simply state the benefits.

czentgr · 2025-02-13T22:36:12Z

presto-docs/src/main/sphinx/presto_cpp/plugin/function_plugin.rst

+      #include "presto_cpp/main/dynamic_registry/DynamicFunctionRegistrar.h"
+
+      template <typename T>
+      struct nameOfStruct {


In most (all?) the code conventions a struct/class name starts with upper case for a class or struct so should do the same here (and then usage later).

czentgr · 2025-02-13T22:38:57Z

presto-docs/src/main/sphinx/presto_cpp/plugin/function_plugin.rst

+
+   Note: the ``int64_t`` return type can be changed as needed. For more examples, see the `README <https://github.com/prestodb/presto-native-execution/main/dynamic_registry/README.md>`_.
+
+2. Create a shared library which may be made using CMakeLists like the following:


Nit: ... may be made using a CMakeLists.txt like ...

We should highlight the file name because it is fixed for a directory.

czentgr · 2025-02-13T22:40:45Z

presto-native-execution/presto_cpp/main/common/Configs.h

@@ -642,6 +642,8 @@ class SystemConfig : public ConfigBase {
  static constexpr std::string_view kInternalCommunicationJwtExpirationSeconds{
      "internal-communication.jwt.expiration-seconds"};

+  /// Optional string containing the path to the plugin directory
+  static constexpr std::string_view kPluginDir{"plugin.dir"};


Nit: add new line before comments on line 647.

czentgr · 2025-02-13T22:45:55Z

presto-docs/src/main/sphinx/presto_cpp/plugin/function_plugin.rst

+      add_library(name_of_dynamic_fn SHARED TestFunction.cpp)
+      target_link_libraries(name_of_dynamic_fn PRIVATE fmt::fmt Folly::folly gflags::gflags)
+
+3. Put your shared libraries in the plugin directory described in :doc:`../plugin`.


This is not true anymore.
The user must configure the new plugin.dir variable to indicate where to pick up the new libraries. If it is the default (empty string) nothing happens.
On the CPP side this is good because we likely need some kind of option where libraries are not loaded even if a library is present - aka never load anything dynamically as otherwise it is too easy to exploit. Java is a bit more secure in that regard.

czentgr · 2025-02-13T22:51:15Z

presto-native-execution/presto_cpp/main/dynamic_registry/DynamicFunctionRegistrar.h

+
+#include "presto_cpp/main/common/Configs.h"
+#include "velox/functions/Macros.h"
+#include "velox/functions/Registerer.h"


Nit: leave a new line between the last include and the namespace.

czentgr · 2025-02-13T22:53:30Z

presto-docs/src/main/sphinx/presto_cpp/plugin/function_plugin.rst

@@ -0,0 +1,44 @@
+=======================


Do we still need the README if we have the documentation here?
There is some divergence between the readme and this.

Perhaps the readme can just point to the documentation link instead? We've done something similar with the velox part of the PR.

We could also link to the Velox piece that is underlying the Presto part.

czentgr · 2025-02-13T22:55:22Z

presto-native-execution/presto_cpp/main/dynamic_registry/examples/MyDynamicFunction.cpp

+} // namespace facebook::velox::common::dynamicRegistry
+
+extern "C" {
+// In this case, we assume that facebook::velox::registerFunction


Do we need this comment? The user here doesn't know about the Velox API. They don't need to know, do they?
Or it might be explained in the doc what it relies on?

czentgr · 2025-02-13T22:56:17Z

presto-native-execution/presto_cpp/main/dynamic_registry/examples/MyDynamicVarcharFunction.cpp

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include "presto_cpp/main/dynamic_registry/DynamicFunctionRegistrar.h"


Nit: please add a new line between include and comment below.

aditi-pandit

@soumiiow : Thanks for this code. Would be great to have an e2e test with a SQL statement using the dynamic functions. On the lines of https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/TestPrestoNativeRemoteFunctions.java

aditi-pandit · 2025-02-14T22:53:47Z

presto-native-execution/presto_cpp/main/dynamic_registry/DynamicFunctionRegistrar.h

+    auto systemConfig = SystemConfig::instance();
+    cpp_nameSpace = systemConfig->prestoDefaultNamespacePrefix();
+  }
+  std::string cpp_name(cpp_nameSpace);


Is there an assumption that namespace should end in "." ? We should make this behavior more robust by adding the period separator if its not present.

aditi-pandit · 2025-02-14T22:54:20Z

presto-native-execution/presto_cpp/main/dynamic_registry/DynamicFunctionRegistrar.h

+    const char* nameSpace = "",
+    const std::vector<velox::exec::SignatureVariable>& constraints = {},
+    bool overwrite = true) {
+  std::string cpp_nameSpace(nameSpace);


use camelcase cppNamespace

aditi-pandit · 2025-02-14T22:56:25Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

@@ -1598,5 +1600,28 @@ protocol::NodeStatus PrestoServer::fetchNodeStatus() {

  return nodeStatus;
 }
+void PrestoServer::registerDynamicFunctions() {
+  auto systemConfig = SystemConfig::instance();


Might be good to have a config of library names to load rather than load all those found in the directory. wdyt ?

aditi-pandit · 2025-02-14T22:58:31Z

presto-native-execution/presto_cpp/main/dynamic_registry/examples/MyDynamicArrayFunction.cpp

Please add an example of a function which is added to the non-default namespace.

soumiiow self-assigned this Jan 7, 2025

soumiiow force-pushed the dylib_new branch from 58ce37c to aedfb72 Compare January 7, 2025 17:16

soumiiow marked this pull request as ready for review January 7, 2025 18:04

soumiiow requested a review from a team as a code owner January 7, 2025 18:04

tdcmeehan self-assigned this Jan 8, 2025

soumiiow mentioned this pull request Jan 14, 2025

presto requires namespaces when using function registration #24363

Open

tdcmeehan added the from:IBM PR from IBM label Jan 30, 2025

prestodb-ci requested review from a team, pdabre12 and psnv03 and removed request for a team January 30, 2025 18:47

czentgr reviewed Feb 3, 2025

View reviewed changes

soumiiow force-pushed the dylib_new branch from aedfb72 to 9cde5a4 Compare February 3, 2025 21:23

soumiiow requested review from steveburnett and a team as code owners February 3, 2025 21:23

soumiiow requested a review from presto-oss February 3, 2025 21:23

steveburnett requested changes Feb 4, 2025

View reviewed changes

soumiiow force-pushed the dylib_new branch from 9cde5a4 to 689b24e Compare February 11, 2025 09:31

soumiiow requested a review from elharo as a code owner February 11, 2025 09:31

soumiiow requested a review from steveburnett February 11, 2025 09:33

soumiiow force-pushed the dylib_new branch from 689b24e to 6cb96e7 Compare February 11, 2025 10:00

steveburnett requested changes Feb 11, 2025

View reviewed changes

soumiiow force-pushed the dylib_new branch from 6cb96e7 to 0ab37d6 Compare February 11, 2025 22:20

steveburnett requested changes Feb 11, 2025

View reviewed changes

presto-docs/src/main/sphinx/presto_cpp/plugin/function_plugin.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/presto_cpp/plugin/function_plugin.rst Outdated Show resolved Hide resolved

soumiiow force-pushed the dylib_new branch from 0ab37d6 to d8b7dc2 Compare February 12, 2025 18:51

soumiiow requested a review from steveburnett February 12, 2025 18:52

steveburnett requested changes Feb 12, 2025

View reviewed changes

presto-docs/src/main/sphinx/presto_cpp/plugin/function_plugin.rst Outdated Show resolved Hide resolved

dylib presto changes + presto-docs + readme + examples

27bd55e

soumiiow force-pushed the dylib_new branch from d8b7dc2 to 27bd55e Compare February 12, 2025 19:15

soumiiow requested a review from steveburnett February 12, 2025 19:16

steveburnett approved these changes Feb 12, 2025

View reviewed changes

czentgr reviewed Feb 13, 2025

View reviewed changes

aditi-pandit changed the title ~~Dynamically Linked Library in Presto CPP~~ [native] Dynamically Linked Library in Presto CPP Feb 13, 2025

aditi-pandit reviewed Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native] Dynamically Linked Library in Presto CPP #24330

[native] Dynamically Linked Library in Presto CPP #24330

soumiiow commented Jan 7, 2025

linux-foundation-easycla bot commented Jan 7, 2025 •

edited

Loading

czentgr left a comment

czentgr Feb 3, 2025

czentgr Feb 3, 2025

czentgr Feb 3, 2025

czentgr Feb 3, 2025

czentgr Feb 3, 2025

steveburnett left a comment

steveburnett Feb 4, 2025

steveburnett Feb 4, 2025

steveburnett Feb 4, 2025

steveburnett Feb 4, 2025

steveburnett left a comment

soumiiow commented Feb 11, 2025

steveburnett left a comment

steveburnett left a comment

steveburnett left a comment

czentgr left a comment

czentgr Feb 13, 2025

czentgr Feb 13, 2025

czentgr Feb 13, 2025

czentgr Feb 13, 2025

czentgr Feb 13, 2025

czentgr Feb 13, 2025

czentgr Feb 13, 2025

czentgr Feb 13, 2025

czentgr Feb 13, 2025

czentgr Feb 13, 2025

aditi-pandit left a comment

aditi-pandit Feb 14, 2025

aditi-pandit Feb 14, 2025

aditi-pandit Feb 14, 2025

aditi-pandit Feb 14, 2025

	# Dynamic Loading of Presto Cpp Extensions
	# Dynamic Loading of Presto CPP Extensions

		@@ -0,0 +1,17 @@
		# Dynamic Loading of Presto Cpp Extensions
		This library adds the ability to load User Defined Functions (UDFs), connectors, or types without having to fork and build Prestissimo, through the use of shared libraries that a Prestissimo worker can access. These are to be loaded on launch of the Presto server. The Presto server searches for any .so or .dylib files and loads them using this library.

	For dynamically loaded function registration, the format followed is mirrored of that of built-in function registration with some noted differences. Using [MyDynamicFunction.cpp](examples/MyDynamicFunction.cpp) as an example, the function uses the extern "C" keyword to protect against name mangling. A registry() function call is also necessary here.
	For dynamically loaded function registration, the format is similar to that of built-in function registration, with some noted differences. Using [MyDynamicFunction.cpp](examples/MyDynamicFunction.cpp) as an example, the function uses the extern "C" keyword to protect against name mangling. A registry() function call is also necessary here.

	4. When the worker or the sidecar process starts, it will scan the plugin directory and attempt to dynamically load all shared libraries
	When the worker or the sidecar process starts, it scans the plugin directory and attempts to dynamically load all shared libraries.


		2. Set the ``plugin.dir`` property to the path of the ``plugins`` directory in the ``config.properties`` file of each of your workers.

		3. Run the coordinator and workers to load your plugins.


		Note: the ``int64_t`` return type can be changed as needed. For more examples, see the `README <https://github.com/prestodb/presto-native-execution/main/dynamic_registry/README.md>`_.

		2. Create a shared library which may be made using CMakeLists like the following:

[native] Dynamically Linked Library in Presto CPP #24330

Are you sure you want to change the base?

[native] Dynamically Linked Library in Presto CPP #24330

Conversation

soumiiow commented Jan 7, 2025

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

linux-foundation-easycla bot commented Jan 7, 2025 • edited Loading

czentgr left a 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

Choose a reason for hiding this comment

steveburnett left a 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

steveburnett left a comment

Choose a reason for hiding this comment

soumiiow commented Feb 11, 2025

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

czentgr left a 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

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

aditi-pandit left a 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

linux-foundation-easycla bot commented Jan 7, 2025 •

edited

Loading