Implement REST API for remote functions #23687

abevk2023 · 2024-09-20T07:24:35Z

Add new function namespace manager and execution module for REST-based remote functions.
Add OpenAPI specification which defines a REST API for remote function servers.
Add REST API reference implementation for the integration tests.

== NO RELEASE NOTE ==

jkhaliqi · 2024-10-09T16:38:37Z

presto-main/pom.xml

@@ -436,7 +436,6 @@
        <dependency>
            <groupId>com.facebook.presto</groupId>
            <artifactId>presto-thrift-api</artifactId>
-            <scope>test</scope>


nit: lets move this above line 384 so it's out of the testing section

Thank you Jacob. Moved

jkhaliqi · 2024-10-09T16:38:49Z

presto-main/pom.xml

@@ -466,7 +465,6 @@
        <dependency>
            <groupId>com.facebook.presto</groupId>
            <artifactId>presto-function-namespace-managers</artifactId>
-            <scope>test</scope>


nit: lets move this above line 384 so it's out of the testing section

tdcmeehan

Add namespace manager to retrieve REST function signatures. ❓

tdcmeehan · 2024-10-24T17:21:08Z

...managers/src/main/java/com/facebook/presto/functionNamespace/rest/RestBasedFunctionApis.java

+                    .setUri(uri)
+                    .build();
+
+            StatusResponseHandler.StatusResponse response = httpClient.execute(request, StatusResponseHandler.createStatusResponseHandler());


Please static import createStatusResponseHandler

...managers/src/main/java/com/facebook/presto/functionNamespace/rest/RestBasedFunctionApis.java

tdcmeehan · 2024-10-24T17:29:18Z

...managers/src/main/java/com/facebook/presto/functionNamespace/rest/RestBasedFunctionApis.java

