Function implementation by rongrong · Pull Request #13384 · prestodb/presto

rongrong · 2019-09-12T06:14:25Z

Ready for pre-review on high level. I'll probably use some data structures introduced in #13484 and add tests once it's merged.

caithagoras · 2019-10-11T04:17:06Z

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

What is preventing us from being able to pass in a Session?

caithagoras · 2019-10-11T04:26:33Z

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

I don't see this class anywhere in the PR.

caithagoras · 2019-10-11T05:00:05Z

In this PR, FunctionLanguage is used to distinguish between built-in and user-defined functions. Rather than tying it with Language, we should give it a different name since it fullfill a different purpose. For example, suppose we support Python and JS in the future, they'll have a different Language, but might still have the same execution strategy of using an external server.

Some naming suggestions:

FunctionImplementationType (this sounds best to me)
FunctionImplementation.Type
FunctionExecutionStrategy

caithagoras · 2019-10-11T05:41:09Z

A few issues with the FunctionImplementation Interface.

Adding FunctionManager.getFunctionImplementation and BuiltInFunctionNamespaceManager.getFunctionImplementation seems to aim for unifying function implementation fetching, since the names are very generic, but that also makes them confusing since they're only fetching scalar functions.
I checked all callers of FunctionImplementation.getFunctionLanguage. This method is never actually used to make decisions - it is only used to perform sanity checks, which can be achieved by properly designing the interfaces.
FunctionImplementation#getImplementation returns an Object. This indicates something can be improved (even returning an empty interface is better than that).

Here is my proposal:

