Introduce `getSqlInvokedFunctions` SPI and `BuiltInPluginFunctionNamespaceManager` for registering sql invoked functions #25597

pdabre12 · 2025-07-22T19:30:21Z

Description

Introduces a new getSqlInvokedFunctions SPI in Presto, which only supports SQL invoked functions. This SPI enables plugins to register SQL invoked functions via a new built-in plugin function namespace manager interface: BuiltInPluginFunctionNamespaceManager.

Motivation and Context

This change is motivated by the need to safely support SQL invoked functions that may overlap with native C++ functions
pulled from the sidecar. By registering inlined SQL invoked functions through a dedicated SPI and validating them against the functions residing in the default function namespace manager, we ensure clear separate and prevent conflicts in function resolution.

Impact

No impact for now, but after this change the inlined SQL invoked functions would no longer be registered as built in functions under the BuiltInTypeAndFunctionNamespaceManager but instead be provided via a plugin.

Test Plan

CI and Unit tests

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

SPI changes
* Add a new  ``getSqlInvokedFunctions`` SPI  in Presto, which only supports SQL invoked functions.

General Changes
* Add a new built-in plugin function namespace manager interface: ``BuiltInPluginFunctionNamespaceManager``.

...-base/src/main/java/com/facebook/presto/metadata/BuiltInTypeAndFunctionNamespaceManager.java

presto-main-base/src/main/java/com/facebook/presto/metadata/BuiltInFunctionHandle.java

pdabre12 · 2025-07-25T21:12:20Z

@rschlussel @tdcmeehan Can you please have a look?

kevintang2022 · 2025-07-25T23:07:56Z

Hi Pratik. I have some questions:

Why are we only doing this for SqlInvoked function? Is it just for the inline sql functions use case?
What happens if there is a naming conflict? The server fails to start. Do you think it would be useful in the case that we do run into a conflict, then we have some logic to determine which of the two functions to keep?

presto-main-base/src/main/java/com/facebook/presto/metadata/FunctionAndTypeManager.java

pdabre12 · 2025-07-29T18:04:49Z

@kevintang2022 I am writing a RFC for this SPI change. I hope that'll answer all the questions, but if not I can come back and clarify here.

tdcmeehan

I'm wondering why BuiltInPluginFunctionNamespaceManager doesn't actually extend from a FunctionNamespaceManager? Also if we changed it so it was a namespace manager, could we use a subclass of FunctionHandle to indicate it's a built-in.

pdabre12 · 2025-07-29T18:13:42Z

@tdcmeehan I did not extend it from FunctionNamespaceManager since I did not think we would need the full capabilities of a FunctionNamespaceManager. However, now I think we are already performing most of the responsibilities that a FunctionNamespaceManager is supposed to do, so I will make BuiltInPluginFunctionNamespaceManager inherit from FunctionNamespaceManager.

feilong-liu · 2025-08-07T21:45:22Z

presto-main-base/src/main/java/com/facebook/presto/metadata/FunctionAndTypeManager.java

+            // Could still match to a magic literal function
+            if (e.getErrorCode().getCode() != StandardErrorCode.FUNCTION_NOT_FOUND.toErrorCode().getCode()) {
+                throw e;
+            }


Why will this happen? I assume this PR should not change the existing behavior, i.e. an AA change?

Do you mean this change in particular?

// Could still match to a magic literal function if (e.getErrorCode().getCode() != StandardErrorCode.FUNCTION_NOT_FOUND.toErrorCode().getCode()) { throw e; }

getMatchingFunctionHandle could return exceptions now, just catching a specific exception, if the exception is because no matching function was found, we continue searching as it could still match to a magic literal function. It still matches what the current behavior is just an additional try catch block.

feilong-liu · 2025-08-07T21:45:59Z

presto-main-base/src/main/java/com/facebook/presto/metadata/FunctionAndTypeManager.java

@@ -834,7 +849,6 @@ private Optional<FunctionNamespaceManager<? extends SqlFunction>> getServingFunc
    }

    @Override
-    @SuppressWarnings("unchecked")


Why removing this?

Collection<SqlFunction> candidates = (Collection<SqlFunction>) functionNamespaceManager.get().getFunctions(Optional.empty(), functionName);
We removed this cast hence that is no longer needed.

feilong-liu · 2025-08-07T21:56:22Z

presto-main-base/src/main/java/com/facebook/presto/metadata/FunctionExtractor.java

