Support CREATE FUNCTION execution by caithagoras · Pull Request #13561 · prestodb/presto

caithagoras · 2019-10-16T19:57:00Z

== RELEASE NOTES ==

General Changes
* Add support for ``CREATE FUNCTION``

caithagoras · 2019-10-24T05:31:58Z

@rongrong Thanks for the review! Support MySQL based function namespace manager is the last commit. The commits beyond that have not been finished and will be moved to a separate PR for the purpose of executing CREATE FUNCTION.

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

rongrong

Looks good. Mostly nits.

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

rongrong · 2019-10-28T23:31:07Z

presto-main/src/main/java/com/facebook/presto/sql/gen/FunctionCallCodeGenerator.java

I think I mentioned before that getSuffix is more accurate than getUnqualifiedName. If you prefer to use "unqualified name", it's more appropriate to have a utility function unqualifyName(qualifiedName). I'm not sure whether you can call the last part of a qualified name "unqualified name".

It is a convention to called the last part of a "fully qualified object" (file path, class name, etc) a unqualified name. Here are some reference:

https://stackoverflow.com/a/27966682

https://en.wikipedia.org/wiki/Fully_qualified_name

Because we're defining a specialized class for a fully-qualified function name, it sounds weird to me that it consists of (functionNamespace, suffix)

Again, those reference only said a name without prefix is a unqualified name, rather than saying the last part of a qualified name is a "unqualified name". The reference you mentioned also used "suffix", which I still think is better than "unqualified name". If you want to be stubborn on this I won't insist. But I think "suffix" is better than "unqualified name" in this case.

Looking at some of our existing classes:

QualifiedObjectName is defined as (catalogName, schemaName, objectName).

QualifiedTablePrefix is defined as (catalogName, schemaName, tableName).

What about defining QualifiedFunctionName as (functionNamespace, functionName)?

rongrong · 2019-10-28T23:32:29Z

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

That should be defined as (CatalogSchemaName functionNamespace, String suffix) then.

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

...rc/main/java/com/facebook/presto/sqlfunction/AbstractSqlInvokedFunctionNamespaceManager.java

presto-main/src/main/java/com/facebook/presto/execution/CreateFunctionTask.java

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

rongrong

Add some tests to AbstractTestQueries, including explainDdl.

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

presto-main/src/main/java/com/facebook/presto/execution/CreateFunctionTask.java

rongrong

Please add some tests to AbstractTestQueries and address remaining comments.

caithagoras · 2019-10-30T02:15:04Z

Added explainDdl test.

Use CatalogSchemaName to represent function namespace. Also - Fix a bug in getServingFunctionNamespaceManager to correctly return the most exact match instead of the least exact match. - Throw PrestoException when resolving non-builtin functions with a QualifiedName with not exactly 3 parts.

Also - Make SqlInvokedFunction the parameter type for createFunction. - Store SqlFunctionId within SqlFunctionHandle. - Convert SqlInvokedFunction#versioned to instance method.

Also rename the class to SqlInvokedFunctionTestUtils

Rollback is not supported by Presto transactions.

caithagoras · 2019-10-30T06:08:50Z

Added test TestSqlFunctions.
Added semantic checks for duplicating column names.
Added a TODO to check for SQL invoked functions in function body.
Fix an issue when fetching function implementation in FunctionManager.

rongrong · 2019-10-30T17:53:09Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

Isn't it simpler to put all names in a set and check the size?

I need the duplicate parameter names to construct a user-friendly error message, but size checking won't tell me what are duplicated.

Alternatively, printing one name instead of a list is fine with me as well, but I don't think that will make the code simpler.

I'd just print the whole parameter list lol but sure.

presto-main/src/main/java/com/facebook/presto/sql/analyzer/SemanticErrorCode.java

rongrong · 2019-10-30T23:26:43Z

Thanks for working on this! Looking forward to see this in production!

facebook-github-bot added the CLA Signed label Oct 16, 2019

caithagoras force-pushed the s1 branch 29 times, most recently from 1bee024 to d9a8110 Compare October 21, 2019 18:07

rongrong reviewed Oct 28, 2019

View reviewed changes

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

rongrong reviewed Oct 28, 2019

View reviewed changes

rongrong reviewed Oct 29, 2019

View reviewed changes

presto-main/src/main/java/com/facebook/presto/execution/CreateFunctionTask.java Outdated Show resolved Hide resolved

rongrong reviewed Oct 29, 2019

View reviewed changes

presto-main/src/main/java/com/facebook/presto/execution/CreateFunctionTask.java Outdated Show resolved Hide resolved

rongrong reviewed Oct 29, 2019

View reviewed changes

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

rongrong reviewed Oct 29, 2019

View reviewed changes

rongrong reviewed Oct 30, 2019

View reviewed changes

rongrong approved these changes Oct 30, 2019

View reviewed changes

caithagoras0 added 8 commits October 29, 2019 20:05

Move spi.relation.FullyQualifiedName to spi.QualifiedFunctionName

9dcb5e9

Rename SqlInvokedRegularFunction#comment to description

052b612

Move classes related to SQL-invoked functions into spi

8688e43

Also - Make SqlInvokedFunction the parameter type for createFunction. - Store SqlFunctionId within SqlFunctionHandle. - Convert SqlInvokedFunction#versioned to instance method.

Move SqlInvokedRegularFunctionTestUtils to presto-sql-functions

6324675

Also rename the class to SqlInvokedFunctionTestUtils

Add ParametersAreNonnullByDefault annotation to cache loading methods

5b1a36b

Rename FunctionNamespaceManager#rollback to #abort

5638810

Rollback is not supported by Presto transactions.

Add support for refreshing functions cache in namespace manager

077b9aa

rongrong reviewed Oct 30, 2019

View reviewed changes

presto-main/src/main/java/com/facebook/presto/sql/analyzer/SemanticErrorCode.java Outdated Show resolved Hide resolved

caithagoras0 added 2 commits October 30, 2019 12:07

Support CREATE FUNCTION execution

8defbfe

Fix an issue when fetching function implementation in FunctionManager

cb83587

viczhang861 mentioned this pull request Nov 21, 2019

Construct QualifiedFunctionName with CatalogSchemaName and function name #13726

Merged

Conversation

caithagoras commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caithagoras commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rongrong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rongrong Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

caithagoras Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rongrong Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

caithagoras Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rongrong Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rongrong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rongrong left a comment

Choose a reason for hiding this comment

Uh oh!

caithagoras commented Oct 30, 2019

Uh oh!

caithagoras commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rongrong Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

caithagoras Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rongrong Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rongrong commented Oct 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

caithagoras commented Oct 16, 2019 •

edited

Loading

caithagoras commented Oct 24, 2019 •

edited

Loading

caithagoras Oct 29, 2019 •

edited

Loading

caithagoras Oct 30, 2019 •

edited

Loading

caithagoras commented Oct 30, 2019 •

edited

Loading

caithagoras Oct 30, 2019 •

edited

Loading