Add a native function namespace manager #23358

pdabre12 · 2024-08-01T19:41:14Z

Description

Adds a native function namespace manager

Motivation and Context

To help resolve : #23000

Impact

Test Plan

Unit and end-to-end tests. More comprehensive end-to-end tests will be written in the future.

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
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 ==

Coordinator Plugin Changes
* Add a native function namespace manager :pr:`23358`

Depends on : #23671

linux-foundation-easycla · 2024-08-01T19:41:19Z

The committers listed above are authorized under a signed CLA.

✅ login: pdabre12 / name: Pratik Joseph Dabre (d5ccae3, 96d7055, 1c556ce, b5c2be0, 48cfc38, b947b5f, 70276a4, eb8c2cb)

steveburnett

LGTM! (docs)

Pull branch, new local doc build, the doc looks good. Thanks!

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, doc looks good. Thanks!

tdcmeehan

Just some initial comments

tdcmeehan · 2024-11-11T21:01:49Z

presto-common/src/main/java/com/facebook/presto/common/type/TypeSignature.java

+            if ((parameter.getKind() != ParameterKind.TYPE) &&
+                    (parameter.getKind() != ParameterKind.NAMED_TYPE)) {


Suggested change

if ((parameter.getKind() != ParameterKind.TYPE) &&

(parameter.getKind() != ParameterKind.NAMED_TYPE)) {

if (!(parameter.getKind() == ParameterKind.TYPE ||

parameter.getKind() == ParameterKind.NAMED_TYPE)) {

@tdcmeehan I thought the original is easier to read. but your suggestion should have || instead of &&.

Edited. I read this as "if the parameter kind is not a TYPE OR a NAMED TYPE", which to me is just a more natural way of describing the outcome than "if the parameter is not a TYPE AND not a NAMED TYPE".

But I just realized we could just do this, which seems better:

public List<TypeSignature> getTypeOrNamedTypeParametersAsTypeSignatures() { List<TypeSignature> result = new ArrayList<>(); for (TypeSignatureParameter parameter : parameters) { switch (parameter.getKind()) { case TYPE: result.add(parameter.getTypeSignature()); break; case NAMED_TYPE: result.add(parameter.getNamedTypeSignature().getTypeSignature()); break; default: throw new IllegalStateException( format("Expected all parameters to be of kind TYPE or NAMED_TYPE but [%s] kind was found for parameter: [%s]", parameter.getKind(), parameter)); } } return unmodifiableList(result); }

tdcmeehan · 2024-11-11T21:05:53Z

...n/java/com/facebook/presto/functionNamespace/AbstractSqlInvokedFunctionNamespaceManager.java

@@ -245,7 +246,7 @@ public CompletableFuture<SqlFunctionResult> executeFunction(String source, Funct
                typeManager.getType(functionMetadata.getReturnType()));
    }

-    private static PrestoException convertToPrestoException(UncheckedExecutionException exception, String failureMessage)
+    protected static PrestoException convertToPrestoException(UncheckedExecutionException exception, String failureMessage)


Just reimplement where needed, don't change the visibility.

tdcmeehan · 2024-11-11T21:06:41Z

...n/java/com/facebook/presto/functionNamespace/AbstractSqlInvokedFunctionNamespaceManager.java

@@ -362,10 +363,13 @@ protected AggregationFunctionImplementation sqlInvokedFunctionToAggregationImple
                        "Need aggregationMetadata to get aggregation function implementation");

                AggregationFunctionMetadata aggregationMetadata = function.getAggregationMetadata().get();
+                List<Type> parameters = function.getSignature().getArgumentTypes().stream().map(


Remind me why we need this please?

Its needed because we changed the SqlInvokedAggregationFunctionImplementation constructor here: https://github.com/prestodb/presto/blob/88e1152a75bf1d4edd555c197ab0c27e756d574e/presto-spi/src/main/java/com/facebook/presto/spi/function/SqlInvokedAggregationFunctionImplementation.java

presto-function-namespace-managers-common/pom.xml

presto-grpc-api/pom.xml

presto-jdbc/pom.xml

presto-main/pom.xml

tdcmeehan · 2024-11-11T21:16:32Z

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

@@ -256,14 +276,14 @@ public CacheStatsMBean getFunctionResolutionCacheStats()
    public void loadFunctionNamespaceManager(
            String functionNamespaceManagerName,
            String catalogName,
-            Map<String, String> properties)
+            Map<String, String> properties,
+            Optional<NodeManager> nodeManager)


Why is it optional?

Because nodeManager is only required while loading the NativeFunctionNamespaceManager and not the other function namespace managers, hence I thought making it optional is the right way.

Can we supply it always and just have other namespace managers not use it?

Also in some of the places where loadFunctionNamespaceManager is called I do not have access to an instance of the NodeManager.

tdcmeehan

Refactored qualifyObjectName from static to instance method ✅

tdcmeehan

Add utility functions to resolve intermediate type in aggregate funct… ❓

Thank you for the really extensive tests for this utility. I think the tests should be broken down to the various different types, because the tests are rather large.

tdcmeehan

[native] Fix bug when handling multiple params and no params aggregat… ❓

Can you help me understand when this bug manifests? Is there any way to add a unit test for it?

tdcmeehan

[native] Add additional function metadata to function signatures endp… ❌

Looks good, but there is a workspace.xml file that needs to be removed.

tdcmeehan

Add a new module presto-function-namespace-managers-common ❌

Previous feedback remains, let's keep common as a very slim module.

tdcmeehan

[native] Introduce native function namespace SPI and integrate with s… ❌

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

tdcmeehan · 2024-11-13T18:25:37Z

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

        if (name.getOriginalParts().size() != 3) {
            throw new PrestoException(FUNCTION_NOT_FOUND, format("Functions that are not temporary or builtin must be referenced by 'catalog.schema.function_name', found: %s", name));
        }
        return QualifiedObjectName.valueOf(name.getParts().get(0), name.getParts().get(1), name.getParts().get(2));
    }