@@ -61,12 +68,12 @@ public static List<? extends SqlFunction> extractFunctions(Class<?> clazz)
        }

        if (clazz.isAnnotationPresent(SqlInvokedScalarFunction.class)) {
-            return SqlInvokedScalarFromAnnotationsParser.parseFunctionDefinition(clazz);
+            return SqlInvokedScalarFromAnnotationsParser.parseFunctionDefinition(clazz, defaultNamespace);


Does this mean that SqlInvokedScalarFunction will all be under the defaultNamespace? There are other function plugins which return SqlInvokedFunctions, for example MySqlFunctionNamespaceManager, RestBasedFunctionNamespaceManager, JsonFileBasedFunctionNamespaceManager, will this be a problem?

I guess we accept the fully qualified name while registering functions under those namespaces.
In this change, I am just adding a parameter to indicate what the default namespace is. If it went down this code path earlier without my change, we were extracting and registering those functions under presto.default.

return Stream.concat(Stream.of(functionHeader.value()), stream(functionHeader.alias())) .map(name -> new SqlInvokedFunction( QualifiedObjectName.valueOf(JAVA_BUILTIN_NAMESPACE, name), parameters, typeVariableConstraints, emptyList(),

Now we just pass in the default namespace param instead of registering it under JAVA_BUILTIN_NAMESPACE (presto.default) always.

feilong-liu · 2025-08-07T21:56:48Z

presto-main-base/src/main/java/com/facebook/presto/metadata/FunctionListBuilder.java

@@ -62,13 +63,13 @@ public FunctionListBuilder scalars(Class<?> clazz)

    public FunctionListBuilder sqlInvokedScalar(Class<?> clazz)
    {
-        functions.addAll(SqlInvokedScalarFromAnnotationsParser.parseFunctionDefinition(clazz));
+        functions.addAll(SqlInvokedScalarFromAnnotationsParser.parseFunctionDefinition(clazz, JAVA_BUILTIN_NAMESPACE));


Similar questions as above

Summary: This PR includes changes for Built in functions for different use cases - Register SQL invoked function in built in namespace when they come from plugins (prestodb#25597) - Register Native functions in built in namespace when they come from sidecar (prestodb/rfcs#41) Since these use cases have a lot of overlap, we can introduce an abstract class in order to reduce duplicate logic. ## Description ## Motivation and Context ## Impact Pull Request resolved: prestodb#25661 Test Plan:  ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == RELEASE NOTES == General Changes * ... * ... Hive Connector Changes * ... * ... ``` If release note is NOT required, use: ``` == NO RELEASE NOTE == ``` Rollback Plan: Differential Revision: D79301760 Pulled By: kevintang2022

presto-main-base/src/main/java/com/facebook/presto/metadata/BuiltInFunctionHandle.java

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

Summary: This PR includes changes for Built in functions for different use cases - Register SQL invoked function in built in namespace when they come from plugins (prestodb#25597) - Register Native functions in built in namespace when they come from sidecar (prestodb/rfcs#41) Since these use cases have a lot of overlap, we can introduce an abstract class in order to reduce duplicate logic. ## Description ## Motivation and Context ## Impact Pull Request resolved: prestodb#25661 Test Plan:  ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == RELEASE NOTES == General Changes * ... * ... Hive Connector Changes * ... * ... ``` If release note is NOT required, use: ``` == NO RELEASE NOTE == ``` Rollback Plan: Differential Revision: D79301760 Pulled By: kevintang2022

tdcmeehan

Just some nits

presto-main-base/src/main/java/com/facebook/presto/metadata/FunctionMap.java

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

…ceManager for registering sql invoked functions

Co-Authored-by: Naveen Mahadevuni <[email protected]>

steveburnett · 2025-08-14T19:33:06Z

Thanks for the release note! Minor format nits:

== RELEASE NOTES ==

SPI changes
* Add a new  ``getSqlInvokedFunctions`` SPI  in Presto, which only supports SQL invoked functions.

General Changes
* Add a new built-in plugin function namespace manager interface: ``BuiltInPluginFunctionNamespaceManager``.

tdcmeehan

Good work!

prestodb-ci added the from:IBM PR from IBM label Jul 22, 2025

pdabre12 changed the title ~~Introduce getSqlFunctions SPI and BuiltInPluginFunctionNamespaceManager for plugin functions~~ Introduce getSqlFunctions SPI and BuiltInPluginFunctionNamespaceManager for registering sql invoked functions Jul 22, 2025

pdabre12 marked this pull request as ready for review July 22, 2025 21:12

pdabre12 requested a review from a team as a code owner July 22, 2025 21:12

prestodb-ci requested review from a team, wanglinsong and nmahadevuni and removed request for a team July 22, 2025 21:12

pdabre12 changed the title ~~Introduce getSqlFunctions SPI and BuiltInPluginFunctionNamespaceManager for registering sql invoked functions~~ Introduce getSqlInvokedFunctions SPI and BuiltInPluginFunctionNamespaceManager for registering sql invoked functions Jul 22, 2025

pdabre12 force-pushed the sql-invoked-functions-SPI branch from 9132721 to 343f6e3 Compare July 22, 2025 22:48

pdabre12 requested review from tdcmeehan, rschlussel and amitkdutta July 22, 2025 22:53

kevintang2022 reviewed Jul 23, 2025

View reviewed changes

...-base/src/main/java/com/facebook/presto/metadata/BuiltInTypeAndFunctionNamespaceManager.java Outdated Show resolved Hide resolved

presto-main-base/src/main/java/com/facebook/presto/metadata/BuiltInFunctionHandle.java Outdated Show resolved Hide resolved

pdabre12 force-pushed the sql-invoked-functions-SPI branch 2 times, most recently from 6b2e35c to 6977df3 Compare July 25, 2025 18:48

pdabre12 requested a review from kevintang2022 July 25, 2025 18:48

pdabre12 force-pushed the sql-invoked-functions-SPI branch from 6977df3 to 8825888 Compare July 25, 2025 19:03

kevintang2022 reviewed Jul 29, 2025

View reviewed changes

presto-main-base/src/main/java/com/facebook/presto/metadata/FunctionAndTypeManager.java Show resolved Hide resolved

tdcmeehan reviewed Jul 29, 2025

View reviewed changes

feilong-liu reviewed Aug 7, 2025

View reviewed changes

pdabre12 force-pushed the sql-invoked-functions-SPI branch from 8825888 to 6c9bea4 Compare August 11, 2025 19:35

pdabre12 requested review from jaystarshot, elharo and ClarenceThreepwood as code owners August 11, 2025 19:35

pdabre12 force-pushed the sql-invoked-functions-SPI branch from 6c9bea4 to 021fb1c Compare August 11, 2025 20:01

pdabre12 requested review from feilong-liu and tdcmeehan August 11, 2025 20:02

kevintang2022 mentioned this pull request Aug 11, 2025

Native built in functions demo 1 #25661

Closed

6 tasks

kevintang2022 reviewed Aug 12, 2025

View reviewed changes

presto-main-base/src/main/java/com/facebook/presto/metadata/BuiltInFunctionHandle.java Outdated Show resolved Hide resolved

kevintang2022 reviewed Aug 12, 2025

View reviewed changes

presto-spi/src/main/java/com/facebook/presto/spi/function/FunctionHandle.java Outdated Show resolved Hide resolved

pdabre12 force-pushed the sql-invoked-functions-SPI branch from da0f19e to 8d9aa5a Compare August 12, 2025 23:09

pdabre12 requested a review from kevintang2022 August 12, 2025 23:09

kevintang2022 previously approved these changes Aug 12, 2025

View reviewed changes

feilong-liu previously approved these changes Aug 12, 2025

View reviewed changes

tdcmeehan reviewed Aug 14, 2025

View reviewed changes

presto-main-base/src/main/java/com/facebook/presto/metadata/FunctionMap.java Show resolved Hide resolved

presto-spi/src/main/java/com/facebook/presto/spi/function/BuiltInFunctionKind.java Outdated Show resolved Hide resolved

pdabre12 dismissed stale reviews from feilong-liu and kevintang2022 via bd0669b August 14, 2025 18:35

pdabre12 and others added 2 commits August 14, 2025 11:38

Introduce getSqlInvokedFunctions SPI and BuiltInPluginFunctionNamespa…

2620479

…ceManager for registering sql invoked functions

Add tests for getSqlInvokedFunctions() SPI

230a997

Co-Authored-by: Naveen Mahadevuni <[email protected]>

pdabre12 force-pushed the sql-invoked-functions-SPI branch from bd0669b to 230a997 Compare August 14, 2025 18:38

pdabre12 requested a review from tdcmeehan August 14, 2025 18:44

tdcmeehan approved these changes Aug 14, 2025

View reviewed changes

pdabre12 merged commit 4a71fed into prestodb:master Aug 14, 2025
68 checks passed

pdabre12 mentioned this pull request Aug 18, 2025

Support BuiltInFunctionKind enum parameter in BuiltInFunctionHandle's JSON constructor creator #25821

Merged

6 tasks

Introduce getSqlInvokedFunctions SPI and BuiltInPluginFunctionNamespaceManager for registering sql invoked functions #25597

Introduce getSqlInvokedFunctions SPI and BuiltInPluginFunctionNamespaceManager for registering sql invoked functions #25597

Uh oh!

Conversation

pdabre12 commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

Uh oh!

Uh oh!

pdabre12 commented Jul 25, 2025

Uh oh!

kevintang2022 commented Jul 25, 2025

Uh oh!

Uh oh!

pdabre12 commented Jul 29, 2025

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

pdabre12 commented Jul 29, 2025

Uh oh!

feilong-liu Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

pdabre12 Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

feilong-liu Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

pdabre12 Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

feilong-liu Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

pdabre12 Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

feilong-liu Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steveburnett commented Aug 14, 2025

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Introduce `getSqlInvokedFunctions` SPI and `BuiltInPluginFunctionNamespaceManager` for registering sql invoked functions #25597

Introduce `getSqlInvokedFunctions` SPI and `BuiltInPluginFunctionNamespaceManager` for registering sql invoked functions #25597

pdabre12 commented Jul 22, 2025 •

edited

Loading