-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip the creation of DistinctLimitNode for global aggregation node #437
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good % small comments
@@ -40,7 +40,7 @@ | |||
*/ | |||
private static boolean isDistinct(AggregationNode node) | |||
{ | |||
return node.getAggregations().isEmpty() && | |||
return node.getAggregations().isEmpty() && !node.getGroupingKeys().isEmpty() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move each condition to separate line
@@ -40,7 +40,7 @@ | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests for queries that were failing previously.
If you are solving prestodb/presto#11276, then please add below query to io.prestosql.tests.AbstractTestAggregations
or AbstractTestQueries
:
select count(1) from (select sum(field1) from table limit 10) a;
@@ -134,7 +134,7 @@ public PlanNode visitAggregation(AggregationNode node, RewriteContext<LimitConte | |||
LimitContext limit = context.get(); | |||
|
|||
if (limit != null && | |||
node.getAggregations().isEmpty() && | |||
node.getAggregations().isEmpty() && !node.getGroupingKeys().isEmpty() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move each condition to separate line
01734cf
to
020dd0f
Compare
@kokosing Have resolved the above comments. |
020dd0f
to
c72e056
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of minor changes.
p.limit( | ||
1, | ||
p.aggregation( | ||
(Consumer<PlanBuilder.AggregationBuilder>) aggregationBuilder -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type declaration for the lambda is unnecessary. You can also chain the invocations and shorten the variable name to reduce verbosity:
p.aggregation(builder -> builder
.singleGroupingSet(p.symbol("foo"))
.source(p.values(p.symbol("foo"))))))
@Test | ||
public void testAggregationWithConstantArgumentsOverScalar() | ||
{ | ||
assertQuery("SELECT count(1) FROM (SELECT count(custkey) FROM orders LIMIT 10) a"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space between FROM
and orders
c72e056
to
3955b22
Compare
@martint I have updated the changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I'll merge it after Travis gives the ok.
Currently if there is a global aggregation node with no aggregation functions and if we try to merge with a limit node we end up creating a DistinctLimitNode which fails when executed because there is no grouping key. So we skip the creation of `DistinctLimitNode` for such aggregation nodes.
3955b22
to
854e814
Compare
Currently if there is a global aggregation node with no aggregation functions and if we try to merge with a limit node we end up creating a
DistinctLimitNode
which fails when executed because there is no grouping key. So we skip the creation ofDistinctLimitNode
for such aggregationnodes.
This is will solve this issue (prestodb/presto#11276).