Add FunctionHandle as abstraction for function resolution by rongrong · Pull Request #12427 · prestodb/presto

rongrong · 2019-03-04T18:45:38Z

No description provided.

highker

Mostly looks good to me % resolveFunction taking session as a param.

highker · 2019-03-04T18:48:00Z

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

Add a simple javadoc maybe to explain the rationale of this abstraction.

What should I say? There's no documentation on TableHandle either. I assume once all work are done it should be obvious from code what FunctionHandle is. Maybe commit message is a better place to add some documentation?

/** * The necessary metadata to represent the trait of a function. */

Metadata will be a different class. This is just the handle. Eventually this should be something like the TableHandle which is an interface with no methods. And it's up to the function namespace to decide what's the metadata, etc.

How about:

/** * FunctionHandle is the unique handle to identify a function implementation in all * namespaces. */

My understanding is even though FunctionHandle doesn't have to have any detail but we still need to be able to uniquely identify a function by:

the signature of the function (or not?).

which namespace it belong to (or maybe SqlPath?). We probably don't have to know how to make it unique within a namespace.

Then I guess that means we have to have something similar to ConnectorTableHandle for plugin(or namespace to know which function is it)? But that's not the scope of this PR anyway.

I guess the problem is if we actually want FunctionHandle to behave like TableHandle, FunctionHandle should be an interface, and you should have SystemFunctionHandle, HiveFunctionHandle, etc, as the actual classes, etc. Currently that's not what FunctionHandle look like. I added a comment to the commit message. I can add a comment here if you guys want. You might also want to add a comment when you move this to SPI to say "Will likely be changed in the next few releases, think twice when you introduce dependencies." 😝

highker · 2019-03-04T18:53:25Z

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

session is not used. Also, I think we explicitly disallow user to specify what functions to use through session properties (with the exception of some legacy behaviors like array_agg). All SQL functions do not have access to full session properties today.

It's introduced in @dain's original PR: trinodb/trino@7915b62. A technical reason to add it now (even it's not used) is otherwise the two resolveFunction would have exactly the same signature. Though I don't think we will give SQL functions access to session, I think session will just be used to resolve which function to use, so it will only be used within FunctionManager/FunctionNamespace.

SqlPath maybe more favorable here. Since we are not using Session(or SqlPath) at the moment anyway, this may as well be just an interface?

I insist adding a TODO or javadoc here. At least for others to understand why session is not used at the moment.

highker · 2019-03-04T19:30:35Z

presto-main/src/main/java/com/facebook/presto/sql/planner/SymbolAllocator.java

Why suffix here? (not prefix or other parts?)

it's something like catalog.schema.name so it's the last part of the QualifiedName.

ok cool. Maybe add a comment to explain.

I don't think this needs a comment. It's based on the pretty much the definition of QualifiedName. There's nothing special or unexpected. In general I don't think we add comment unless there's something the code itself doesn't explain or doesn't make sense.

I agree with RongRong here, since it is a nameHint, it is quite self explaining why we just use the last part. But to make it more clear, maybe rename it to nameHintForFunction (or not, should be fine for both)?

highker · 2019-03-04T19:33:10Z

...n/java/com/facebook/presto/sql/planner/optimizations/ImplementIntersectAndExceptAsUnion.java

newline after this line

highker · 2019-03-04T19:44:08Z

...in/java/com/facebook/presto/sql/planner/optimizations/OptimizeMixedDistinctAggregations.java

Even the original statement is with two line; but we can squeeze them into one in this case:

if (functionName.equals("count") || functionName.equals("count_if") || functionName.equals("approx_distinct")) {`

highker

Finished the last 3 commits. Overall looks good to me. I will take another pass once we have decided what to do with the session parameter.

highker · 2019-03-05T01:16:46Z

presto-main/src/test/java/com/facebook/presto/metadata/TestFunctionRegistry.java

hmmmm... Keep one with OperatorType and the other omitted may not be proper?

lol i removed both in a later version. Just wait.

highker · 2019-03-05T01:27:00Z

presto-main/src/main/java/com/facebook/presto/operator/scalar/RowComparisonOperator.java

operatorHandle

highker

Reviewed all the commits once again. LGTM.

highker · 2019-03-05T18:51:54Z

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

I insist adding a TODO or javadoc here. At least for others to understand why session is not used at the moment.

Add FunctionHandle abstraction. Eventually FunctionHandle should function similarly to TableHandle, as an abstraction, with implementation details decided by FunctionNamespace. Right now this is just a wrapper on Signature to enable further refactor.

facebook-github-bot added the CLA Signed label Mar 4, 2019

rongrong requested review from hellium01 and highker March 4, 2019 18:45

highker self-assigned this Mar 4, 2019

highker reviewed Mar 4, 2019

View reviewed changes

rongrong force-pushed the function-handle-review branch from f860b43 to 8fc9691 Compare March 5, 2019 00:19

highker reviewed Mar 5, 2019

View reviewed changes

rongrong force-pushed the function-handle-review branch 2 times, most recently from 4e7d225 to 9a797d4 Compare March 5, 2019 06:07

hellium01 mentioned this pull request Mar 5, 2019

Move FunctionHandle to SPI #12432

Merged

highker approved these changes Mar 5, 2019

View reviewed changes

highker assigned rongrong and unassigned highker Mar 6, 2019

rongrong force-pushed the function-handle-review branch from 9a797d4 to 1466285 Compare March 6, 2019 17:36

rongrong added 8 commits March 6, 2019 10:59

Remove unused variable in LocalExecutionPlanner

5358e76

Add FunctionHandle

05bae3b

Add FunctionHandle abstraction. Eventually FunctionHandle should function similarly to TableHandle, as an abstraction, with implementation details decided by FunctionNamespace. Right now this is just a wrapper on Signature to enable further refactor.

Resolve functions to a handle during analysis

7dc031e

Switch window function to use FunctionHandle

7d50083

Switch aggergation function to use FunctionHandle

8d32902

Switch cast to FunctionHandle

f316414

Remove FunctionManager canResolveOperator

7363e31

Switch resolveOperator to FunctionHandle

de71128

rongrong force-pushed the function-handle-review branch from 1466285 to de71128 Compare March 6, 2019 19:00

rongrong closed this Mar 6, 2019

rongrong deleted the function-handle-review branch March 6, 2019 20:00

rongrong merged commit de71128 into prestodb:master Mar 6, 2019

martint mentioned this pull request Mar 15, 2019

Enable Dynamically registered functions #9613

Closed

9 tasks

rongrong added the sql-function label Mar 15, 2019

Conversation

rongrong commented Mar 4, 2019

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellium01 Mar 5, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hellium01 Mar 5, 2019 •

edited

Loading