All decisions are made using FunctionMetadata.getFunctionImplemenationType if needed (you're already doing this in this PR)
Either
(*) Introduce a new method FunctionNamespace.getSqlInvokedFunctionImplementation, mimicking the 3 existing getters, doing the checks and the down cast, and returning a SqlInvokedRegularFunctionImplemenation.
Or
(**) FunctionManager.getFunctionImplementation and FunctionNamespaceManager.getFunctionImplementation needs to include scalar, aggregation, and window, to at least match the expected behavior as indicated by the method name.
Remove the 2 methods in FunctionImplementation to make it an empty
interface.
- When you have checkArgument(implementation.getFunctionLanguage() == SQL), that means what you should really pass in instead is a SqlInvokedRegularFunctionImplemenation instance. (Most of cases are in SqlFunctionUtils)
- If you change all those parameters, I think you'll end up having to do one down-cast right after you fetch the implementation from FunctionManager if you do (**), or no cast at all if you do (*). And I think it is a better practice to cast early in order to fail early.

rongrong · 2019-10-11T20:01:16Z

In this PR, FunctionLanguage is used to distinguish between built-in and user-defined functions. Rather than tying it with Language, we should give it a different name since it fullfill a different purpose. For example, suppose we support Python and JS in the future, they'll have a different Language, but might still have the same execution strategy of using an external server.

Some naming suggestions:

FunctionImplementationType (this sounds best to me)

FunctionImplementation.Type

FunctionExecutionStrategy

Ya, it makes less and less sense to have this FunctionLanguage especially after the syntax support. The "language" here should theoretically be associated with the "execution engine" (how to run them) so I'll take FunctionExecutionStrategy. If you have more ideas let me know. I'm terrible with names. 🤣

rongrong · 2019-10-11T20:04:48Z

A few issues with the FunctionImplementation Interface.

Adding FunctionManager.getFunctionImplementation and BuiltInFunctionNamespaceManager.getFunctionImplementation seems to aim for unifying function implementation fetching, since the names are very generic, but that also makes them confusing since they're only fetching scalar functions.

I checked all callers of FunctionImplementation.getFunctionLanguage. This method is never actually used to make decisions - it is only used to perform sanity checks, which can be achieved by properly designing the interfaces.

FunctionImplementation#getImplementation returns an Object. This indicates something can be improved (even returning an empty interface is better than that).

Here is my proposal:

All decisions are made using FunctionMetadata.getFunctionImplemenationType if needed (you're already doing this in this PR)

Either
(*) Introduce a new method FunctionNamespace.getSqlInvokedFunctionImplementation, mimicking the 3 existing getters, doing the checks and the down cast, and returning a SqlInvokedRegularFunctionImplemenation.
Or
(**) FunctionManager.getFunctionImplementation and FunctionNamespaceManager.getFunctionImplementation needs to include scalar, aggregation, and window, to at least match the expected behavior as indicated by the method name.

Remove the 2 methods in FunctionImplementation to make it an empty
interface.

When you have checkArgument(implementation.getFunctionLanguage() == SQL), that means what you should really pass in instead is a SqlInvokedRegularFunctionImplemenation instance. (Most of cases are in SqlFunctionUtils)

If you change all those parameters, I think you'll end up having to do one down-cast right after you fetch the implementation from FunctionManager if you do (**), or no cast at all if you do (*). And I think it is a better practice to cast early in order to fail early.

We sort of had a conversation about unifying the metadata and implementation but seems like they should still be two separate interfaces. At the point I'm inclined to have the implementation to be an empty interface. I'm thinking of maybe calling it ScalarFunctionImplementation to get away with the integration / window function issues. Problem with unifying them all is that their internal representation for builtin functions are very different.

highker

One comment on ScalarFunctionImplementation; otherwise looks good to me.

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

highker · 2019-10-20T04:23:10Z

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

This design pattern doesn't look at an "Interface design pattern" but more like a "delegation". FunctionImplementationType should be good enough for caller to decide who to delegate. This interface may not be necessary. In the worst case, presto-main depends on presto-sql-function, parseSqlFunctionExpression can down cast it:

((SqlInvokedRegularSqlFunctionImplementation) functionImplementation).getImplementation()

where only SqlInvokedRegularSqlFunctionImplementation needs a String getImplementation() method.

presto-main/src/main/java/com/facebook/presto/sql/relational/SqlFunctionUtils.java

rongrong · 2019-10-23T21:49:31Z

Addressed comments. Please review. Thanks! @caithagoras @highker

highker

LGTM; I don't have other comments

Since RowExpression could include CallExpression that are defined in sql text, which would be parsed to different expressions depending on SqlFunctionProperties, we need to include SqlFunctionProperties in the cache keys.

…ation

caithagoras · 2019-10-29T02:11:13Z

Commit title Rename ScalarFunctionImplementation to BuiltInScalarFunctionImplement… exceeds 72 characters and has been cut off.

rongrong · 2019-10-29T03:46:05Z

Commit title Rename ScalarFunctionImplementation to BuiltInScalarFunctionImplement… exceeds 72 characters and has been cut off.

What can I change it to? 🤣

rongrong force-pushed the function-implementation branch from c525055 to b08514e Compare September 12, 2019 06:19

facebook-github-bot added the CLA Signed label Sep 12, 2019

rongrong force-pushed the function-implementation branch 5 times, most recently from 24dd8de to 2e31a25 Compare September 14, 2019 01:19

rongrong force-pushed the function-implementation branch 6 times, most recently from 66ce470 to bf8ed19 Compare October 9, 2019 01:08

rongrong changed the title ~~[POC] Function implementation~~ Function implementation Oct 9, 2019

rongrong requested review from arhimondr, caithagoras, highker and wenleix October 9, 2019 01:15

caithagoras self-assigned this Oct 9, 2019

rongrong force-pushed the function-implementation branch 3 times, most recently from 15aab08 to 6bb4075 Compare October 10, 2019 23:50

caithagoras reviewed Oct 11, 2019

View reviewed changes

rongrong force-pushed the function-implementation branch from 6bb4075 to d80e9eb Compare October 11, 2019 19:51

rongrong force-pushed the function-implementation branch from d80e9eb to ecd86a9 Compare October 11, 2019 21:35

highker requested review from arhimondr, caithagoras and highker October 18, 2019 18:19

highker reviewed Oct 20, 2019

View reviewed changes

rongrong force-pushed the function-implementation branch 3 times, most recently from c5a897e to 9b6a769 Compare October 23, 2019 19:16

highker self-requested a review October 24, 2019 04:12

highker approved these changes Oct 24, 2019

View reviewed changes

rongrong added 11 commits October 28, 2019 17:44

Remove TypeProvider from LocalExecutionPlanContext

b4069ba

Check call convention once in ExpressionInterpreter.visitFunctionCall

9cb7095

Check call convention once in RowExpressionInterpreter.visitCall

a66271b

Fix error message in FunctionManager

43b0d90

Factor out Expression related session configs into SqlFunctionProperties

7c704ee

Remove Session from SqlToRowExpressionTranslator

281c0a4

Add SqlFunctionProperties to bytecode compiler cache keys

56bb187

Since RowExpression could include CallExpression that are defined in sql text, which would be parsed to different expressions depending on SqlFunctionProperties, we need to include SqlFunctionProperties in the cache keys.

Rename ScalarFunctionImplementation to BuiltInScalarFunctionImplement…

5fde5d4

…ation

Add funcion implementation type and argument names to FunctionMetadata

8e8a592

Add ScalarFunctionImplementation abstraction

349f879

Support SQL function execution

6563a19

rongrong force-pushed the function-implementation branch from 9b6a769 to 6563a19 Compare October 29, 2019 00:57

rongrong merged commit 694bd5d into prestodb:master Oct 29, 2019

Conversation

rongrong commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caithagoras Oct 11, 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!

caithagoras Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

rongrong commented Oct 11, 2019

Uh oh!

rongrong commented Oct 11, 2019

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

highker Oct 20, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rongrong commented Oct 23, 2019

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

caithagoras commented Oct 29, 2019

Uh oh!

rongrong commented Oct 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rongrong commented Sep 12, 2019 •

edited

Loading

caithagoras Oct 11, 2019 •

edited

Loading

caithagoras commented Oct 11, 2019 •

edited

Loading

caithagoras commented Oct 11, 2019 •

edited

Loading