Skip to content

Move FunctionHandle to SPI#12432

Merged
hellium01 merged 1 commit intoprestodb:masterfrom
hellium01:AdjustFunctionHandle
Mar 8, 2019
Merged

Move FunctionHandle to SPI#12432
hellium01 merged 1 commit intoprestodb:masterfrom
hellium01:AdjustFunctionHandle

Conversation

@hellium01
Copy link
Contributor

@hellium01 hellium01 commented Mar 5, 2019 •
edited
Loading

This PR moves FunctionHandle and its dependency to SPI. This PR depends on #12427. This is the first part of a larger PR: #12375

@hellium01 hellium01 requested a review from highker March 5, 2019 19:01
highker
highker reviewed Mar 5, 2019
View reviewed changes
presto-benchmark/src/main/java/com/facebook/presto/benchmark/PredicateFilterSqlBenchmark.java Outdated
Copy link

@highker highker Mar 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alter back to lower cases...

presto-spi/src/main/java/com/facebook/presto/spi/function/FunctionHandle.java Outdated
Copy link

@highker highker Mar 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test -> condition

presto-spi/src/main/java/com/facebook/presto/spi/function/SignatureUtils.java Outdated
Copy link

@highker highker Mar 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to InternalSignatureUtils.java

@highker
Copy link

highker commented Mar 5, 2019 •
edited
Loading

cc: @vkorukanti @vivekb @zhenxiao @vineeth

@hellium01 hellium01 force-pushed the AdjustFunctionHandle branch 2 times, most recently from 442fed1 to ae2535b Compare March 6, 2019 03:06
highker
highker approved these changes Mar 6, 2019
View reviewed changes
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge after #12427

presto-main/src/test/java/com/facebook/presto/type/TestTypeRegistry.java Outdated
Copy link

@highker highker Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify why this line is auto optimized...

Copy link
Contributor Author

@hellium01 hellium01 Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unchanged in my commit?

Copy link
Contributor

@rongrong rongrong Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should not be here. I removed the commit that introduces CastType.

rongrong
rongrong reviewed Mar 6, 2019
View reviewed changes
presto-spi/src/main/java/com/facebook/presto/spi/function/FunctionHandle.java Outdated
Copy link
Contributor

@rongrong rongrong Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less also add a @Unstable annotation?

Copy link
Contributor Author

@hellium01 hellium01 Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot use @Unstable or @Beta in spi because they are from guava. But I can add a @apiNote in the JavaDoc here.

@rongrong
Copy link
Contributor

rongrong commented Mar 6, 2019

As discussed offline, I'd prefer we only move FunctionHandle and Signature to SPI for now. Adding SignatureBuilder seems to bring too much unnecessary code into SPI. We can reevaluate later if needed. Please also Add annotation to FunctionHandle to make it more visible that this is likely to change in the near future. Thanks!

@hellium01 hellium01 force-pushed the AdjustFunctionHandle branch 3 times, most recently from 0798665 to 5190c0e Compare March 7, 2019 02:00
highker
highker reviewed Mar 7, 2019
View reviewed changes
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some extra unnecessary smartness introduced by IntelliJ

presto-main/src/main/java/com/facebook/presto/metadata/FunctionInvokerProvider.java Outdated
Copy link

@highker highker Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check why we have two extra imports....

presto-main/src/main/java/com/facebook/presto/metadata/FunctionManager.java Outdated
Copy link

@highker highker Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here..

presto-main/src/main/java/com/facebook/presto/metadata/SqlAggregationFunction.java Outdated
Copy link

@highker highker Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same..

highker
highker approved these changes Mar 7, 2019
View reviewed changes
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, those imports are due to package move

rongrong
rongrong reviewed Mar 7, 2019
View reviewed changes
presto-main/src/main/java/com/facebook/presto/metadata/SignatureBuilder.java Outdated
Copy link
Contributor

@rongrong rongrong Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably no longer needed if you are not moving it to SPI.

Copy link

@highker highker Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

rongrong
rongrong reviewed Mar 7, 2019
View reviewed changes
presto-main/src/main/java/com/facebook/presto/metadata/SignatureBuilder.java Outdated
Copy link
Contributor

@rongrong rongrong Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

highker
highker reviewed Mar 7, 2019
View reviewed changes
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message body:

Move FunctionHandle to SPI for preparation of allowing connectors
participating query plan optimization. Signature helps connectors to
resolve functions.

@hellium01 hellium01 force-pushed the AdjustFunctionHandle branch 2 times, most recently from 03d7ae4 to d260698 Compare March 7, 2019 19:10
@hellium01
Move FunctionHandle to SPI
d898fe5 
Move FunctionHandle to SPI for preparation of allowing connectors
participating query plan optimization. Signature helps connectors to
resolve functions.
@hellium01 hellium01 force-pushed the AdjustFunctionHandle branch from d260698 to d898fe5 Compare March 7, 2019 22:41
@hellium01 hellium01 merged commit ad99f43 into prestodb:master Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rongrong rongrong rongrong left review comments

@highker highker highker approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hellium01 @highker @rongrong @facebook-github-bot