New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support MySQL-based FunctionNamespaceManager #13972

Merged

caithagoras merged 6 commits into prestodb:master from caithagoras:s1

Feb 8, 2020

Contributor

caithagoras commented Jan 16, 2020 •

edited by rongrong

Loading

== RELEASE NOTES ==

General Changes
* Add a MySQL-based function namespace manager implementation that supports creating, altering, and dropping SQL functions. (:doc:`/admin/function-namespace-managers`)

caithagoras added the wip label

caithagoras changed the title ~~[WIP] Support MySql-backed FunctionNamespaceManager~~ [WIP] Support MySql-based FunctionNamespaceManager

caithagoras changed the title ~~[WIP] Support MySql-based FunctionNamespaceManager~~ [WIP] Support MySQL-based FunctionNamespaceManager

caithagoras force-pushed the s1 branch 4 times, most recently from cb46ab3 to b6d4803 Compare

January 16, 2020 03:15

caithagoras changed the title ~~[WIP] Support MySQL-based FunctionNamespaceManager~~ Support MySQL-based FunctionNamespaceManager

caithagoras requested a review from rongrong

January 16, 2020 03:15

caithagoras assigned rongrong

caithagoras added sql-function and removed wip labels

caithagoras force-pushed the s1 branch 2 times, most recently from 7cc9ff8 to 6b887ea Compare

January 16, 2020 19:23

Contributor

rongrong commented Jan 21, 2020

A very high level comment: I see that you are renaming the module presto-sql-function to presto-function-namespace-managers and adding the function namespace manager directly to this module. If you are planning to do this, you probably want to introduce subdirectories to organize the code rather than keep them all under .../sql-function.

caithagoras force-pushed the s1 branch from 6b887ea to 2557eae Compare

January 21, 2020 22:46

Contributor Author

caithagoras commented Jan 21, 2020 •

edited

Loading

@rongrong Renamed package com.facebook.presto.sqlfunction to com.facebook.presto.functionNamespace

rongrong requested changes

View reviewed changes

...ce-managers/src/main/java/com/facebook/presto/functionNamespace/db/FunctionNamespaceDao.java Outdated Show resolved Hide resolved

...ce-managers/src/main/java/com/facebook/presto/functionNamespace/db/FunctionNamespaceDao.java Outdated Show resolved Hide resolved

...ce-managers/src/main/java/com/facebook/presto/functionNamespace/db/FunctionNamespaceDao.java Outdated Show resolved Hide resolved

...ce-managers/src/main/java/com/facebook/presto/functionNamespace/db/FunctionNamespaceDao.java Outdated Show resolved Hide resolved

...ce-managers/src/main/java/com/facebook/presto/functionNamespace/db/FunctionNamespaceDao.java Outdated Show resolved Hide resolved

...ce-managers/src/main/java/com/facebook/presto/functionNamespace/db/FunctionNamespaceDao.java Outdated Show resolved Hide resolved

...ce-managers/src/main/java/com/facebook/presto/functionNamespace/db/FunctionNamespaceDao.java Outdated Show resolved Hide resolved

...gers/src/main/java/com/facebook/presto/functionNamespace/db/SqlInvokedFunctionRowMapper.java Outdated Show resolved Hide resolved

...s/src/test/java/com/facebook/presto/functionNamespace/db/TestDbFunctionNamespaceManager.java Outdated Show resolved Hide resolved

...src/main/java/com/facebook/presto/functionNamespace/db/DbFunctionNamespaceManagerConfig.java Outdated Show resolved Hide resolved

caithagoras force-pushed the s1 branch 4 times, most recently from 459927a to 9b97d8d Compare

January 23, 2020 20:29

caithagoras added the wip label

caithagoras force-pushed the s1 branch 2 times, most recently from 4ae4d07 to ad2a9a6 Compare

January 24, 2020 01:05

Contributor Author

caithagoras commented Jan 24, 2020 •

edited

Loading

@rongrong Ready for another pass.
Made a few improvements in the 3rd commit and will merge into the 2nd commit before merging.

