feat(functions): Add support for REST based remote functions #10911

Joe-Abraham · 2024-09-02T05:36:22Z

netlify · 2024-09-02T05:36:42Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`36f7abb`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/678dcdba923b010008ff11e8

pedroerp · 2024-09-09T15:19:16Z

Pretty cool! I see the PR is still as draft, but I can help review when it's ready. Would also be nice to add some documentation on how to use it, the configs parameters, etc.

Joe-Abraham · 2024-11-28T03:53:00Z

@aditi-pandit Can you please review the changes?

aditi-pandit · 2024-12-02T11:56:37Z

velox/functions/remote/client/Remote.h

-/// (non-remote) function registered with the same name. The `overwrite` flag
-/// controls whether to overwrite in these cases.
+/// (non-remote) function registered with the same name. The `overwrite`
+/// flagwrite controls whether to overwrite in these cases.


Nit : wording... maybe write is not needed here.

aditi-pandit · 2024-12-02T11:57:59Z

velox/functions/remote/client/Remote.cpp

 #include <folly/io/async/EventBase.h>
+#include <sstream>
+#include <string>
+#include "velox/common/memory/ByteStream.h"


Add empty line between the system and velox includes.

Added new line

aditi-pandit · 2024-12-02T11:58:34Z

velox/functions/remote/client/Remote.cpp

 #include "velox/vector/VectorStream.h"

+#include "velox/functions/remote/client/RestClient.h"


Move this include to the correct alphabetical order in the previous velox includes.

corrected the inclusion list

aditi-pandit · 2024-12-02T12:00:03Z

velox/functions/remote/client/Remote.h

-  /// Network address of the servr to communicate with. Note that this can hold
-  /// a network location (ip/port pair) or a unix domain socket path (see
+  /// URL of the HTTP/REST server for remote function.
+  /// Or Network address of the servr to communicate with. Note that this can


Nit : spelling "server"

Corrected it

aditi-pandit · 2024-12-02T14:51:37Z

velox/functions/remote/client/RestClient.cpp

