kosiew commented on code in PR #23813:
URL: https://github.com/apache/datafusion/pull/23813#discussion_r3637182849
##########
datafusion/expr/src/utils.rs:
##########
@@ -652,6 +652,106 @@ pub fn find_aggregate_exprs<'a>(exprs: impl
IntoIterator<Item = &'a Expr>) -> Ve
})
}
+/// Returns an error if any of `exprs` nests aggregate or window function calls
+/// in a way that has no physical equivalent: an aggregate call may not contain
+/// another aggregate call (`sum(sum(x))`) or a window call
+/// (`sum(sum(x) OVER ())`), and a window call may not contain another window
+/// call (`sum(sum(x) OVER ()) OVER ()`). The reverse nesting, an aggregate
used
+/// as the argument of a window call (`sum(sum(x)) OVER ()`), is legal: there
the
+/// aggregate is evaluated by the `Aggregate` node and the window function is
+/// evaluated on top of its result.
+///
+/// Such expressions are not valid SQL either, so they are rejected while the
+/// logical plan is built rather than failing later with an error that does not
+/// point back at the original SQL.
+///
+/// Callers do not need to invoke this directly: [`Aggregate::try_new`] and
Review Comment:
Nice improvement. One small thing: I think "every way of building those
nodes" is a bit stronger than what the code guarantees today.
`Window::try_new_with_schema` and `Aggregate::try_new_with_schema` don't
perform this validation, and `Window` can also be constructed directly from its
public fields.
The SQL and `LogicalPlanBuilder` paths introduced in this PR are protected,
so this isn't blocking. I'd suggest narrowing the wording here (and in the PR
description) to those validated construction paths. If the intent is to enforce
this invariant across all public construction paths, it would be good to either
add the checks there as well or document that those constructors expect already
validated expressions.
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -9607,3 +9607,69 @@ SET datafusion.execution.target_partitions = 4;
statement ok
DROP TABLE hits_raw;
+
+# Nested aggregate function calls are rejected during planning
+# issue: https://github.com/apache/datafusion/issues/23812
+statement ok
+CREATE TABLE nested_agg_t AS VALUES (1, 10.0), (1, 20.0), (2, 30.0);
+
+statement error DataFusion error: Error during planning: Aggregate function
calls cannot be nested: 'sum\(nested_agg_t\.column2\)' is nested inside
'sum\(sum\(nested_agg_t\.column2\)\)'
+SELECT column1, sum(sum(column2)) FROM nested_agg_t GROUP BY column1;
+
+statement error DataFusion error: Error during planning: Aggregate function
calls cannot be nested: 'count\(nested_agg_t\.column2\)' is nested inside
'sum\(count\(nested_agg_t\.column2\)\)'
+SELECT column1, sum(count(column2)) FROM nested_agg_t GROUP BY column1;
+
+statement error DataFusion error: Error during planning: Aggregate function
calls cannot be nested
+SELECT sum(sum(column2)) FROM nested_agg_t;
+
+statement error DataFusion error: Error during planning: Aggregate function
calls cannot be nested
+SELECT column1 FROM nested_agg_t GROUP BY column1 HAVING sum(sum(column2)) > 0;
+
+statement error DataFusion error: Error during planning: Aggregate function
calls cannot be nested
+SELECT column1, sum(column2 + sum(column2)) FROM nested_agg_t GROUP BY column1;
+
+statement error DataFusion error: Error during planning: Aggregate function
calls cannot be nested
+SELECT sum(column2) FILTER (WHERE sum(column2) > 0) FROM nested_agg_t;
+
+statement error DataFusion error: Error during planning: Aggregate function
calls cannot be nested
+SELECT array_agg(column2 ORDER BY sum(column2)) FROM nested_agg_t;
+
+# A window function nested inside an aggregate is rejected as well
+statement error DataFusion error: Error during planning: Aggregate function
calls cannot contain window function calls
+SELECT sum(sum(column2) OVER ()) FROM nested_agg_t;
+
+# ... as are nested window functions
+statement error DataFusion error: Error during planning: Window function calls
cannot be nested
+SELECT sum(sum(column2) OVER ()) OVER () FROM nested_agg_t;
+
+statement error DataFusion error: Error during planning: Window function calls
cannot be nested
Review Comment:
The coverage here looks great. One additional case that might be worth
adding is a nested window function in `PARTITION BY`, for example:
```sql
row_number() OVER (PARTITION BY row_number() OVER ())
```
`Expr::apply_children` traverses `PARTITION BY`, but the current SQL tests
exercise window arguments and `ORDER BY`. Adding a regression test for
`PARTITION BY` would help protect that traversal path too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
