feat: Add support for registering components dynamically by soumiiow · Pull Request #11439 · facebookincubator/velox

soumiiow · 2024-11-05T13:11:52Z

Allow users to dynamically register Velox components. Clients such as Presto can use this feature to dynamically load User Defined Functions, connectors, and types.

Based off: https://github.com/facebookincubator/velox/pull/1005/files

netlify · 2024-11-05T13:12:12Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`542dd28`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67ae9129d5bcfe000887fb9c

pedroerp · 2024-11-05T15:50:57Z

@soumiiow thanks for looking into this. Out of curiosity, why doesn't this work in MacOS?

pedroerp · 2024-11-05T15:52:59Z

velox/common/CMakeLists.txt

we use snake case for directory names "dynamic_registry"

pedroerp

Very cool! I few small comments but overall looks good.

pedroerp · 2024-11-05T15:53:27Z

velox/common/dynamicRegistry/DynamicLibraryLoader.cpp

nit: new line before namespace definition.

pedroerp · 2024-11-05T15:55:15Z

velox/common/dynamicRegistry/DynamicLibraryLoader.cpp

for consistency, could you use LOG(INFO) and print the file name / path of the library loaded?

pedroerp · 2024-11-05T15:56:10Z

velox/common/dynamicRegistry/DynamicLibraryLoader.cpp

we can probably omit the "Functions" from the name, and this can be used to really load anything, as long as you provide the registration functions. Let's name it loadDynamicLibrary()

pedroerp · 2024-11-05T15:57:02Z

velox/common/dynamicRegistry/README.md

nit: the titles are too long; maybe just add the docs as a refular numbered list?

I pushed it out without the title formatting but does this look a bit cluttered now?

pedroerp · 2024-11-05T15:58:20Z

velox/common/dynamicRegistry/tests/DynamicLinkTest.cpp

can you use VELOX_ASSERT_THROW() instead to validate the right exception is being thrown?

pedroerp · 2024-11-05T15:59:22Z

velox/common/dynamicRegistry/tests/CMakeLists.txt

please vendor the macro. Maybe something like VELOX_TEST_DYNAMIC_LIBRARY_PATH

velox/common/dynamicRegistry/DynamicLibraryLoader.cpp

velox/common/dynamicRegistry/tests/DynamicLinkTest.cpp

assignUser · 2024-11-12T19:54:41Z

velox/common/dynamic_registry/tests/CMakeLists.txt

please use the GTest:: targets

assignUser · 2024-11-12T19:56:46Z

velox/common/dynamic_registry/tests/CMakeLists.txt

