Allow different namespaces in builtin function namespace manager #25330
Conversation
nmahadevuni
requested review from
a team,
ClarenceThreepwood,
elharo,
feilong-liu and
jaystarshot
as code owners
prestodb-ci
requested review from
a team,
NivinCS and
infvg
and removed request for
a team
nmahadevuni
force-pushed
the
new_func_namespace
branch
from
42ef450 to
1254613
Compare
yingsu00
left a comment
yingsu00
left a comment
There was a problem hiding this comment.
Thanks @nmahadevuni . It mostly look good. Could you please update the PR message to explain that without the namespace Iceberg's DAY(), YEAR(), MONTH() functions would conflict with Presto builtin ones. Add the same message to the commit itself too. Thanks!
| } | ||
| } | ||
|
|
||
| public void assertFunction(String expr, Type type, Object value) |
| assertQuery("SELECT " + expr, Column.of(type, value)); | ||
| } | ||
|
|
||
| public void assertInvalidFunction(String expr, String exceptionPattern) |
There was a problem hiding this comment.
Not used. Can you please add some tests with invalide function name and namespace?
| if (scalarFunction != null) { | ||
| String baseName = scalarFunction.value().isEmpty() ? camelToSnake(annotatedName(annotated)) : scalarFunction.value(); | ||
| builder.add(new ScalarImplementationHeader(baseName, new ScalarHeader(description, scalarFunction.visibility(), scalarFunction.deterministic(), scalarFunction.calledOnNullInput(), descriptor))); | ||
| String functionNamespace; |
There was a problem hiding this comment.
Just put the declaration of type to the line 99
| return Optional.empty(); | ||
| } | ||
|
|
||
| public static class Column |
nmahadevuni
force-pushed
the
new_func_namespace
branch
from
1254613 to
894384a
Compare
|
Thanks @yingsu00 . I have addressed your comments. Please review. |
tdcmeehan
left a comment
tdcmeehan
left a comment
There was a problem hiding this comment.
I don't think it's a good idea to hard-code the function's namespace in the function definition itself.
- The catalog and schema are runtime properties. They are not known in advance (in code).
- We already have a catalog and schema used to determine the function namespace manager. Overloading it will be confusing (for example, if I have dynamic function namespace manager, it is confusing that a change to the schema can mean that overwrites the built-in function).
Let's think of a better design.
|
@tdcmeehan What approach would you recommend here? Do you already have something in mind or have you started working on it? If so, we're happy to close this PR and leave it in your capable hands. |
I had a chat with Tim. We decided to have a new interface |
That sounds good. |
|
Closing this as its addressed in #25594 |
4 participants
Description
This change allows functions registered via
Plugin->getFunctions()SPI to use a different namespace.Motivation and Context
At present, functions registered via getFunctions() SPI are registered in
"presto.default"namespace which is restrictive. Iceberg's DAY(), MONTH(), YEAR() functions would conflict with the builtin functions with the same name. We want to support functions specific to connectors in a custom namespace. Example: Iceberg partition transform functions as described in #25166Impact
No impact
Test Plan
Added new tests in TestScalarFunctions.java