Presto: Window function dependencies not in source plan output

[electrum]

with t1 as (
  select orderkey, totalprice
  from tpch.tiny.orders
),
t2 as (
  select orderkey, totalprice, sum(totalprice) over () x
  from t1
),
t3 as (
  select orderkey, max(x) over()
  from t2
)
select * from t3

```
java.lang.IllegalArgumentException: Invalid node. Window function dependencies ([sum]) not in source plan output ([orderkey])
at com.google.common.base.Preconditions.checkArgument(Preconditions.java:145)
at com.facebook.presto.sql.planner.sanity.ValidateDependenciesChecker.checkDependencies(ValidateDependenciesChecker.java:645)
at com.facebook.presto.sql.planner.sanity.ValidateDependenciesChecker.access$100(ValidateDependenciesChecker.java:80)
at com.facebook.presto.sql.planner.sanity.ValidateDependenciesChecker$Visitor.visitWindow(ValidateDependenciesChecker.java:187)
at com.facebook.presto.sql.planner.sanity.ValidateDependenciesChecker$Visitor.visitWindow(ValidateDependenciesChecker.java:94)
at com.facebook.presto.sql.planner.plan.WindowNode.accept(WindowNode.java:154)
at com.facebook.presto.sql.planner.sanity.ValidateDependenciesChecker$Visitor.visitOutput(ValidateDependenciesChecker.java:303)
at com.facebook.presto.sql.planner.sanity.ValidateDependenciesChecker$Visitor.visitOutput(ValidateDependenciesChecker.java:94)
at com.facebook.presto.sql.planner.plan.OutputNode.accept(OutputNode.java:82)
at com.facebook.presto.sql.planner.sanity.ValidateDependenciesChecker.validate(ValidateDependenciesChecker.java:91)
at com.facebook.presto.sql.planner.sanity.ValidateDependenciesChecker.validate(ValidateDependenciesChecker.java:86)
at com.facebook.presto.sql.planner.sanity.PlanSanityChecker.lambda$validate$0(PlanSanityChecker.java:44)
at java.lang.Iterable.forEach(Iterable.java:75)
at com.facebook.presto.sql.planner.sanity.PlanSanityChecker.validate(PlanSanityChecker.java:44)
at com.facebook.presto.sql.planner.LogicalPlanner.plan(LogicalPlanner.java:127)
at com.facebook.presto.sql.planner.LogicalPlanner.plan(LogicalPlanner.java:111)
at com.facebook.presto.execution.SqlQueryExecution.doAnalyzeQuery(SqlQueryExecution.java:297)
at com.facebook.presto.execution.SqlQueryExecution.analyzeQuery(SqlQueryExecution.java:276)
at com.facebook.presto.execution.SqlQueryExecution.start(SqlQueryExecution.java:234)
at com.facebook.presto.execution.resourceGroups.InternalResourceGroup.lambda$startInBackground$1(InternalResourceGroup.java:478)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)

[cawallin]

@electrum what was the query that caused this bug?

[electrum]

@cawallin Updated with a reproduction

[cberner]

I bisect'ed this to 29956b7486968596d5f831717194045c77803887 introduced as part of https://github.com/prestodb/presto/pull/6814

[martint]

No need to block the release for this. The feature can be turned off with optimizer.reorder-windows=false.

[maciejgrzybek]

I'll look into it. Keep me assigned.

[maciejgrzybek]

I believe the problem was introduced by MergeWindows (far earlier than ReorderWindows). MergeWindows is not disableable, so I believe it is a RELEASE-BLOCKED in fact.
I'm working on patch.

The problem is windows are merged regardless of their dependencies, so as long as two window nodes are adjacent and have qualifying specification (identical PartitionBy and OrderBy) they will be merged. This is not correct if one depends on output of another.

Output symbols of window nodes should be also checked before merging / reordering.
This (checks for output symbols) was meant to be implemented as part of https://github.com/prestodb/presto/issues/6681.

I will prepare patch today.

[maciejgrzybek]

Fix for the problem is here: https://github.com/prestodb/presto/pull/7298

[maciejgrzybek]

Closing since https://github.com/prestodb/presto/pull/7298 got merged.