+            }
+            return version;
+        }
+        catch (Exception e) {


If it's an IOException, let it bubble up.

It is not. Removed the exception for missing ETag.

tdcmeehan · 2024-10-24T17:39:54Z

.../main/java/com/facebook/presto/functionNamespace/rest/RestBasedFunctionNamespaceManager.java

+        latestFunctions.clear();
+        UdfFunctionSignatureMap udfFunctionSignatureMap = restApis.getAllFunctions();
+        if (udfFunctionSignatureMap == null || udfFunctionSignatureMap.isEmpty()) {
+            return Collections.emptyList();


ImmutableList.of()

tdcmeehan · 2024-10-24T20:39:49Z

...java/com/facebook/presto/functionNamespace/rest/RestBasedFunctionNamespaceManagerConfig.java

+
+public class RestBasedFunctionNamespaceManagerConfig
+{
+    private String restUrl = "";


Let's make the default null, to indicate it's a mandatory config.

tdcmeehan · 2024-10-24T20:40:14Z

...c/test/java/com/facebook/presto/functionNamespace/TestRestBasedFunctionNamespaceManager.java

+    private RestBasedFunctionNamespaceManager functionNamespaceManager;
+    public static final String TEST_CATALOG = "unittest";
+    public static final URI REST_SERVER_URI = URI.create("http://127.0.0.1:1122");
+    TestingFunctionResource resource;


Missing visibility.

tdcmeehan

Introduce REST based Function Namespace Manager

tdcmeehan · 2024-11-01T20:13:03Z

...managers/src/main/java/com/facebook/presto/functionNamespace/rest/RestBasedFunctionApis.java

+        this.functionSignatureMapJsonCodec = nativeFunctionSignatureMapJsonCodec;
+        this.httpClient = httpClient;
+        this.managerConfig = managerConfig;


Please add requireNonNull for all inputs.

tdcmeehan · 2024-11-01T20:21:59Z

.../main/java/com/facebook/presto/functionNamespace/rest/RestBasedFunctionNamespaceManager.java

+    private static final Logger log = Logger.get(RestBasedFunctionNamespaceManager.class);
+    private final RestBasedFunctionApis restApis;
+    private final List<SqlInvokedFunction> latestFunctions = new ArrayList<>();
+    private Optional<String> cachedETag = Optional.empty();


I think this needs to be an AtomicReference, since it's updated concurrently.

Corrected. Used AtomicReference.

tdcmeehan · 2024-12-10T16:26:19Z

...nagers/src/main/java/com/facebook/presto/functionNamespace/rest/RestSqlFunctionExecutor.java

+
+    private URI getExecutionEndpoint(SqlFunctionId functionId, String functionVersion)
+    {
+        if (restBasedFunctionNamespaceManagerConfig.getRestUrl() == null) {


Because the getter is annotated with @NotNull, I believe this is pre-validated and you can skip this.

tdcmeehan · 2024-12-10T16:26:51Z

...nagers/src/main/java/com/facebook/presto/functionNamespace/rest/RestSqlFunctionExecutor.java

+        @Override
+        public SqlFunctionResult handle(Request request, Response response)
+        {
+            if (response.getStatusCode() == 404) {


Search the codebase for constants representing these HTTP error codes

Used HTTP error codes in java.net.

tdcmeehan · 2024-12-17T17:16:59Z

...managers/src/main/java/com/facebook/presto/functionNamespace/rest/RestBasedFunctionApis.java

+    {
+        this.functionSignatureMapJsonCodec = requireNonNull(nativeFunctionSignatureMapJsonCodec, "nativeFunctionSignatureMapJsonCodec is null");
+        this.httpClient = requireNonNull(httpClient, "httpClient is null");
+        this.managerConfig = requireNonNull(managerConfig, "httpClient is null");


Suggested change

this.managerConfig = requireNonNull(managerConfig, "httpClient is null");

this.managerConfig = requireNonNull(managerConfig, "managerConfig is null");

Removed managerConfig

.../main/java/com/facebook/presto/functionNamespace/rest/RestBasedFunctionNamespaceManager.java

tdcmeehan · 2024-12-17T17:22:07Z

.../main/java/com/facebook/presto/functionNamespace/rest/RestBasedFunctionNamespaceManager.java

+        return latestFunctions;
+    }
+
+    private void createSqlInvokedFunctions(UdfFunctionSignatureMap udfFunctionSignatureMap, List<SqlInvokedFunction> functionList)


Can you have this return the list of functions, and merge the output caller side? Rather than mutating the funcionList?

Corrected. Returned list of functions.

...nagers/src/main/java/com/facebook/presto/functionNamespace/rest/RestSqlFunctionExecutor.java

tdcmeehan · 2024-12-17T17:28:56Z

presto-main/pom.xml

+                        <configuration>
+                            <finalName>presto-function-server-executable</finalName>
+                            <transformers>
+                                <transformer implementation="org.apache.maven.plugins.shade.resource.ManifestResourceTransformer">


Why is this needed?

Reference function server is package as presto-function-server-executable and executed in parallel to presto server launcher. Could you please refer the PR for e2e testing https://github.com/prestodb/presto/pull/23777/files#diff-4f5cabe26761257a4d685a6edc7a43e0fe0f78762f50eeb48530f2bd3b3ee7caR5

Removed. We will add function server executable in subsequent PR.

tdcmeehan · 2025-01-22T21:33:31Z

presto-spark-base/pom.xml

@@ -265,7 +265,6 @@
        <dependency>
            <groupId>com.facebook.presto</groupId>
            <artifactId>presto-function-namespace-managers</artifactId>
-            <scope>test</scope>


Is this intentionally removed?

Reverted. No required now, as FunctionServer is moved to new maven module presto-function-server. This change was done to use presto-function-namespace-managers in thepresto-main for the FunctionServer.

tdcmeehan · 2025-01-22T21:35:32Z

presto-function-namespace-managers/pom.xml

+            <artifactId>jackson-datatype-jdk8</artifactId>
+        </dependency>
+
+        <dependency>


Do we need this dependency?

Yes. It is required for presto-function-namespace-managers. Throws undeclared dependency error if removed.

[INFO] --- maven-dependency-plugin:3.1.1:analyze-only (default) @ presto-function-namespace-managers --- [WARNING] Used undeclared dependencies found: [WARNING] com.facebook.drift:drift-transport-netty:jar:1.40:compile

tdcmeehan · 2025-01-27T17:04:55Z

presto-function-server/pom.xml

+                <executions>
+                    <execution>
+                        <id>modernizer</id>
+                        <phase>none</phase> <!-- This disables the plugin -->


Do we need to disable it?

Enabled and reused existing src/modernizer/violations.xml. Previously disabled due to missing file.

tdcmeehan · 2025-01-27T17:07:29Z

presto-spi/src/main/java/com/facebook/presto/spi/function/SqlInvokedFunction.java

@@ -136,7 +136,39 @@ public SqlInvokedFunction(
            throw new IllegalArgumentException("aggregationMetadata must be present for aggregation functions and absent otherwise");
        }
    }
+    public SqlInvokedFunction(


This looks mostly copied from the constructor above. Can they be consolidated?

Corrected. Refactored to consolidate shared logic.

Stale

czentgr · 2025-01-29T20:34:36Z

presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.h

@@ -1509,6 +1509,8 @@ struct JsonBasedUdfFunctionMetadata {
  String schema = {};
  RoutineCharacteristics routineCharacteristics = {};
  std::shared_ptr<AggregationFunctionMetadata> aggregateMetadata = {};
+  std::shared_ptr<SqlFunctionId> functionId = {};


Quick question, did you create the h and cpp files by running the generator or did you manually add these lines?

@abevk2023 : Did you follow the instructions at https://github.com/prestodb/presto/tree/master/presto-native-execution/presto_cpp/presto_protocol#presto-native-worker-protocol-code-generation to generate this code ?

Yes. I have followed the same instructions to generate the code.

Co-authored-by: Jacob Khaliqi <[email protected]>

tdcmeehan · 2025-01-30T14:16:19Z

It seems the [prestocpp-linux-build-engine (pull_request)](https://github.com/prestodb/presto/actions/runs/13049563626/job/36418195012?pr=23687) job has been very unlucky for this PR and consistently chooses a runner which has only 13GB of space. This causes it to run out of space.

Thanks to @czentgr who ran the same code in another branch and was able to run it on a runner which has the expected 15GB of space, proving this is not related to the code in this PR: https://github.com/prestodb/presto/actions/runs/13054091353/job/36420794542

abevk2023 requested review from shrinidhijoshi and a team as code owners September 20, 2024 07:24

abevk2023 requested a review from presto-oss September 20, 2024 07:24

abevk2023 changed the title ~~Implement REST API for remote functions~~ [WIP] Implement REST API for remote functions Sep 20, 2024

abevk2023 marked this pull request as draft September 20, 2024 08:45

abevk2023 force-pushed the reference_function_server branch from 5d88dbc to 6efefeb Compare September 23, 2024 06:15

tdcmeehan self-assigned this Sep 23, 2024

abevk2023 force-pushed the reference_function_server branch 6 times, most recently from 7203526 to f05ea00 Compare October 1, 2024 09:35

abevk2023 force-pushed the reference_function_server branch 2 times, most recently from 74cddc6 to cf23586 Compare October 8, 2024 15:35

abevk2023 marked this pull request as ready for review October 8, 2024 15:46

abevk2023 requested review from jaystarshot, feilong-liu, elharo and ClarenceThreepwood as code owners October 8, 2024 15:46

abevk2023 force-pushed the reference_function_server branch from cf23586 to 5e5abbe Compare October 9, 2024 10:27

abevk2023 changed the title ~~[WIP] Implement REST API for remote functions~~ Implement REST API for remote functions Oct 9, 2024

jkhaliqi reviewed Oct 9, 2024

View reviewed changes

abevk2023 force-pushed the reference_function_server branch from 5e5abbe to bb4001b Compare October 10, 2024 04:16

abevk2023 mentioned this pull request Oct 18, 2024

Introduce Plugin interface for REST function server #23857

Merged

abevk2023 force-pushed the reference_function_server branch from bb4001b to bb11659 Compare October 24, 2024 13:47

tdcmeehan requested changes Oct 24, 2024

View reviewed changes

abevk2023 force-pushed the reference_function_server branch from bb11659 to eb021e3 Compare October 28, 2024 09:41

tdcmeehan reviewed Nov 1, 2024

View reviewed changes

tdcmeehan requested changes Dec 10, 2024

View reviewed changes

tdcmeehan requested changes Dec 17, 2024

View reviewed changes

abevk2023 force-pushed the reference_function_server branch 3 times, most recently from 6b81999 to 23f69f4 Compare December 20, 2024 08:03

abevk2023 force-pushed the reference_function_server branch 2 times, most recently from 007d089 to 9e96957 Compare January 6, 2025 05:32

abevk2023 mentioned this pull request Jan 21, 2025

Add REST based Function Namespace Manager #23328

Closed

tdcmeehan reviewed Jan 22, 2025

View reviewed changes

abevk2023 force-pushed the reference_function_server branch 4 times, most recently from 63bdd7d to 1d3518b Compare January 27, 2025 15:58

tdcmeehan reviewed Jan 27, 2025

View reviewed changes

abevk2023 force-pushed the reference_function_server branch 2 times, most recently from 5efdedd to 44d6eb2 Compare January 28, 2025 13:09

tdcmeehan previously approved these changes Jan 28, 2025

View reviewed changes

abevk2023 force-pushed the reference_function_server branch from 44d6eb2 to 464406a Compare January 29, 2025 05:17

tdcmeehan dismissed their stale review via 36fde3a January 29, 2025 17:12

tdcmeehan force-pushed the reference_function_server branch from 464406a to 36fde3a Compare January 29, 2025 17:12

czentgr reviewed Jan 29, 2025

View reviewed changes

tdcmeehan force-pushed the reference_function_server branch from 36fde3a to 6ebe86c Compare January 29, 2025 20:36

abevk2023 and others added 2 commits January 30, 2025 14:28

Add REST based Function Namespace Manager

ff61404

Co-authored-by: Jacob Khaliqi <[email protected]>

Add reference REST function server

4a71933

abevk2023 force-pushed the reference_function_server branch from 6ebe86c to 4a71933 Compare January 30, 2025 09:41

tdcmeehan approved these changes Jan 30, 2025

View reviewed changes

tdcmeehan merged commit dd1893c into prestodb:master Jan 30, 2025
61 of 62 checks passed

	this.managerConfig = requireNonNull(managerConfig, "httpClient is null");
	this.managerConfig = requireNonNull(managerConfig, "managerConfig is null");

Implement REST API for remote functions #23687

Implement REST API for remote functions #23687

Conversation

abevk2023 commented Sep 20, 2024

Choose a reason for hiding this comment

abevk2023 Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdcmeehan 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

tdcmeehan 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

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

abevk2023 Jan 23, 2025 • edited Loading

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

tdcmeehan commented Jan 30, 2025

abevk2023 Oct 10, 2024 •

edited

Loading

abevk2023 Jan 23, 2025 •

edited

Loading