adriangb opened a new pull request, #23813:
URL: https://github.com/apache/datafusion/pull/23813
## Which issue does this PR close?
- Closes #23812.
## Rationale for this change
Nested aggregate calls such as `sum(sum(x))` were accepted by the planner
and produced a
`LogicalPlan::Aggregate` whose `aggr_expr` contained an inner
`Expr::AggregateFunction` as a
function *argument*. That plan has no physical equivalent, so the query
failed late, during
physical planning, with:
```
This feature is not implemented: Physical plan does not support logical
expression
AggregateFunction(AggregateFunction { func: AggregateUDF { inner: Sum {
signature: Signature { ... } } }, ... })
```
Two problems: the query should be rejected at planning time, and the error
text dumps the `Debug`
formatting of a `Signature` (including the full coercion table) without ever
mentioning nesting,
the offending function, or the offending column.
PostgreSQL rejects these queries with `aggregate function calls cannot be
nested`.
## What changes are included in this PR?
- New `datafusion_expr::utils::check_no_nested_aggregates`, which errors if
any aggregate function
call contains another aggregate function call in its arguments, `FILTER`
or `ORDER BY`.
- `Aggregate::try_new` calls it, so both the SQL path and the
`DataFrame`/`LogicalPlanBuilder` path
are covered, and the failure happens while the logical plan is built.
- The error names both expressions and carries a `Diagnostic` (with a span
pointing into the
original SQL and a help message), for example:
```
Error during planning: Aggregate function calls cannot be nested:
'sum(t.column2)' is nested inside 'sum(sum(t.column2))'
```
The legal cases from the issue are unaffected, as they do not put an
aggregate inside an aggregate:
a scalar function over an aggregate (`abs(sum(x))`), a window function over
an aggregate
(`sum(sum(x)) OVER ()`, which plans as `WindowAggr` over `Aggregate`), and
an aggregate over an
aggregate computed in a subquery.
## Are these changes tested?
Yes:
- unit tests for `check_no_nested_aggregates` (arguments, `FILTER`, `ORDER
BY`, and the legal
window-over-aggregate case) in `datafusion/expr/src/utils.rs`;
- a `LogicalPlanBuilder::aggregate` test covering the non-SQL path;
- SQL planner tests for `SELECT`, `GROUP BY` and `HAVING`, plus a test
asserting the
`sum(sum(x)) OVER ()` plan is unchanged;
- a diagnostic test asserting the span and help message;
- sqllogictests in `aggregate.slt` covering every row of the table in the
issue.
## Are there any user-facing changes?
Queries with nested aggregates now fail during planning with an actionable
error instead of failing
during physical planning with a `NotImplemented` error. No previously
working query changes
behavior.
`datafusion_expr::utils::check_no_nested_aggregates` is a new public
function.
--
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]
