Skip to content

Adding presto-pinot with pushdown working#13504

Merged
highker merged 1 commit intoprestodb:masterfrom
agrawaldevesh:devesh-pinot-test
Oct 26, 2019
Merged

Adding presto-pinot with pushdown working#13504
highker merged 1 commit intoprestodb:masterfrom
agrawaldevesh:devesh-pinot-test

Conversation

@agrawaldevesh
Copy link
Contributor

@agrawaldevesh agrawaldevesh commented Oct 5, 2019 •
edited by highker
Loading

Adding presto-pinot with pushdown

This presto-pinot connector works as follows:

  • It builds upon the new plan connector optimizer framework to push the
    maximal query to the pinot system.
  • By default, aggregation and limit queries are send to the pinot broker,
    while just filter queries are send to the servers parallelly. This is
    useful when joining against another (say hive) table wherein the join
    needs to be parallelized and segments from pinot (fetched directly from
    the pinot servers) need to be joined with hive splits. If you wish to
    not send any queries to the broker, please set the catalog session
    property prefer_broker_queries to false.
  • You can disable parallel queries (useful for low latency deployment)
    we do at Uber, by setting the catalog property
    forbid_segment_queries=false.
  • The connector needs a way to talk to the controller (to fetch the
    brokers and the table schema). You can either provide the controller
    URI or the URI of the Muttley proxy (an RPC proxy mechanism we use at
    Uber). It is also possible to use the pinot-rest-proxy instead of
    going to the pinot-broker (again something we use for our test pinot
    clusters at Uber).

In addition the pinot connector would split the filter expressions such
that expressions that can be pushed down to pinot are done so, and those
that aren't are handled in presto.

It handles the full range of pinot PQL and respects pinot's weirdnesses
to guarantee that all results are still in the expected SQL semantics.

Usage: See the newly added connector/pinot.rst documentation file.

== RELEASE NOTES ==

Pinot Changes
* Added a new Pinot connector

SPI Changes
* Split `ConnectorPlanOptimizerProvider` to have two phases for connectors to participate query optimization: `LOGICAL` and `PHYSICAL`. The two phases correspond to pre- and post-shuffle optimization respectively.

@highker highker self-requested a review October 6, 2019 05:33
@agrawaldevesh agrawaldevesh force-pushed the devesh-pinot-test branch 2 times, most recently from 9ebfaa7 to ae9bb18 Compare October 9, 2019 06:30
highker
highker reviewed Oct 9, 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.

Will be unblocked by #13521

@agrawaldevesh agrawaldevesh force-pushed the devesh-pinot-test branch 3 times, most recently from ab074c2 to 563b1a5 Compare October 19, 2019 02:43
@agrawaldevesh
Copy link
Contributor Author

agrawaldevesh commented Oct 19, 2019 •
edited
Loading

@highker @wenleix

I think this PR is ready enough to start to be reviewed.

It depends on #13521. It also subsumes #13413 and has implemented all of the changes suggested in there. (cc: @haibow)

I know it is kind of big .. it has almost no changes to the presto-main except:

  • A way to run the ApplyConnectorOptimization pre exchange and tell it that it is being run pre exchange. Currently only the presto-pinot connector is implementing the pre-exchange optimizer. So it does not change the behavior for others that still run post exchanges.
  • It adds some getters for unit testing

The build will fail until https://github.com/agrawaldevesh/presto-pinot-driver can be pushed to mavencentral and the presto-pinot-driver version upgraded to 0.1.0. I am working with @wenleix on that.

I am also happy to meet in person and give a code overview. But a good place to start is from the PinotConnectorPlanOptimizer.java file.

@highker highker changed the title [WIP DO NOT REVIEW] Adding presto-pinot with pushdown working Adding presto-pinot with pushdown working Oct 19, 2019
@highker highker requested review from highker and wenleix October 19, 2019 03:20
@highker highker mentioned this pull request Oct 19, 2019
Closed
@sachdevs sachdevs self-requested a review October 19, 2019 17:44
@highker highker requested a review from hellium01 October 20, 2019 06:17
highker
highker reviewed Oct 20, 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.

High-level comments only. The dependency looks much cleaner!

hellium01
hellium01 reviewed Oct 22, 2019
View reviewed changes
@agrawaldevesh agrawaldevesh mentioned this pull request Oct 22, 2019
Merged
@agrawaldevesh
Copy link
Contributor Author

agrawaldevesh commented Oct 22, 2019 via email

@highker
Copy link

highker commented Oct 22, 2019

It is ok to have many top-level modules