Please use target_compile_definitions( on the relevant target instead.

assignUser · 2024-11-12T19:58:15Z

velox/common/dynamic_registry/CMakeLists.txt

Suggested change

velox_add_library(velox_dynamic_function_loader DynamicLibraryLoader.cpp)

velox_add_library(velox_dynamic_function_loader DynamicLibraryLoader.cpp)

velox_link_libraries(velox_dynamic_function_loader PRIVATE velox_exception)

velox/common/dynamic_registry/README.md

velox/common/dynamic_registry/tests/CMakeLists.txt

velox/common/dynamic_registry/tests/MyDynamicFunction.cpp

velox/common/dynamic_registry/README.md

velox/common/dynamic_registry/DynamicLibraryLoader.h

velox/common/dynamic_registry/DynamicLibraryLoader.cpp

aditi-pandit

Thanks @soumiiow. Had bunch of minor comments, except for a bigger one around testing.

velox/common/dynamic_registry/CMakeLists.txt

aditi-pandit · 2024-11-13T05:35:23Z

velox/common/dynamic_registry/DynamicLibraryLoader.cpp

Nit : rename registryFunction

hey!! so i got some previous feedback to stay away from the "registryFunction" in the naming so as to not make it seem like this library is to be used exclusively for functions, and to move away from our initial design which was made with only the function loading in mind. Perhaps, would there be a better name for this variable than the work "item"? I can only rlly think of registryItem or registryPtr but would love to hear your suggestions too

@soumiiow : To me this is almost like the "main" function in a executable program. How about "loadLibrary" or "loadUserLibrary" or "enterUserLibrary" ? There could be code beyond registration here as well.

aditi-pandit · 2024-11-13T05:36:07Z

velox/common/dynamic_registry/DynamicLibraryLoader.cpp

Add a comment "Invoke the registry function"

velox/common/dynamic_registry/tests/MyDynamicFunction.cpp

velox/common/dynamic_registry/tests/DynamicLinkTest.cpp

velox/common/dynamic_registry/tests/MyDynamicFunction.cpp

aditi-pandit · 2024-11-13T06:07:54Z

velox/common/dynamic_registry/README.md

This is not relevant in Velox. And also since its not used anywhere in the current code, its hard to put this in picture.

aditi-pandit · 2024-11-13T06:18:03Z

velox/common/dynamic_registry/README.md

It might be good to not talk about Prestissimo in this README.

This is a generic utility for dynamically loading a "registry" function from a library. Its sufficient to just say that this is for "Extensibility" features that add custom user code which could include new Velox types, functions, operators and connectors.

aditi-pandit

@soumiiow : Thanks for updating your tests. Have a bunch of questions.

velox/common/dynamic_registry/DynamicLibraryLoader.h

velox/common/dynamic_registry/DynamicLibraryLoader.cpp

velox/common/dynamic_registry/CMakeLists.txt

velox/common/dynamic_registry/tests/DynamicLinkTest.cpp

mohsaka · 2025-01-23T01:03:23Z

@soumiiow Addressed all of the unaddressed comments and made all of the changes for compilation on mac in this PR
#12111

Specifically this commit,
ac18115

pedroerp

That's awesome. Thanks for adding support for this.

Made a few small comments, but overall it looks good to me. Feel free to tag "ready-to-merge" when they are addressed and I'll get it merged.

pedroerp · 2025-02-12T00:33:49Z

velox/common/dynamic_registry/DynamicLibraryLoader.h

nit: remove this extra line

pedroerp · 2025-02-12T00:35:12Z

velox/common/dynamic_registry/DynamicLibraryLoader.cpp

LOG(INFO) << "Loaded library " << fileName << ". Searching registry symbol " << kSymbolName;

pedroerp · 2025-02-12T00:36:40Z

velox/common/dynamic_registry/DynamicLibraryLoader.cpp

do we also need to check if loadUserLibrary is nullptr?

Added, as well as a short comment explaining how it can be nullptr but we can't actually use it when its nullptr.

pedroerp · 2025-02-12T00:42:54Z

velox/docs/develop/dynamic-loading.rst

Can we also clarify that this only works if the builds are guaranteed to be based on the same Velox version and hence maintain ABI compatibility? This might not be immediate clear to users.

Added. Thanks!

majetideepak · 2025-02-12T18:39:57Z

CI is green. @pedroerp can you please help import and merge? Thank you.

…uccessful mac & linux compilation Co-authored-by: mohsaka <michael.ohsaka@ibm.com>

facebook-github-bot · 2025-02-14T02:16:56Z

@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

czentgr · 2025-02-14T14:26:35Z

@pedroerp the facebook builds&tools failed with a warning and the linter also reported a testing failure. Is this something that needs to be addressed in this PR? Thanks!

mohsaka · 2025-02-14T19:37:41Z

@pedroerp the facebook builds&tools failed with a warning and the linter also reported a testing failure. Is this something that needs to be addressed in this PR? Thanks!

@czentgr It's been fixed on the Meta side already. The README needed a newline at the end of the file. If we need the change here, I can make it. But I believe they merge over there and close the PR here, merging back to main here.

I don't want to trigger a full pipeline rerun due to this.

facebook-github-bot · 2025-02-15T01:53:11Z

@pedroerp merged this pull request in be9de86.

soumiiow requested a review from majetideepak as a code owner November 5, 2024 13:11

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 5, 2024

Yuhta requested a review from pedroerp November 5, 2024 15:40

pedroerp reviewed Nov 5, 2024

View reviewed changes

majetideepak reviewed Nov 5, 2024

View reviewed changes

velox/common/dynamicRegistry/DynamicLibraryLoader.cpp Outdated Show resolved Hide resolved

majetideepak reviewed Nov 5, 2024

View reviewed changes

velox/common/dynamicRegistry/tests/DynamicLinkTest.cpp Outdated Show resolved Hide resolved

soumiiow force-pushed the velox-dylib branch from 789da39 to c5cbea2 Compare November 12, 2024 15:42

soumiiow requested a review from assignUser as a code owner November 12, 2024 15:42

soumiiow force-pushed the velox-dylib branch from c5cbea2 to f8afbc0 Compare November 12, 2024 15:51

soumiiow requested review from majetideepak and pedroerp November 12, 2024 16:59

assignUser reviewed Nov 12, 2024

View reviewed changes

majetideepak reviewed Nov 12, 2024

View reviewed changes

aditi-pandit reviewed Nov 13, 2024

View reviewed changes

soumiiow force-pushed the velox-dylib branch 2 times, most recently from 45cf7af to 4e82b71 Compare December 4, 2024 07:04

soumiiow force-pushed the velox-dylib branch from 4e82b71 to ae2ad2e Compare December 17, 2024 13:51

soumiiow force-pushed the velox-dylib branch 2 times, most recently from b6660ad to eb20996 Compare January 7, 2025 16:55

soumiiow mentioned this pull request Jan 7, 2025

[native] Dynamically Linked Library in Presto CPP prestodb/presto#24330

Merged

6 tasks

aditi-pandit reviewed Jan 7, 2025

View reviewed changes

soumiiow mentioned this pull request Jan 14, 2025

presto requires namespaces when using function registration prestodb/presto#24363

Open

mohsaka mentioned this pull request Jan 17, 2025

[WIP] MacOS CMake compatibility for dynamic functions #12111

Closed

soumiiow force-pushed the velox-dylib branch from eb20996 to d0b34fc Compare January 25, 2025 02:12

soumiiow changed the title ~~Dynamically Linked Library in CPP~~ feat: Dynamically Linked Library in CPP Jan 27, 2025

soumiiow force-pushed the velox-dylib branch from d0b34fc to ff3bd9c Compare January 27, 2025 22:11

mohsaka force-pushed the velox-dylib branch 4 times, most recently from e06974a to 3bbd8db Compare February 12, 2025 00:13

pedroerp approved these changes Feb 12, 2025

View reviewed changes

mohsaka force-pushed the velox-dylib branch 7 times, most recently from 2dbe57e to 2ccbceb Compare February 12, 2025 16:54

majetideepak approved these changes Feb 12, 2025

View reviewed changes

mohsaka approved these changes Feb 12, 2025

View reviewed changes

czentgr approved these changes Feb 12, 2025

View reviewed changes

majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Feb 12, 2025

majetideepak changed the title ~~feat: Dynamically Linked Library in CPP~~ feat: Add support for loading shared libraries Feb 12, 2025

majetideepak changed the title ~~feat: Add support for loading shared libraries~~ feat: Add support for registering components dynamically Feb 12, 2025

dylib functionality + namespace wrapper and other review comments + s…

542dd28

…uccessful mac & linux compilation Co-authored-by: mohsaka <michael.ohsaka@ibm.com>

mohsaka force-pushed the velox-dylib branch from 2ccbceb to 542dd28 Compare February 14, 2025 00:41

facebook-github-bot closed this in be9de86 Feb 15, 2025

facebook-github-bot added the Merged label Feb 15, 2025

aditi-pandit mentioned this pull request Feb 19, 2025

[native] Advance Velox prestodb/presto#24595

Merged

majetideepak mentioned this pull request Mar 3, 2025

misc: Revert GlobalConfig to use gflags again #12503

Closed

	velox_add_library(velox_dynamic_function_loader DynamicLibraryLoader.cpp)
	velox_add_library(velox_dynamic_function_loader DynamicLibraryLoader.cpp)
	velox_link_libraries(velox_dynamic_function_loader PRIVATE velox_exception)

Conversation

soumiiow commented Nov 5, 2024 • edited by majetideepak Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

pedroerp commented Nov 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedroerp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aditi-pandit left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soumiiow commented Nov 5, 2024 •

edited by majetideepak

Loading

netlify bot commented Nov 5, 2024 •

edited

Loading

aditi-pandit left a comment •

edited

Loading

mohsaka commented Feb 14, 2025 •

edited

Loading