+    public void configureDefaultNamespace(String defaultNamespacePrefixString)
+    {
+        String pattern = "[a-z]+\\.[a-z]+";


Let's use a constant for this.

tdcmeehan · 2024-11-13T21:00:28Z

...ain/java/com/facebook/presto/sidecar/functionNamespace/NativeFunctionDefinitionProvider.java

+            throws IllegalStateException
+    {
+        try {
+            throw new UnsupportedOperationException();


Can we just throw UOE?

tdcmeehan · 2024-11-13T21:01:04Z

...plugin/src/main/java/com/facebook/presto/sidecar/functionNamespace/NativeFunctionHandle.java

+        return signature.toString();
+    }
+
+    private static void checkArgument(boolean condition, String message, Object... args)


We can use the Guava utility.

tdcmeehan · 2024-11-13T21:03:37Z

.../main/java/com/facebook/presto/sidecar/functionNamespace/NativeFunctionNamespaceManager.java

+        return latestFunctionsTemp;
+    }
+
+    private void populateNameSpaceManager(UdfFunctionSignatureMap udfFunctionSignatureMap)


populateNamespaceManager

tdcmeehan · 2024-11-13T21:04:51Z

.../main/java/com/facebook/presto/sidecar/functionNamespace/NativeFunctionNamespaceManager.java

+    private Map<SqlFunctionId, SqlInvokedFunction> bootstrapNamespace()
+    {
+        UdfFunctionSignatureMap nativeFunctionSignatureMap = functionDefinitionProvider.getUdfDefinition(nodeManager);
+        Map<SqlFunctionId, SqlInvokedFunction> latestFunctionsTemp = new ConcurrentHashMap<>();


Please just use an immutable map, there is no need for concurrency here.

tdcmeehan · 2024-11-13T21:05:23Z

.../main/java/com/facebook/presto/sidecar/functionNamespace/NativeFunctionNamespaceManager.java

+        UdfFunctionSignatureMap nativeFunctionSignatureMap = functionDefinitionProvider.getUdfDefinition(nodeManager);
+        Map<SqlFunctionId, SqlInvokedFunction> latestFunctionsTemp = new ConcurrentHashMap<>();
+        if (nativeFunctionSignatureMap == null || nativeFunctionSignatureMap.isEmpty()) {
+            return latestFunctionsTemp;


return ImmutableMap.of();

tdcmeehan · 2024-11-13T21:12:17Z

.../main/java/com/facebook/presto/sidecar/functionNamespace/NativeFunctionNamespaceManager.java

+        return functionMetadataManager.getSpecializedFunctionKey(signature);
+    }
+
+    private Map<SqlFunctionId, SqlInvokedFunction> bootstrapNamespace()


Here is how I would suggest the concurrency control to go.

Wherever we mutate latestFunctions, synchronize that. If it's lightweight and in-memory, just synchronize the method. This applies to createFunction and bootstrapNamespace`.

tdcmeehan · 2024-11-13T21:15:22Z

.../main/java/com/facebook/presto/sidecar/functionNamespace/NativeFunctionNamespaceManager.java

+                parameterTypeSignature = parameter.getTypeSignature();
+            }
+
+            if (parameterTypeSignature.getParameters().isEmpty()) {


Let's very clearly call out the hack, where it is needed and why. Let's prefix the comment with // HACK: so it's obvious.

It seems like this is several methods long, so let's add an end comment that says when the hack is over.

tdcmeehan

[native] Add e2e native function validation tests with sidecar ✅

tdcmeehan

Add OpenAPI documentation for /v1/functions ✅

rschlussel · 2024-10-02T15:28:47Z

presto-main/src/main/java/com/facebook/presto/connector/ConnectorAwareNodeManager.java

-            throw new PrestoException(TOO_MANY_SIDECARS, "Expected exactly one coordinator sidecar, but found " + coordinatorSidecars.size());
-        }
-        return getOnlyElement(coordinatorSidecars);
+        return coordinatorSidecars.iterator().next();


same comment as on the other PR that we should choose randomly.

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

rschlussel · 2024-11-15T17:03:42Z

presto-common/src/main/java/com/facebook/presto/common/type/TypeSignature.java

+            if ((parameter.getKind() != ParameterKind.TYPE) &&
+                    (parameter.getKind() != ParameterKind.NAMED_TYPE)) {


@tdcmeehan I thought the original is easier to read. but your suggestion should have || instead of &&.

rschlussel · 2024-11-15T17:09:57Z

presto-common/src/main/java/com/facebook/presto/common/type/TypeSignature.java

@@ -146,6 +146,25 @@ public List<TypeSignature> getTypeParametersAsTypeSignatures()
        return result;
    }

+    public List<TypeSignature> getTypeOrNamedTypeParametersAsTypeSignatures()


Can you instead modify getTypeParametersAsTypeSignatures. I don't think there's any reason NamedTypeParameters are excluded there.

rschlussel · 2024-11-15T17:27:48Z

presto-common/src/main/java/com/facebook/presto/common/type/TypeSignatureUtils.java

+
+            // If parameter params are of type long e.g decimal(15, 2) or
+            // of type varchar e.g decimal(i4, i5), we don't need to recursively call the function on its params
+            if (parameter.getParameters().isEmpty() || areParametersLongOrVariable(parameter.getParameters())) {


what about parameters that are none of these kinds (long_enum, varchar_enum, distinct_type)?

also, since you've been looking at these different parameter kinds, would be great to add some javadoc to the enum to explain what the different parameter kinds mean.

rschlussel · 2024-11-15T17:37:07Z

presto-common/src/main/java/com/facebook/presto/common/type/TypeSignatureUtils.java

+{
+    private TypeSignatureUtils() {}
+
+    public static TypeSignature resolveIntermediateType(TypeSignature actualTypeSignature, List<TypeSignature> actualTypeSignatureParameters, List<TypeSignature> expectedTypeSignatureParameters)


why do we need special logic to handle aggregation function intermediate types?

We only need special logic to handle generic aggregate functions.
To resolve the intermediate type for generic aggregate functions, we rely on utility functions from TypeSignatureUtils. Specifically, we use the parameter types passed down in the function signatures to determine and insert the appropriate intermediateType.

Why do we need this?
This is required because aggregate function metadata for generic aggregate functions contain generic intermediate args i.e K,V and we need a way to resolve these args.

Example:
max function:
For the max function, here is the relevant metadata:

{ "aggregateMetadata": { "intermediateType": "t", "isOrderSensitive": false }, "docString": "native.default.max", "functionKind": "AGGREGATE", "outputType": "t", "paramTypes": [ "t" ], "routineCharacteristics": { "determinism": "DETERMINISTIC", "language": "CPP", "nullCallClause": "CALLED_ON_NULL_INPUT" }, "schema": "default", "typeVariableConstraints": [ { "comparableRequired": true, "name": "t", "nonDecimalNumericRequired": false, "orderableRequired": true, "variadicBound": "" } ], "variableArity": false }

In this case:

The functionHandle passed is an instance of NativeFunctionHandle.

Let's suppose the signature that gets passed down is native.default.max(bigint):bigint, we use the passed down param type bigint to resolve the intermediate type t as bigint.

max_by function:

{ "aggregateMetadata": { "intermediateType": "row(bigint,array(boolean),array(v))", "isOrderSensitive": true }, "docString": "native.default.max_by", "functionKind": "AGGREGATE", "outputType": "array(v)", "paramTypes": [ "v", "boolean", "bigint" ], "routineCharacteristics": { "determinism": "DETERMINISTIC", "language": "CPP", "nullCallClause": "CALLED_ON_NULL_INPUT" }, "schema": "default", "typeVariableConstraints": [ { "comparableRequired": false, "name": "v", "nonDecimalNumericRequired": false, "orderableRequired": false, "variadicBound": "" } ], "variableArity": false }

In this case:

The functionHandle passed is an instance of NativeFunctionHandle.

Let's suppose the signature that gets passed down is native.default.max_by(integer, boolean, bigint):array(integer), we use the passed down param types integer to resolve the intermediate type "intermediateType": "row(bigint,array(boolean),array(v)) as "intermediateType": "row(bigint,array(boolean),array(integer))

rschlussel · 2024-11-15T17:39:27Z

presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp

@@ -845,15 +845,17 @@ void VeloxQueryPlanConverterBase::toAggregations(
          auto pos = functionId.find(";", start + 1);
          if (pos == std::string::npos) {
            auto argumentType = functionId.substr(start + 1);
-            aggregate.rawInputTypes.push_back(
-                stringToType(argumentType, typeParser_));
+            if (!argumentType.empty()) {


Can you explain the bug? Can you share an example of how this manifests. Also, add a test. If this is a bug fix, also consider pulling this out into a separate PR to merge earlier.

Opened a PR: #24059

…te functions

…oint

…ation tests

pdabre12 · 2024-11-21T20:47:27Z

@tdcmeehan @rschlussel
Thank you for your suggestions.
Addressed your comments, please have another look.

tdcmeehan

Add utility functions to resolve intermediate type in generic aggrega…

tdcmeehan · 2024-11-21T21:41:20Z

presto-common/src/main/java/com/facebook/presto/common/type/TypeSignatureUtils.java

+        return resolveTypeSignatures(actualTypeSignature, typeSignatureMap).getTypeSignature();
+    }
+
+    public static Map<TypeSignature, TypeSignature> getTypeSignatureMap(List<TypeSignature> parameters, List<TypeSignature> argumentTypes)


I believe this is only used in the tests and in the above method. Can we make it private? I think there's sufficient unit test coverage through resolveIntermediateType.

tdcmeehan

Add a new module presto-function-namespace-managers-common ✅

Much better, thanks!

tdcmeehan

Looking pretty good, some more comments.

tdcmeehan · 2024-11-26T20:34:22Z

...to-main/src/main/java/com/facebook/presto/server/NativeFunctionNamespaceManagerProvider.java

+
+import static java.util.Objects.requireNonNull;
+
+public class NativeFunctionNamespaceManagerProvider


Why do we need this class? Seems unused?

tdcmeehan · 2024-11-26T20:41:29Z

presto-main/src/main/java/com/facebook/presto/server/PluginManagerConfig.java

+    }
+
+    @Config("presto.default-namespace")
+    public PluginManagerConfig setDefaultNamespacePrefix(String defaultNamespacePrefix)


Rather than match a default namespace with a namespace that is configured in the function namespace manager, just make it a boolean flag property on the function namespace manager's config that it is the default namespace. If none have that specified, use presto.default, if more than one have that specified, then throw.

The place to add this property would be in StaticFunctionNamespaceStore.

tdcmeehan · 2024-11-26T20:44:03Z

.../main/java/com/facebook/presto/sidecar/functionNamespace/NativeFunctionNamespaceManager.java

+        return aggregationImplementationByHandle.get(sqlFunctionHandle);
+    }
+
+    private static SqlInvokedFunction copyFunction(SqlInvokedFunction function)


SqlInvokedFunction is immutable, why do we need to do this copy?

tdcmeehan · 2024-11-26T20:45:29Z

.../main/java/com/facebook/presto/sidecar/functionNamespace/NativeFunctionNamespaceManager.java

+                parameterTypeSignature = parameter.getTypeSignature();
+            }
+
+            if (parameterTypeSignature.getParameters().isEmpty()) {


It seems like this is several methods long, so let's add an end comment that says when the hack is over.

tdcmeehan · 2024-11-26T20:46:49Z

.../main/java/com/facebook/presto/spi/function/SqlInvokedAggregationFunctionImplementation.java

+    @Override
+    public AggregationMetadata getAggregationMetadata()
+    {
+        return null;


I believe it should be safe or even preferable to throw here, since they should be called.

pdabre12 force-pushed the native-function-namespace-manager branch from 49d3b9d to 5ed5a18 Compare August 13, 2024 23:37

pramodsatya mentioned this pull request Aug 15, 2024

[native] Retrieve Json function metadata for prestissimo functions #22332

Merged

pdabre12 force-pushed the native-function-namespace-manager branch 2 times, most recently from bcb09bc to ce7ca4b Compare August 17, 2024 00:06

steveburnett previously approved these changes Sep 10, 2024

View reviewed changes

pdabre12 dismissed steveburnett’s stale review via 4d00991 September 12, 2024 18:28

pdabre12 force-pushed the native-function-namespace-manager branch from 4d00991 to c3ed29f Compare September 13, 2024 23:03

pdabre12 requested a review from steveburnett September 13, 2024 23:05

pdabre12 changed the title ~~[WIP] Native function namespace manager~~ [WIP] Add a native function namespace manager Sep 13, 2024

steveburnett previously approved these changes Sep 25, 2024

View reviewed changes

pdabre12 dismissed steveburnett’s stale review via 3fde055 September 27, 2024 00:48

pdabre12 force-pushed the native-function-namespace-manager branch 2 times, most recently from 3fde055 to ebfa361 Compare September 27, 2024 05:55

pdabre12 changed the title ~~[WIP] Add a native function namespace manager~~ Add a native function namespace manager Sep 27, 2024

pdabre12 force-pushed the native-function-namespace-manager branch from f203d25 to 88e1152 Compare November 1, 2024 01:09

pdabre12 marked this pull request as ready for review November 1, 2024 16:40

pdabre12 requested review from elharo, a team, shrinidhijoshi, jaystarshot, feilong-liu and ClarenceThreepwood as code owners November 1, 2024 16:40

pdabre12 requested a review from presto-oss November 1, 2024 16:40

tdcmeehan self-assigned this Nov 11, 2024

tdcmeehan requested changes Nov 11, 2024

View reviewed changes

tdcmeehan reviewed Nov 12, 2024

View reviewed changes

tdcmeehan requested changes Nov 13, 2024

View reviewed changes

tdcmeehan reviewed Nov 13, 2024

View reviewed changes

tdcmeehan requested changes Nov 13, 2024

View reviewed changes

tdcmeehan reviewed Nov 13, 2024

View reviewed changes

tdcmeehan requested changes Nov 13, 2024

View reviewed changes

tdcmeehan reviewed Nov 13, 2024

View reviewed changes

rschlussel reviewed Nov 15, 2024

View reviewed changes

pdabre12 added 8 commits November 21, 2024 11:45

[cherry-pick] Do not review

48cfc38

[native] Add presto.default-namespace Prestissimo config property

eb8c2cb

Add utility functions to resolve intermediate type in generic aggrega…

96d7055

…te functions

[native] Add additional function metadata to function signatures endp…

b5c2be0

…oint

Add a new module presto-function-namespace-managers-common

70276a4

[native] Introduce native function namespace SPI

b947b5f

[native] Integrate SPI with sidecar and add e2e native function valid…

1c556ce

…ation tests

Add OpenAPI documentation for /v1/functions

d5ccae3

pdabre12 force-pushed the native-function-namespace-manager branch from 88e1152 to d5ccae3 Compare November 21, 2024 20:43

tdcmeehan reviewed Nov 21, 2024

View reviewed changes

pdabre12 mentioned this pull request Nov 21, 2024

[native] Fix bug when parsing SqlFunctionHandle #24059

Open

tdcmeehan requested changes Nov 26, 2024

View reviewed changes

		if ((parameter.getKind() != ParameterKind.TYPE) &&
		(parameter.getKind() != ParameterKind.NAMED_TYPE)) {


		import static java.util.Objects.requireNonNull;

		public class NativeFunctionNamespaceManagerProvider

Add a native function namespace manager #23358

Are you sure you want to change the base?

Add a native function namespace manager #23358

Conversation

pdabre12 commented Aug 1, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

linux-foundation-easycla bot commented Aug 1, 2024 • edited Loading

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

tdcmeehan left a comment

Choose a reason for hiding this comment

tdcmeehan Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdcmeehan Nov 15, 2024 • 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

tdcmeehan left a comment

Choose a reason for hiding this comment

tdcmeehan left a comment

Choose a reason for hiding this comment

tdcmeehan left a comment

Choose a reason for hiding this comment

tdcmeehan left a comment

Choose a reason for hiding this comment

tdcmeehan left a 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

tdcmeehan left a 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

pdabre12 commented Nov 21, 2024

tdcmeehan left a 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

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

pdabre12 commented Aug 1, 2024 •

edited

Loading

linux-foundation-easycla bot commented Aug 1, 2024 •

edited

Loading

tdcmeehan Nov 11, 2024 •

edited

Loading

tdcmeehan Nov 15, 2024 •

edited

Loading