@highker highker requested a review from zhenxiao October 22, 2019 22:45
@highker highker self-requested a review October 23, 2019 01:40
sachdevs
sachdevs reviewed Oct 23, 2019
View reviewed changes
Copy link
Contributor

@sachdevs sachdevs left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @agrawaldevesh! A couple of high level comments to get started:

highker
highker reviewed Oct 23, 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.

Finished reviewing classes PinotExpression, PinotFilterExpressionConverter, PinotProjectExpressionConverter, and PinotQueryGenerator

sachdevs
sachdevs reviewed Oct 23, 2019
View reviewed changes
@agrawaldevesh
Copy link
Contributor Author

agrawaldevesh commented Oct 23, 2019

@highker @sachdevs ... Thanks a lot for the detailed review !

I have incorporated your feedback and updated the code.

highker
highker reviewed Oct 24, 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.

@agrawaldevesh, there are still some amount of comments not addressed? Could you go through those in https://github.com/prestodb/presto/pull/13504/files ? Make sure to open each file and exam the comment.

highker
highker reviewed Oct 24, 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 many nits and small coding style to be fixed. I only skimmed through the files rather than digging into each individual. @agrawaldevesh, could do a thorough exam on all the files to make sure the coding style is right based on the existing comments? Thanks!

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PushdownUtils.java Outdated
Copy link

@highker highker Oct 24, 2019

Choose a reason for hiding this comment

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

Let's call this PinotPushdownUtils

Copy link
Contributor Author

@agrawaldevesh agrawaldevesh Oct 24, 2019

Choose a reason for hiding this comment

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

Hmm ... something to discuss about: I want to use these for presto-aresdb connector too. We originally had them in presto-spi as a part of our fork. Is there a designated common module that connectors can share code in ? Or I can always have the presto-aresdb-toolkit depend on presto-pinot-toolkit but that would look weird.

It would be funny to create another module named presto-pinot-aresdb-utils to house these :)

But I can rename them to something pinot specific for now but this is generic.

@agrawaldevesh
Copy link
Contributor Author

agrawaldevesh commented Oct 24, 2019

@highker ... I just did a pass over the entire patch and I think you will find that a lot of your common nits have been addressed :-)

hellium01
hellium01 reviewed Oct 24, 2019
View reviewed changes
Copy link
Contributor

@hellium01 hellium01 left a comment

Choose a reason for hiding this comment

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

Some initial comments.

...bc/src/main/java/com/facebook/presto/plugin/jdbc/optimization/JdbcPlanOptimizerProvider.java Outdated
Copy link
Contributor

@hellium01 hellium01 Oct 24, 2019

Choose a reason for hiding this comment

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

Another way is to add a getStage() method in ConnectorPlanOptimizer but current design is fine given we won't (probably shouldn't) have more than two stages.

...t-toolkit/src/test/java/com/facebook/presto/pinot/query/TestPinotConnectorPlanOptimizer.java Outdated
Copy link
Contributor

@hellium01 hellium01 Oct 24, 2019

Choose a reason for hiding this comment

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

This comment might not be necessary?

...t-toolkit/src/test/java/com/facebook/presto/pinot/query/TestPinotConnectorPlanOptimizer.java Outdated
Copy link
Contributor

@hellium01 hellium01 Oct 24, 2019

Choose a reason for hiding this comment

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

testPartialPredicatePushdown?

...ot-toolkit/src/main/java/com/facebook/presto/pinot/query/PinotFilterExpressionConverter.java Outdated
Copy link
Contributor

@hellium01 hellium01 Oct 24, 2019

Choose a reason for hiding this comment

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

Do we want to put parenthesis around sub clauses:

format("( (%s) %s (%s))", ...)

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/query/PinotQueryGenerator.java Outdated
int aggregations = 0;
boolean groupByExists = false;