+}
+size_t writeCallback(char* ptr, size_t size, size_t nmemb, void* userdata) {
+  auto* outputBuf = static_cast<IOBufQueue*>(userdata);
+  size_t total_size = size * nmemb;


nit : Use camelCase naming -> totalSize

Corrected it

aditi-pandit · 2024-12-02T14:52:31Z

velox/functions/remote/client/RestClient.cpp

+  return totalCopied;
+}
+size_t writeCallback(char* ptr, size_t size, size_t nmemb, void* userdata) {
+  auto* outputBuf = static_cast<IOBufQueue*>(userdata);


Use camel case "userData"

Corrected it

aditi-pandit · 2024-12-02T14:53:53Z

velox/functions/remote/client/RestClient.cpp

+using namespace folly;
+namespace facebook::velox::functions {
+namespace {
+size_t readCallback(char* dest, size_t size, size_t nmemb, void* userp) {


Please write comments explaining the signature and the parameters.

Added the documentation

aditi-pandit · 2024-12-02T14:54:45Z

velox/functions/remote/client/RestClient.h

+
+class RestClient : public HttpClient {
+ public:
+  std::unique_ptr<folly::IOBuf> performCurlRequest(


Please write the documentation for this API. What is it for ? What do the parameters mean ?

Added the documentation

aditi-pandit · 2024-12-02T15:07:22Z

velox/functions/remote/server/RemoteFunctionRestService.h

+      memory::memoryManager()->addLeafPool()};
+};
+
+class listener : public std::enable_shared_from_this<listener> {


Can you add some documentation about these classes and what are they for ?

Added documentation

aditi-pandit · 2024-12-02T15:19:42Z

velox/functions/remote/server/RemoteFunctionRestService.cpp

+        // called to use the functions mentioned in this map
+};
+
+TypePtr deserializeType(const std::string& input) {


There is a lot of repetition between this code and https://github.com/facebookincubator/velox/blob/main/velox/functions/remote/server/RemoteFunctionService.cpp. Please can you refactor.

introduced the RemoteFunctionHelper.h and moved the duplicate code.

assignUser · 2025-01-06T22:38:07Z

velox/functions/remote/client/CMakeLists.txt

@@ -16,11 +16,23 @@ velox_add_library(velox_functions_remote_thrift_client ThriftClient.cpp)
 velox_link_libraries(velox_functions_remote_thrift_client
                     PUBLIC remote_function_thrift FBThrift::thriftcpp2)

+set(curl_SOURCE BUNDLED)
+velox_resolve_dependency(curl)


This doesn't actually build curl. We only use it to force cpr to use the version we want. So if you require curl I would say use set_source to allow system curl to be used (this might require changes to cpr.cmake but iirc cpr can work with system curl as well).

Also move this into the root cml within an if().

assignUser · 2025-01-06T22:39:29Z

velox/functions/remote/client/CMakeLists.txt

+
+velox_add_library(velox_functions_remote_rest_client RestClient.cpp)
+velox_link_libraries(velox_functions_remote_rest_client Folly::folly
+                     ${CURL_LIBRARIES})


Suggested change

${CURL_LIBRARIES})

CURL::libcurl)

Always prefer targets over variables.

assignUser · 2025-01-07T16:38:51Z

CMakeLists.txt

+  velox_set_source(curl)
+  velox_resolve_dependency(curl)


Thanks. If you now try a build without testing you will see that curl is not installed.
But it should be enough to add FetchContent_MakeAvailable(curl) to curl.cmake. This might interact weirdly with cpr trying it's own curl build but you'll just have to test that (I recommend make clean before).

Thank you @assignUser for the review points and I have made the necessary changes.

I am bit confused where to add FetchContent_MakeAvailable(curl). Can you please suggest the change?

@Joe-Abraham : @assignUser has suggested to add it to curl.cmake

aditi-pandit

@Joe-Abraham : A quick round of comments. I have to look at the server/ files in more detail still.

aditi-pandit · 2025-01-14T22:13:51Z

velox/functions/remote/client/tests/RemoteFunctionRestTest.cpp

+#include <folly/init/Init.h>
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+#include <cstdio>


This header should move to the top as it is a standard C library.

aditi-pandit · 2025-01-15T19:57:12Z

CMakeLists.txt

+  velox_set_source(curl)
+  velox_resolve_dependency(curl)


@Joe-Abraham : @assignUser has suggested to add it to curl.cmake

aditi-pandit · 2025-01-15T20:00:49Z

velox/functions/remote/client/Remote.cpp

 class RemoteFunction : public exec::VectorFunction {
 public:
  RemoteFunction(
      const std::string& functionName,
      const std::vector<exec::VectorFunctionArg>& inputArgs,
-      const RemoteVectorFunctionMetadata& metadata)
+      const RemoteVectorFunctionMetadata& metadata,
+      std::unique_ptr<HttpClient> httpClient = nullptr)


Why do we prefer to pass an HttpClient here ? For thrift, a ThriftClient is created per RemoteFunction. Might be better to do so for HttpClient as well. You have an eventBase_ to work with as well.

aditi-pandit · 2025-01-15T20:06:41Z

velox/functions/remote/client/Remote.cpp

-        serde_(getSerde(serdeFormat_)) {
+        restClient_(httpClient ? std::move(httpClient) : getRestClient()),
+        metadata_(metadata) {
+    if (metadata.location.type() == typeid(SocketAddress)) {


You can have a member variable of enum for RemoteType = {REST, HTTP}, and set it in the constructor and use it in the apply instead of doing the type check each time.

aditi-pandit · 2025-01-15T20:12:25Z

velox/functions/remote/client/Remote.cpp

  std::unique_ptr<RemoteFunctionClient> thriftClient_;
-  remote::PageFormat serdeFormat_;


Why did you remove the serdeFormat_ and serde_ member variables ? It was better to construct them once in the constructor and use them in the function apply code, instead of call getSerde each time.

aditi-pandit · 2025-01-15T20:24:38Z

velox/functions/remote/client/RestClient.cpp

+  IOBufQueue inputBufQueue(IOBufQueue::cacheChainLength());
+  inputBufQueue.append(std::move(requestPayload));
+
+  CURL* curl = curl_easy_init();


Does this initialization need to be done each time invokeFunction is called ? Can these be member variables initialized in the constructor ?

aditi-pandit · 2025-01-15T20:30:56Z

velox/functions/remote/client/tests/RemoteFunctionRestTest.cpp

+  }
+}
+
+VELOX_INSTANTIATE_TEST_SUITE_P(


Would be better to enhance RemoteFunctionTest fixtures to do both Rest and Thrift Server testing. You could use Rest/Thrift as a parameterization to TEST_SUITE.

aditi-pandit · 2025-01-15T20:35:10Z

velox/functions/remote/server/RemoteFunctionServiceMain.cpp

  // Always registers all Presto functions and make them available under a
  // certain prefix/namespace.
  LOG(INFO) << "Registering Presto functions";
  functions::prestosql::registerAllScalarFunctions(FLAGS_function_prefix);

+  std::remove(FLAGS_uds_path.c_str());


Don't understand this change.

aditi-pandit · 2025-01-15T20:36:30Z

velox/functions/remote/server/RemoteFunctionServiceMain.cpp

@@ -18,6 +18,7 @@
 #include <gflags/gflags.h>
 #include <glog/logging.h>
 #include <thrift/lib/cpp2/server/ThriftServer.h>
+#include "velox/functions/prestosql/StringFunctions.h"


Why is this needed ?

aditi-pandit · 2025-01-15T20:38:14Z

velox/functions/remote/server/RemoteFunctionServiceMain.cpp

Might be good to separate the changes in this file into a separate PR. Though the only change needed here really is the memory::initializeMemoryManager({}); line.

I'll send out a PR.

Co-authored-by: Wills Feng <[email protected]>

assignUser · 2025-01-21T03:16:37Z

CMakeLists.txt

+  set(cpr_SOURCE BUNDLED)
+  velox_resolve_dependency(cpr)


#10911 (comment)

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 Sep 2, 2024

Joe-Abraham changed the title ~~Add support for REST based remote functions~~ [WIP] Add support for REST based remote functions Sep 2, 2024

Joe-Abraham force-pushed the udf branch 12 times, most recently from b88d136 to b85e0e6 Compare September 4, 2024 09:32

Yuhta requested review from pedroerp and mbasmanova September 4, 2024 18:28

Joe-Abraham force-pushed the udf branch 6 times, most recently from 0cd4510 to 74023dc Compare September 9, 2024 08:10

Joe-Abraham force-pushed the udf branch 3 times, most recently from abe87e1 to 6c1606e Compare September 13, 2024 05:06

Joe-Abraham force-pushed the udf branch 3 times, most recently from 05115f4 to 2ffec26 Compare September 20, 2024 05:18

Joe-Abraham marked this pull request as ready for review November 28, 2024 03:52

Joe-Abraham requested review from assignUser and majetideepak as code owners November 28, 2024 03:52

Joe-Abraham mentioned this pull request Nov 28, 2024

[native] Add support for REST based remote function prestodb/presto#23568

Open

aditi-pandit reviewed Dec 2, 2024

View reviewed changes

Joe-Abraham force-pushed the udf branch from 9d0787d to 37e6d84 Compare December 3, 2024 08:57

Joe-Abraham requested a review from aditi-pandit December 3, 2024 10:17

Joe-Abraham force-pushed the udf branch from 37e6d84 to 8278ff8 Compare December 3, 2024 10:17

Joe-Abraham force-pushed the udf branch from 8278ff8 to 97483aa Compare December 13, 2024 08:54

Joe-Abraham force-pushed the udf branch 2 times, most recently from 394cc7f to 98a6281 Compare January 3, 2025 04:58

assignUser reviewed Jan 6, 2025

View reviewed changes

Joe-Abraham force-pushed the udf branch 2 times, most recently from d031069 to c0dd88d Compare January 7, 2025 05:58

assignUser reviewed Jan 7, 2025

View reviewed changes

Joe-Abraham marked this pull request as draft January 9, 2025 04:44

Joe-Abraham force-pushed the udf branch 3 times, most recently from a4a213f to 8da88f3 Compare January 13, 2025 04:43

Joe-Abraham marked this pull request as ready for review January 13, 2025 04:49

Joe-Abraham requested a review from assignUser January 13, 2025 13:36

Joe-Abraham force-pushed the udf branch from 8da88f3 to 4df8be1 Compare January 15, 2025 05:05

aditi-pandit reviewed Jan 15, 2025

View reviewed changes

Joe-Abraham force-pushed the udf branch from 4df8be1 to e747d4a Compare January 17, 2025 04:53

Joe-Abraham and others added 3 commits January 20, 2025 09:44

feat(functions): Add support for REST based remote functions

ef15bbb

Co-authored-by: Wills Feng <[email protected]>

Change curl to cpr

454d19b

Fix review comments

36f7abb

Joe-Abraham force-pushed the udf branch from 0efa665 to 36f7abb Compare January 20, 2025 04:14

assignUser requested changes Jan 21, 2025

View reviewed changes

CMakeLists.txt

Comment on lines +524 to +525

set(cpr_SOURCE BUNDLED)

velox_resolve_dependency(cpr)

Copy link

Collaborator

assignUser Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#10911 (comment)

		#include "velox/vector/VectorStream.h"

		#include "velox/functions/remote/client/RestClient.h"

		std::unique_ptr<RemoteFunctionClient> thriftClient_;
		remote::PageFormat serdeFormat_;

feat(functions): Add support for REST based remote functions #10911

Are you sure you want to change the base?

feat(functions): Add support for REST based remote functions #10911

Conversation

Joe-Abraham commented Sep 2, 2024 • edited Loading

netlify bot commented Sep 2, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

pedroerp commented Sep 9, 2024

Joe-Abraham commented Nov 28, 2024

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

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

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

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

Joe-Abraham commented Sep 2, 2024 •

edited

Loading

netlify bot commented Sep 2, 2024 •

edited

Loading