Aggregation Pushdown for Druid connector by zhenxiao · Pull Request #14224 · prestodb/presto

zhenxiao · 2020-03-07T00:27:47Z

== RELEASE NOTES ==

Druid Changes
* Aggregation Pushdown for Druid connector

highker

Could you help going over the patch and fix those nits?

highker · 2020-03-09T22:16:12Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidAggregationProjectConverter.java

highker · 2020-03-09T22:17:05Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidAggregationProjectConverter.java

nit: replace the space with a period a the end.

presto-druid/src/main/java/com/facebook/presto/druid/DruidAggregationProjectConverter.java

highker · 2020-03-09T22:18:02Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidAggregationProjectConverter.java

highker · 2020-03-09T22:23:50Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidPushdownUtils.java

Having a public enum in a util class doesn't sound good. Can you make AggregationColumnNode a top-level class and call it DruidAggregationColumnNode? Put the enum and two implementations as the inner class of it.

highker · 2020-03-09T22:24:38Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

Move to the end of the class

highker · 2020-03-09T22:24:45Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

highker · 2020-03-09T22:25:12Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

leave a space after function:

highker · 2020-03-09T22:25:29Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

typo aggregatin

zhenxiao · 2020-03-10T02:30:12Z

thank you, @highker
comments addressed

luohao

Half way done. Will take a look at the rest.

luohao · 2020-03-10T18:55:55Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

so this refers to query without Group By clause?

query with Group By, but no aggregations, e.g. count(*)

luohao · 2020-03-10T18:59:10Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

Druid seems to support more than just min/max/avg/sum. Some of them should have a mapping to Presto aggreation functions(https://druid.apache.org/docs/latest/querying/sql.html#aggregation-functions), while the others may not apply due to implementation(e.g., need to be careful with HLL).

Do you plan to add other functions?

yep, will add in following PRs. Start with easy ones :)

luohao · 2020-03-10T18:59:44Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

what is PQL?

luohao · 2020-03-10T19:01:47Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

Is count the only aggregation function in this case?

yep, count(*) is the only case

zhenxiao

thank you, @luohao
comments addressed

zhenxiao · 2020-03-10T23:12:56Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

yep, will add in following PRs. Start with easy ones :)

zhenxiao · 2020-03-10T23:13:22Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

query with Group By, but no aggregations, e.g. count(*)

zhenxiao · 2020-03-10T23:13:38Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

yep, count(*) is the only case

highker

nits mostly

highker · 2020-03-16T16:40:12Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidAggregationColumnNode.java

requireNonNull

highker · 2020-03-16T16:41:27Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidAggregationColumnNode.java

requireNonNull

highker · 2020-03-16T16:41:40Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidAggregationColumnNode.java

highker · 2020-03-16T16:43:50Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidAggregationProjectConverter.java

if (!...) { throw new } // remove the `else`

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

highker · 2020-03-16T17:01:43Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

break after .stream()

aggregationNode.getCallExpression().getArguments().stream() .filter(... .map..

highker · 2020-03-16T17:02:43Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGenerator.java

this is always false

oh, my bad. Let me fix it

highker · 2020-03-16T17:07:05Z

presto-druid/src/main/java/com/facebook/presto/druid/DruidQueryGeneratorContext.java

merge this two ifs

presto-druid/src/test/java/com/facebook/presto/druid/TestDruidQueryGenerator.java

highker · 2020-03-16T17:08:03Z

presto-druid/src/test/java/com/facebook/presto/druid/TestDruidQueryGenerator.java

zhenxiao · 2020-03-16T20:54:37Z

thank you @highker
comments addressed.

highker

one nit

presto-druid/src/test/java/com/facebook/presto/druid/TestDruidQueryGenerator.java

zhenxiao · 2020-03-16T23:46:49Z

@luohao am going to merge this PR soon. Could you please take a look, see if any problems?

zhenxiao requested review from highker and luohao March 7, 2020 00:27

highker reviewed Mar 9, 2020

View reviewed changes

zhenxiao force-pushed the druid-aggregation branch from 01dd5e9 to c8e75f8 Compare March 10, 2020 02:28

zhenxiao requested a review from highker March 10, 2020 02:30

luohao reviewed Mar 10, 2020

View reviewed changes

zhenxiao force-pushed the druid-aggregation branch from c8e75f8 to d8b92c3 Compare March 10, 2020 23:12

zhenxiao commented Mar 10, 2020

View reviewed changes

zhenxiao requested a review from luohao March 10, 2020 23:14

highker reviewed Mar 16, 2020

View reviewed changes

zhenxiao force-pushed the druid-aggregation branch from d8b92c3 to d4d132f Compare March 16, 2020 20:52

zhenxiao requested a review from highker March 16, 2020 20:54

highker approved these changes Mar 16, 2020

View reviewed changes

presto-druid/src/test/java/com/facebook/presto/druid/TestDruidQueryGenerator.java Outdated Show resolved Hide resolved

Aggregation Pushdown for Druid connector

36a28ed

zhenxiao force-pushed the druid-aggregation branch from d4d132f to 36a28ed Compare March 16, 2020 23:45

zhenxiao merged commit ad1be42 into prestodb:master Mar 17, 2020

caithagoras mentioned this pull request Mar 29, 2020

Add release notes for 0.234 #14305

Merged

9 tasks

caithagoras mentioned this pull request May 4, 2020

Add release notes for 0.234 #14474

Closed

34 tasks

zhenxiao deleted the druid-aggregation branch May 31, 2020 23:38

Conversation

zhenxiao commented Mar 7, 2020

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

zhenxiao commented Mar 10, 2020

Uh oh!

luohao 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!

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!

zhenxiao 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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhenxiao commented Mar 16, 2020

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!