Add RFC for new SPI for SQL invoked functions

[pdabre12]

No description provided.

[tdcmeehan]

Correct me if I'm wrong, but strictly speaking Java users may continue to use the getFunctions() SPI, right?

[pdabre12]

Yes, getFunctions() SPI can still be used by Java users.

The key difference is when the new getSqlInvokedFunctions() SPI is used in a plugin, the sql invoked functions under that plugin returned by the getSqlInvokedFunctions() SPI will be loaded under a new plugin-based namespace manager BuiltInPluginFunctionNamespaceManager.

In theory, you could also have a getFunctions() SPI for the same plugin and all those functions will be registered as built-in functions under the BuiltInTypeAndFunctionNamespaceManager as is the current behavior.

I will add a test case testing this behaviour.

[feilong-liu]

Looks good to me. Add one more comment on extensions of the design to better support Java/Cpp/SQL functions.

[feilong-liu]

Will SQL invoked functions having native implementations be skipped? I am asking because I see in the current PR https://github.com/prestodb/presto/pull/25597/files it will throw error for duplicates.
I am wondering if we can skip if getting name conflicts, as it will not need manually removal of existing functions.

[pdabre12]

Yes, SQL invoked functions having native implementations will be skipped provided we load the presto-native-sql-invoked-functions-plugin, this is only supported for sidecar-enabled clusters.

I am wondering if we can skip if getting name conflicts, as it will not need manually removal of existing functions.

Yes, this could be done but it makes sense only in the case of overridden native implementations because we prioritize those over the sql invoked fns. In other cases/ plugins loaded , there isn't a priority assigned to the Fns loaded via the plugin so I thought it's better to fail at duplicate signatures instead of silently skipping them. What do you think?

Also the functions loaded via the plugin will be available before the functions from the sidecar(as its a lazy fetch call), so you will need to remove that overridden plugin loaded sql invoked fn from BuiltInPluginFunctionNamespaceManager which isn't ideal.

[feilong-liu]

If name conflicts will lead to failure, does this mean that we need to make sure that functions registered does not have functions with the same name?
As shared in #41, we are trying to register all functions, java, native, SQL into the same namespace, and there are overlap between these functions. If we are to fail for conflict names, it means that we need to manually removing some functions from registration.

[pdabre12]

So we check for conflict mismatches only against the plugins and the default serving namespace manager. The code block where we do the conflict check.

private synchronized FunctionMap checkForNamingConflicts()
    {
        Optional<FunctionNamespaceManager<?>> functionNamespaceManager =
                functionAndTypeManager.getServingFunctionNamespaceManager(functionAndTypeManager.getDefaultNamespace());
        checkArgument(functionNamespaceManager.isPresent(), "Cannot find function namespace for catalog '%s'", functionAndTypeManager.getDefaultNamespace().getCatalogName());
        checkForNamingConflicts(functionNamespaceManager.get().listFunctions(Optional.empty(), Optional.empty()));
        return functions;
    }

    private synchronized void checkForNamingConflicts(Collection<? extends SqlFunction> functions)
    {
        for (SqlFunction function : functions) {
            for (SqlFunction existingFunction : this.functions.list()) {
                checkArgument(!function.getSignature().equals(existingFunction.getSignature()), "Function already registered: %s", function.getSignature());
            }
        }
    }

We would need to add code here to check signature conflicts for functions residing in the BuiltInNativeFunctionNamespaceManager.

To clarify, the checks are:

1. Plugins with other plugins
2. Plugins with BuiltInNativeFunctionNamespaceManager
3. Plugins with current serving namespace manager

Note that we aren't checking for conflicts with BuiltInNativeFunctionNamespaceManager against the current serving namespace manager, but we still wanna enforce failing on duplicate signatures in other places.

[pdabre12]

In short, the specific use case shared in #41 should work, but we might need some modifications in #resolveFunctionInternal

[feilong-liu]

@kevintang2022 has a proposal which use this design to support native functions in Prestissimo https://docs.google.com/document/d/13tv1NYKWfy3v54kV1fyXFOYMHpCUkcEd6w8XSTyd2d0/edit?usp=sharing
which needs some extensions for this design.
I wonder if we can have one more built-in function plugin for native functions, so that native functions will also be under presto.default namespace, which will lead to seamless support for evaluation of functions of Java/Cpp/SQL at the same time.

[pdabre12]

I commented on that RFC, had one doubt, will come back to this question.

[pdabre12]

Thanks for the review @tdcmeehan @feilong-liu . Can you please have another look?

[pdabre12]

@rschlussel Do you have any comments on this?