Use annotation to pre-define all <function-namespaces-table> and <sql-functions-table> without requiring String to be passed as parameters.
Use module to create FunctionNamespaceManager in test, so that the module is tested as well.
Update some config names and default values.

caithagoras force-pushed the s1 branch 4 times, most recently from a57e47d to 9740926 Compare

January 24, 2020 21:53


          Use Return statement as the body for SQL-invoked functions

e49addd

caithagoras force-pushed the s1 branch from bd666b2 to a45ced3 Compare

February 7, 2020 16:34

sachdevs reviewed

View reviewed changes

presto-docs/src/main/sphinx/admin/function-namespace-managers.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/admin/function-namespace-managers.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/admin/function-namespace-managers.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/admin/function-namespace-managers.rst Outdated Show resolved Hide resolved

caithagoras force-pushed the s1 branch 3 times, most recently from b5816c3 to 1ff467c Compare

February 7, 2020 21:46

Contributor Author

caithagoras commented Feb 7, 2020

All comments addressed. Thanks for the feedback!

caithagoras force-pushed the s1 branch 4 times, most recently from fba7572 to 5e34041 Compare

February 7, 2020 22:21

rongrong reviewed

View reviewed changes

presto-docs/src/main/sphinx/admin/function-namespace-managers.rst Outdated

    
              Function namespace managers support storing and retrieving SQL

              functions, allowing Presto engine to perform actions such as creating,

              altering, deleting, and executing functions.

Contributor

rongrong Feb 7, 2020

"executing functions" is not new and it's not something "allowed" by function namespace managers.

Contributor Author

caithagoras Feb 7, 2020 •

edited

Loading

I intend to say "resolving functions"

Contributor

rongrong Feb 7, 2020

resolving function is also not introduced by function namespace manager though.

presto-docs/src/main/sphinx/admin/function-namespace-managers.rst Show resolved Hide resolved

caithagoras force-pushed the s1 branch 3 times, most recently from 975a5c9 to 177bd36 Compare

February 7, 2020 22:59

sachdevs reviewed

View reviewed changes

Contributor

sachdevs left a comment

The function namespace part is very understandable. Looks good. Couple of minor suggested changes but overall seems very clear.

presto-docs/src/main/sphinx/admin/function-namespace-managers.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/admin/function-namespace-managers.rst Outdated

    
              name. A function is uniquely identified by its qualified function name

              and parameter type list.

              Each instance of function namespace manager binds to a catalog name

Contributor

sachdevs Feb 7, 2020

of a function namespace manager

Contributor Author

caithagoras Feb 7, 2020 •

edited

Loading

When we're saying a function namespace manager, we're referencing an instance, not a type of function namespace manager. So, maybe, I should just say each Each function namespace manager....

presto-docs/src/main/sphinx/admin/function-namespace-managers.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/sql/create-function.rst Outdated Show resolved Hide resolved

caithagoras0 added 2 commits

February 7, 2020 15:21


          Introduce db based function namespace manager

ad0617d


          Introduce MySql-based function namespace manager

50c4364

Also, include function namespace managers plugin in Presto server

caithagoras force-pushed the s1 branch 5 times, most recently from e754e1f to 48e5619 Compare

February 7, 2020 23:30


          Add documentation for SQL functions

27de387

- Add documentation for CREATE FUNCTION, ALTER FUNCTION, and DROP
  FUNCTION statements.
- Add documentation for Function Namespace Managers configuration.

caithagoras force-pushed the s1 branch from 48e5619 to 27de387 Compare

February 7, 2020 23:33

rongrong approved these changes

View reviewed changes

Contributor

rongrong left a comment

Thanks for the contribution!

Contributor

rongrong commented Feb 7, 2020

For the release note, I'd mention "function namespace manager" and refer to the documentation added.

caithagoras merged commit 44c044a into prestodb:master

caithagoras deleted the s1 branch

February 8, 2020 03:47

caithagoras mentioned this pull request

Add release notes for 0.232 #14130

Merged

8 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet