Multiple PushAggregationThroughOuterJoin issues

[sopel39]

Query:

presto> explain select x, max(x) from (select * from (values (1)) t(x) left join (values (1)) t2(y) on t.x = t2.y) group by x;
Query is gone (server restarted?)

java.lang.IllegalStateException: Cannot resolve symbol field
	at com.google.common.base.Preconditions.checkState(Preconditions.java:585)
	at com.facebook.presto.sql.planner.ExpressionSymbolInliner$Visitor.rewriteSymbolReference(ExpressionSymbolInliner.java:68)
	at com.facebook.presto.sql.planner.ExpressionSymbolInliner$Visitor.rewriteSymbolReference(ExpressionSymbolInliner.java:55)

Diagnosis: PushAggregationThroughOuterJoin should verify that aggregations don't contain probe symbols

With optimization:

presto> select x, count(*) from (select * from (values (1)) t(x) left join (select * from (values (1)) t2(y) where false) t2(y) on true) group by x;
 x | _col1
---+-------
 1 |     0
(1 row)

without:

presto> set session push_aggregation_through_join=false;
SET SESSION
presto> select x, count(*) from (select * from (values (1)) t(x) left join (select * from (values (1)) t2(y) where false) t2(y) on true) group by x;
 x | _col1
---+-------
 1 |     1
(1 row)

Diagnosis: PushAggregationThroughOuterJoin shouldn't fire if default aggregation would be pushed to build side

CC: @rschlussel2

[atris]

@sopel39 Is this issue still applicable?

[sopel39]

@atris yes

[atris]

Cool, can you assign it to me?

…

On Tue, Jun 5, 2018 at 11:43 PM Karol Sobczak ***@***.***> wrote: @atris <https://github.com/atris> yes — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10724 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABpO42qXMXI23v5ioJSsJRnOcbDKTHfHks5t5spRgaJpZM4URJKI> .

-- Regards, Atri *l'apprenant*

[findepi]

@atris we're agile, there is not need for formal assignment. Just ping here that you're working on this (you kinda already did) and go for it!

[atris]

Ack, thanks

[atris]

@findepi @sopel39 For the second part, will not pushing down aggregation when the join is a Left join be the best way to solve the bug? I could not see any regressions in the approach, but wanted to double check with you guys

[sopel39]

The second part only relates to default aggregations pushdown. The rule shouldn't fire If aggregation was non-default (e.g: all grouping sets are non-empty) and would become default when pushed down.

[atris]

@sopel39 When you say default aggregations, do you mean aggregations with default values?

[sopel39]

@atris

I mean aggregations like

select sum(column) from table

, e.g: aggregations with empty group by section. Those produce value even if there are not rows.