for (AggregationColumnNode expression : computeAggregationNodes(node).orElseThrow(() -> new PinotException(PINOT_UNSUPPORTED_EXPRESSION, Optional.empty(), "Unsupported aggregation node " + node))) {
Copy link
Contributor

@hellium01 hellium01 Oct 24, 2019

Choose a reason for hiding this comment

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

nit: instead of lengthy computeAggregationNodes orElseThrow, can we just throw inside computeAggregationNode?

@highker highker self-requested a review October 24, 2019 23:20
highker
highker reviewed Oct 25, 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.

The failure is related. Could you fix it really quick?

highker
highker reviewed Oct 25, 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.

@agrawaldevesh, could you update your commit message body with the following text? (Basically I just wrap your words into 70 chars per line and removed some out-of-date statement)

Adding presto-pinot with pushdown

This presto-pinot connector works as follows:
* It builds upon the new plan connector optimizer framework to push the
  maximal query to the pinot system.
* By default, aggregation and limit queries are send to the pinot broker,
  while just filter queries are send to the servers parallelly. This is
  useful when joining against another (say hive) table wherein the join
  needs to be parallelized and segments from pinot (fetched directly from 
  the pinot servers) need to be joined with hive splits. If you wish to 
  not send any queries to the broker, please set the catalog session 
  property prefer_broker_queries to false.
* You can disable parallel queries (useful for low latency deployment) 
  we do at Uber, by setting the catalog property
  forbid_segmnt_queries=false.
* The connector needs a way to talk to the controller (to fetch the 
  brokers and the table schema). You can either provide the controller 
  URI or the URI of the Muttley proxy (an RPC proxy mechanism we use at
  Uber). It is also possible to use the pinot-rest-proxy instead of 
  going to the pinot-broker (again something we use for our test pinot
  clusters at Uber).

In addition the pinot connector would split the filter expressions such
that expressions that can be pushed down to pinot are done so, and those
that aren't are handled in presto.

It handles the full range of pinot PQL and respects pinot's weirdnesses
to guarantee that all results are still in the expected SQL semantics.

Usage:
Place the catalog file like so in the etc/catalog directory. The schema
name is immaterial and it can be anything. (since pinot does not have a
notion of schema). Example catalog files can be found below:

$ cat local_run/etc/catalog/pinotnotparallel.properties
connector.name=pinot
pinot.controller-url=controller_host:9000
pinot.scan-parallelism-enabled=false

$ cat local_run/etc/catalog/pinotparallel.properties
connector.name=pinot
pinot.controller-url=controller_host:9000
pinot.scan-parallelism-enabled=true

highker
highker approved these changes Oct 25, 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.

LGTM!

@highker highker mentioned this pull request Oct 25, 2019
Closed
3 tasks
Adding presto-pinot with pushdown
95b935b 
This presto-pinot connector works as follows:
* It builds upon the new plan connector optimizer framework to push the
  maximal query to the pinot system.
* By default, aggregation and limit queries are send to the pinot broker,
  while just filter queries are send to the servers parallelly. This is
  useful when joining against another (say hive) table wherein the join
  needs to be parallelized and segments from pinot (fetched directly from
  the pinot servers) need to be joined with hive splits. If you wish to
  not send any queries to the broker, please set the catalog session
  property prefer_broker_queries to false.
* You can disable parallel queries (useful for low latency deployment)
  we do at Uber, by setting the catalog property
  forbid_segment_queries=false.
* The connector needs a way to talk to the controller (to fetch the
  brokers and the table schema). You can either provide the controller
  URI or the URI of the Muttley proxy (an RPC proxy mechanism we use at
  Uber). It is also possible to use the pinot-rest-proxy instead of
  going to the pinot-broker (again something we use for our test pinot
  clusters at Uber).

In addition the pinot connector would split the filter expressions such
that expressions that can be pushed down to pinot are done so, and those
that aren't are handled in presto.

It handles the full range of pinot PQL and respects pinot's weirdnesses
to guarantee that all results are still in the expected SQL semantics.

Usage: See the newly added connector/pinot.rst documentation file.
@agrawaldevesh
Copy link
Contributor Author

agrawaldevesh commented Oct 26, 2019

Tested and fixed. Good to be merged in.

@highker highker merged commit 78f2470 into prestodb:master Oct 26, 2019
agrawaldevesh pushed a commit to agrawaldevesh/incubator-pinot that referenced this pull request Oct 29, 2019
[New Transformation function] DateTrunc: Identical semantics to prest…
a6e01b2 
…o's date_trunc

This will only be called by Presto connector (prestodb/presto#13504) and it has identically semantics
to presto's SQL date_trunc, albeit with a timezone specialization.

Please see details on presto's date_trunc function here:
https://prestodb.github.io/docs/current/functions/datetime.html

This is needed so that the presto's date_trunc invocations can be
faithfully translated as is to this new function. Its a new function so
that it is trivial to roll out to harmlessly without a lot of regression
testing.

Without this function, we cannot handle timezones nor week truncations
in the existing pinot's dateTimeConvert function.

It basically copies the PrestoDB code:
https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/operator/scalar/DateTimeFunctions.java,
(specializations of date_trunc for TIMESTAMP and
TIMESTAMP_WITH_TIME_ZONE

I am even checking in the zone-index.properties used by presto to ensure that
even the time zones are 1:1 b/w presto and this function. (sync'd to the
latest prestodb/presto repo)

Understanding this UDF requires knowledge of the joda-time API. I am not
documenting this heavily since it is a copy of the Presto UDF.
Jackie-Jiang pushed a commit to apache/pinot that referenced this pull request Oct 29, 2019
@Jackie-Jiang
[New Transformation function] DateTrunc: Identical semantics to prest…
4f849ea 
…o's date_trunc (#4740)

This will only be called by Presto connector (prestodb/presto#13504) and it has identically semantics
to presto's SQL date_trunc, albeit with a timezone specialization.

Please see details on presto's date_trunc function here:
https://prestodb.github.io/docs/current/functions/datetime.html

This is needed so that the presto's date_trunc invocations can be
faithfully translated as is to this new function. Its a new function so
that it is trivial to roll out to harmlessly without a lot of regression
testing.

Without this function, we cannot handle timezones nor week truncations
in the existing pinot's dateTimeConvert function.

It basically copies the PrestoDB code:
https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/operator/scalar/DateTimeFunctions.java,
(specializations of date_trunc for TIMESTAMP and
TIMESTAMP_WITH_TIME_ZONE

I am even checking in the zone-index.properties used by presto to ensure that
even the time zones are 1:1 b/w presto and this function. (sync'd to the
latest prestodb/presto repo)

Understanding this UDF requires knowledge of the joda-time API. I am not
documenting this heavily since it is a copy of the Presto UDF.
chenboat pushed a commit to chenboat/incubator-pinot that referenced this pull request Nov 9, 2019
@chenboat
[New Transformation function] DateTrunc: Identical semantics to prest…
17a925a 
…o's date_trunc (apache#4740)

This will only be called by Presto connector (prestodb/presto#13504) and it has identically semantics
to presto's SQL date_trunc, albeit with a timezone specialization.

Please see details on presto's date_trunc function here:
https://prestodb.github.io/docs/current/functions/datetime.html

This is needed so that the presto's date_trunc invocations can be
faithfully translated as is to this new function. Its a new function so
that it is trivial to roll out to harmlessly without a lot of regression
testing.

Without this function, we cannot handle timezones nor week truncations
in the existing pinot's dateTimeConvert function.

It basically copies the PrestoDB code:
https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/operator/scalar/DateTimeFunctions.java,
(specializations of date_trunc for TIMESTAMP and
TIMESTAMP_WITH_TIME_ZONE

I am even checking in the zone-index.properties used by presto to ensure that
even the time zones are 1:1 b/w presto and this function. (sync'd to the
latest prestodb/presto repo)

Understanding this UDF requires knowledge of the joda-time API. I am not
documenting this heavily since it is a copy of the Presto UDF.
This was referenced Nov 11, 2019
Closed
Closed
Closed
Closed
Merged
chenboat pushed a commit to chenboat/incubator-pinot that referenced this pull request Nov 15, 2019
@chenboat
[New Transformation function] DateTrunc: Identical semantics to prest…
85b6128 
…o's date_trunc (apache#4740)

This will only be called by Presto connector (prestodb/presto#13504) and it has identically semantics
to presto's SQL date_trunc, albeit with a timezone specialization.

Please see details on presto's date_trunc function here:
https://prestodb.github.io/docs/current/functions/datetime.html

This is needed so that the presto's date_trunc invocations can be
faithfully translated as is to this new function. Its a new function so
that it is trivial to roll out to harmlessly without a lot of regression
testing.

Without this function, we cannot handle timezones nor week truncations
in the existing pinot's dateTimeConvert function.

It basically copies the PrestoDB code:
https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/operator/scalar/DateTimeFunctions.java,
(specializations of date_trunc for TIMESTAMP and
TIMESTAMP_WITH_TIME_ZONE

I am even checking in the zone-index.properties used by presto to ensure that
even the time zones are 1:1 b/w presto and this function. (sync'd to the
latest prestodb/presto repo)

Understanding this UDF requires knowledge of the joda-time API. I am not
documenting this heavily since it is a copy of the Presto UDF.
@aaneja aaneja mentioned this pull request Jul 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hellium01 hellium01 hellium01 left review comments

@haibow haibow haibow left review comments

@highker highker highker approved these changes

@wenleix wenleix Awaiting requested review from wenleix

@zhenxiao zhenxiao Awaiting requested review from zhenxiao

@sachdevs sachdevs Awaiting requested review from sachdevs

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@agrawaldevesh @highker @haibow @hellium01 @sachdevs @facebook-github-bot