Address comments

mohsaka · Jan 31, 2025 · 1d75353 · 1d75353
1 parent 225d439
commit 1d75353
Show file tree

Hide file tree

Showing 8 changed files with 38 additions and 14 deletions.
diff --git a/velox/common/dynamic_registry/README.md b/velox/common/dynamic_registry/README.md
@@ -4,7 +4,11 @@ This generic utility adds extensibility features to load User Defined Functions
 
 ## 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 [MyDynamicTestFunction.cpp](tests/MyDynamicTestFunction.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 followed is mirrored of that of built-in function registration with some noted differences. Using [MyDynamicTestFunction.cpp](tests/MyDynamicTestFunction.cpp) as an example, the function uses the extern "C" keyword to protect against name mangling. A registry() function call is also necessary here. 
+Make sure to also include the necessary header file:
+    ```
+    #include "velox/functions/DynamicUdf.h"
+    ```
 
 2. Register functions dynamically by creating .dylib (MacOS) or .so (Linux) shared libraries.
 These shared libraries may be made using CMakeLists like the following:
@@ -19,7 +23,7 @@ These shared libraries may be made using CMakeLists like the following:
 * MacOS:
     ```
     add_library(name_of_dynamic_fn SHARED TestFunction.cpp)
-    target_link_libraries(name_of_dynamic_fn PRIVATE fmt::fmt Folly::folly gflags::gflags xsimd)
+    target_link_libraries(name_of_dynamic_fn PRIVATE Folly::folly xsimd)
     ``` 
 
     Additionally, the below flag allows symbols to be resolved at runtime:

diff --git a/velox/common/dynamic_registry/tests/MyDynamicErrFunction.cpp b/velox/common/dynamic_registry/tests/MyDynamicErrFunction.cpp
@@ -13,8 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#include "velox/functions/Macros.h"
-#include "velox/functions/Registerer.h"
+#include "velox/functions/DynamicUdf.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

diff --git a/velox/common/dynamic_registry/tests/MyDynamicFunction.cpp b/velox/common/dynamic_registry/tests/MyDynamicFunction.cpp
@@ -13,8 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#include "velox/functions/Macros.h"
-#include "velox/functions/Registerer.h"
+#include "velox/functions/DynamicUdf.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

diff --git a/velox/common/dynamic_registry/tests/MyDynamicIntFunctionOverload.cpp b/velox/common/dynamic_registry/tests/MyDynamicIntFunctionOverload.cpp
@@ -13,8 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#include "velox/functions/Macros.h"
-#include "velox/functions/Registerer.h"
+#include "velox/functions/DynamicUdf.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

diff --git a/velox/common/dynamic_registry/tests/MyDynamicIntFunctionOverwrite.cpp b/velox/common/dynamic_registry/tests/MyDynamicIntFunctionOverwrite.cpp
@@ -13,8 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#include "velox/functions/Macros.h"
-#include "velox/functions/Registerer.h"
+#include "velox/functions/DynamicUdf.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

diff --git a/velox/common/dynamic_registry/tests/MyDynamicVarcharFunctionOverload.cpp b/velox/common/dynamic_registry/tests/MyDynamicVarcharFunctionOverload.cpp
@@ -13,8 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#include "velox/functions/Macros.h"
-#include "velox/functions/Registerer.h"
+#include "velox/functions/DynamicUdf.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

diff --git a/velox/common/dynamic_registry/tests/MyDynamicVarcharFunctionOverwrite.cpp b/velox/common/dynamic_registry/tests/MyDynamicVarcharFunctionOverwrite.cpp
@@ -13,8 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#include "velox/functions/Macros.h"
-#include "velox/functions/Registerer.h"
+#include "velox/functions/DynamicUdf.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

diff --git a/velox/functions/DynamicUdf.h b/velox/functions/DynamicUdf.h
@@ -0,0 +1,26 @@
+/*
+ * 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.
+ */
+#pragma once
+
+#include "velox/functions/Udf.h"
+
+extern "C"{
+void registry();
+void check(){
+  registry();
+}
+}
+