feat: Make registry symbol name customizable for dynamic component registration (facebookincubator#12878)

Summary:
Users can now create shared libraries for dynamic component registration with these proposed changes

Pull Request resolved: facebookincubator#12878

Reviewed By: gggrace14

Differential Revision: D74858268

Pulled By: xiaoxmeng

fbshipit-source-id: 6e7b07d6a80c68c2095ce6a845d968306a1cd62c

Author: soumiiow

velox/common/dynamic_registry/DynamicLibraryLoader.cpp

@@ -20,9 +20,9 @@
 
 namespace facebook::velox {
 
-static constexpr const char* kSymbolName = "registry";
-
-void loadDynamicLibrary(const std::string& fileName) {
+void loadDynamicLibrary(
+    const std::string& fileName,
+    const std::string& registrationFunctionName) {
   // Try to dynamically load the shared library.
   void* handler = dlopen(fileName.c_str(), RTLD_NOW);
 
@@ -31,9 +31,10 @@ void loadDynamicLibrary(const std::string& fileName) {
   }
 
   LOG(INFO) << "Loaded library " << fileName << ". Searching registry symbol "
-            << kSymbolName;
+            << registrationFunctionName;
+
   // Lookup the symbol.
-  void* registrySymbol = dlsym(handler, kSymbolName);
+  void* registrySymbol = dlsym(handler, registrationFunctionName.c_str());
   auto loadUserLibrary = reinterpret_cast<void (*)()>(registrySymbol);
   const char* error = dlerror();
 
@@ -45,13 +46,13 @@ void loadDynamicLibrary(const std::string& fileName) {
 
   if (loadUserLibrary == nullptr) {
     VELOX_USER_FAIL(
-        "Symbol '{}' resolved to a nullptr, unable to invoke it.", kSymbolName);
+        "Symbol '{}' resolved to a nullptr, unable to invoke it.",
+        registrationFunctionName);
   }
 
   // Invoke the registry function.
-  LOG(INFO) << "Found registry function. Invoking it.";
   loadUserLibrary();
-  LOG(INFO) << "Registration complete.";
+  LOG(INFO) << "Registered functions by " << registrationFunctionName;
 }
 
 } // namespace facebook::velox

velox/common/dynamic_registry/DynamicLibraryLoader.h

@@ -38,6 +38,9 @@ namespace facebook::velox {
 ///
 /// Loading a library twice can cause a components to be registered twice.
 /// This can fail for certain Velox components.
-void loadDynamicLibrary(const std::string& fileName);
+
+void loadDynamicLibrary(
+    const std::string& fileName,
+    const std::string& entryPointSymbol = "registerExtensions");
 
 } // namespace facebook::velox

velox/common/dynamic_registry/DynamicUdf.h

@@ -26,6 +26,8 @@ add_library(velox_overload_int_function_dynamic SHARED
             DynamicIntFunctionOverload.cpp)
 add_library(velox_overload_varchar_function_dynamic SHARED
             DynamicVarcharFunctionOverload.cpp)
+add_library(velox_function_non_default_dynamic SHARED
+            DynamicFunctionNonDefault.cpp)
 
 set(CMAKE_DYLIB_TEST_LINK_LIBRARIES fmt::fmt Folly::folly glog::glog xsimd)
 
@@ -53,6 +55,10 @@ target_link_libraries(
   velox_overload_varchar_function_dynamic
   PRIVATE ${CMAKE_DYLIB_TEST_LINK_LIBRARIES})
 
+target_link_libraries(
+  velox_function_non_default_dynamic
+  PRIVATE ${CMAKE_DYLIB_TEST_LINK_LIBRARIES})
+
 if(APPLE)
   set(COMMON_LIBRARY_LINK_OPTIONS "-Wl,-undefined,dynamic_lookup")
 else()
@@ -74,6 +80,8 @@ target_link_options(velox_overload_int_function_dynamic PRIVATE
                     ${COMMON_LIBRARY_LINK_OPTIONS})
 target_link_options(velox_overload_varchar_function_dynamic PRIVATE
                     ${COMMON_LIBRARY_LINK_OPTIONS})
+target_link_options(velox_function_non_default_dynamic PRIVATE
+                    ${COMMON_LIBRARY_LINK_OPTIONS})
 
 # Here's the actual test which will dynamically load the library defined above.
 add_executable(velox_function_dynamic_link_test DynamicLinkTest.cpp)

velox/common/dynamic_registry/tests/CMakeLists.txt

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include "velox/common/dynamic_registry/DynamicUdf.h"
+#include "velox/functions/Udf.h"
 
 // This file defines a mock function that will be dynamically linked and
 // registered. There are no restrictions as to how the function needs to be
@@ -39,7 +39,7 @@ struct DynamicFunction {
 } // namespace facebook::velox::common::dynamicRegistry
 
 extern "C" {
-void registry() {
+void registerExtensions() {
   facebook::velox::registerFunction<
       facebook::velox::common::dynamicRegistry::DynamicFunction,
       int64_t,

velox/common/dynamic_registry/tests/DynamicErrFunction.cpp

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include "velox/common/dynamic_registry/DynamicUdf.h"
+#include "velox/functions/Udf.h"
 
 // This file defines a mock function that will be dynamically linked and
 // registered. There are no restrictions as to how the function needs to be
@@ -39,7 +39,7 @@ struct DynamicFunction {
 extern "C" {
 // In this case, we assume that facebook::velox::registerFunction
 // will be available and resolve when this library gets loaded.
-void registry() {
+void registerExtensions() {
   facebook::velox::registerFunction<
       facebook::velox::common::dynamicRegistry::DynamicFunction,
       int64_t>({"dynamic"});

velox/common/dynamic_registry/tests/DynamicFunction.cpp

@@ -0,0 +1,47 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "velox/functions/Udf.h"
+
+// This file defines a mock function that will be dynamically linked and
+// registered. There are no restrictions as to how the function needs to be
+// defined, but the library (.so) needs to provide a `void registry()` C
+// function in the top-level namespace.
+//
+// (note the extern "C" directive to prevent the compiler from mangling the
+// symbol name).
+
+namespace facebook::velox::common::dynamicRegistry {
+
+template <typename T>
+struct DynamicFunction {
+  FOLLY_ALWAYS_INLINE bool call(int64_t& result) {
+    result = 123;
+    return true;
+  }
+};
+
+} // namespace facebook::velox::common::dynamicRegistry
+
+extern "C" {
+// In this case, we assume that facebook::velox::registerFunction
+// will be available and resolve when this library gets loaded.
+void registerExtensionsNew() {
+  facebook::velox::registerFunction<
+      facebook::velox::common::dynamicRegistry::DynamicFunction,
+      int64_t>({"dynamic_non_default"});
+}
+}

velox/common/dynamic_registry/tests/DynamicFunctionNonDefault.cpp

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include "velox/common/dynamic_registry/DynamicUdf.h"
+#include "velox/functions/Udf.h"
 
 // This file defines a mock function that will be dynamically linked and
 // registered. There are no restrictions as to how the function needs to be
@@ -39,7 +39,7 @@ struct DynamicFunction {
 
 extern "C" {
 
-void registry() {
+void registerExtensions() {
   facebook::velox::registerFunction<
       facebook::velox::common::dynamicRegistry::DynamicFunction,
       int64_t,

velox/common/dynamic_registry/tests/DynamicIntFunctionOverload.cpp

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include "velox/common/dynamic_registry/DynamicUdf.h"
+#include "velox/functions/Udf.h"
 
 // This file defines a mock function that will be dynamically linked and
 // registered. There are no restrictions as to how the function needs to be
@@ -38,7 +38,7 @@ struct DynamicFunction {
 
 extern "C" {
 
-void registry() {
+void registerExtensions() {
   facebook::velox::registerFunction<
       facebook::velox::common::dynamicRegistry::DynamicFunction,
       int64_t>({"dynamic_overwrite"});

velox/common/dynamic_registry/tests/DynamicIntFunctionOverwrite.cpp

@@ -203,4 +203,40 @@ TEST_F(DynamicLinkTest, dynamicLoadOverloadFunc) {
   EXPECT_EQ(TypeKind::BIGINT, resolvedAfterSecond->type()->kind());
 }
 
+TEST_F(DynamicLinkTest, dynamicLoadFuncNonDefaultRegistry) {
+  const auto dynamicFunction = [&]() {
+    return evaluateOnce<int64_t>(
+        "dynamic_non_default()", makeRowVector(ROW({}), 1));
+  };
+
+  const auto dynamicFunctionNestedCall = [&]() {
+    return evaluateOnce<int64_t>(
+        "mod(dynamic_non_default(), 10)", makeRowVector(ROW({}), 1));
+  };
+
+  VELOX_ASSERT_THROW(
+      dynamicFunction(), "Scalar function doesn't exist: dynamic_non_default.");
+
+  auto signaturesBefore = getFunctionSignatures().size();
+  std::string libraryPath =
+      getLibraryPath("libvelox_function_non_default_dynamic");
+  loadDynamicLibrary(libraryPath, "registerExtensionsNew");
+  auto signaturesAfter = getFunctionSignatures().size();
+  EXPECT_EQ(signaturesAfter, signaturesBefore + 1);
+  EXPECT_EQ(123, dynamicFunction());
+
+  auto& registry = exec::simpleFunctions();
+  auto resolved = registry.resolveFunction("dynamic_non_default", {});
+  EXPECT_EQ(TypeKind::BIGINT, resolved->type()->kind());
+
+  EXPECT_EQ(3, dynamicFunctionNestedCall());
+
+  // Testing missing default registry function name.
+  VELOX_ASSERT_THROW(
+      loadDynamicLibrary(libraryPath),
+      fmt::format(
+          "Couldn't find Velox registry symbol: {}: undefined symbol: registerExtensions",
+          libraryPath));
+}
+
 } // namespace facebook::velox::functions::test