adriangb opened a new issue, #23812:
URL: https://github.com/apache/datafusion/issues/23812
### Describe the bug
Nested aggregate calls such as `sum(sum(x))` are accepted by the SQL planner
and produce a `LogicalPlan::Aggregate` whose `aggr_expr` contains an inner
`Expr::AggregateFunction` as a *function argument*. That plan is not physically
plannable, so the query fails at physical planning with:
```
This feature is not implemented: Physical plan does not support logical
expression AggregateFunction(AggregateFunction { func: AggregateUDF { inner:
Sum { signature: Signature { type_signature: OneOf([Coercible([Exact {
desired_type: Decimal }]), Coercible([Implicit { desired_type:
Native(LogicalType(Native(UInt64), UInt64)), implicit_coercion:
ImplicitCoercion { allowed_source_types: [ ... ] } }]) ... ]), volatility:
Immutable, parameter_names: None } } }, params: AggregateFunctionParams { args:
[Column(Column { relation: Some(Bare { table: "t" }), name: "column2" })],
distinct: false, filter: None, order_by: [], null_treatment: None } })
```
Two problems:
1. The query should be rejected at planning time, not at execution time.
2. The error text does not tell the user what is wrong with their SQL. It
leaks the `Debug` formatting of a `Signature` (including the full coercion
table), and never mentions nesting, the offending function name, or the
offending column.
This is not specific to UDAFs or to a particular function: built-in
`sum(sum(x))` and `sum(count(x))` both reproduce it.
### To Reproduce
SQL (any table with a numeric column):
```sql
CREATE TABLE t AS VALUES (1, 10.0), (1, 20.0), (2, 30.0);
SELECT column1, sum(sum(column2)) FROM t GROUP BY column1;
```
Full MRE against `datafusion = "54.1.0"`:
```rust
use datafusion::prelude::*;
#[tokio::main]
async fn main() {
let ctx = SessionContext::new();
ctx.sql("CREATE TABLE t AS VALUES (1, 10.0), (1, 20.0), (2, 30.0)")
.await.unwrap().collect().await.unwrap();
// Logical planning succeeds and yields a non-plannable Aggregate.
let df = ctx
.sql("SELECT column1, sum(sum(column2)) FROM t GROUP BY column1")
.await
.expect("logical planning unexpectedly succeeds");
println!("{}", df.logical_plan().display_indent());
// Fails here instead.
let err = df.collect().await.unwrap_err();
println!("{err}");
}
```
The logical plan it prints, with the nested aggregate visible inside `aggr`:
```
Projection: t.column1, sum(sum(t.column2))
Aggregate: groupBy=[[t.column1]], aggr=[[sum(sum(t.column2))]]
TableScan: t
```
Variants I checked on 54.1.0, all with the same outcome (logical plan OK,
physical planning fails):
| Query | Result |
|---|---|
| `SELECT column1, sum(sum(column2)) FROM t GROUP BY column1` | fails at
physical planning |
| `SELECT column1, sum(count(column2)) FROM t GROUP BY column1` | fails at
physical planning |
| `SELECT sum(sum(column2)) FROM t` (no `GROUP BY`) | fails at physical
planning |
| `SELECT column1 FROM t GROUP BY column1 HAVING sum(sum(column2)) > 0` |
fails at physical planning |
| `SELECT column1, abs(sum(column2)) FROM t GROUP BY column1` | works
(correct: scalar over aggregate) |
| `SELECT column1, sum(sum(column2)) OVER () FROM t GROUP BY column1` |
works (correct: window over aggregate) |
### Expected behavior
The nested-aggregate cases should fail during planning with a message naming
the problem. PostgreSQL 17, for comparison:
```
postgres=# SELECT column1, sum(sum(column2)) FROM t GROUP BY column1;
ERROR: aggregate function calls cannot be nested
LINE 1: SELECT column1, sum(sum(column2)) FROM t GROUP BY column1;
^
```
Note that the last two rows of the table above already behave correctly, so
this is narrowly about rejecting the illegal nesting, not about changing
anything else. In particular `sum(sum(x)) OVER ()` is legal in PostgreSQL (a
window function over an aggregate result) and DataFusion already plans it
correctly as `WindowAggr` over `Aggregate` and returns the right answer, so a
fix must not regress that path.
### Additional context
Mechanism, as far as I traced it:
`find_aggregate_exprs`
([`datafusion/expr/src/utils.rs`](https://github.com/apache/datafusion/blob/main/datafusion/expr/src/utils.rs#L649))
collects aggregate expressions via `find_exprs_in_expr`, which returns
`TreeNodeRecursion::Jump` on a match ("Stop recursing down this expr once we
find a match"). For `sum(sum(x))` that yields exactly one expression, the
*outer* `sum(sum(x))`, with the inner aggregate still embedded in its
arguments. `select_to_plan`
([`datafusion/sql/src/select.rs`](https://github.com/apache/datafusion/blob/main/datafusion/sql/src/select.rs))
passes that straight to `LogicalPlanBuilder::aggregate`, and nothing
downstream rejects it. `DefaultPhysicalPlanner` then calls
`create_physical_expr` on the inner `Expr::AggregateFunction` argument, which
has no physical-expression equivalent, producing the `NotImplemented` above.
The `Jump` behaviour looks intentional and useful for the legal cases, so
the natural fix seems to be an explicit check rather than a change to the
traversal: after collecting `aggr_exprs`, verify that no collected aggregate
contains a further `Expr::AggregateFunction` in its arguments, `filter`, or
`order_by`, and return a `plan_err!` naming the outer and inner function.
Adding the same invariant check to `LogicalPlan::Aggregate::try_new` would
additionally cover plans built through the `DataFrame`/`LogicalPlanBuilder`
APIs, though I have only verified the SQL path.
If the approach above sounds right to the maintainers, I am happy to put up
a PR.
Found while triaging user-reported query failures in [Pydantic
Logfire](https://github.com/pydantic/logfire), where our metric helpers are
UDAFs and users reasonably-but-incorrectly write `sum(metric_increase(value,
ts))`. The unhelpful error text is what made it hard to diagnose, since nothing
in it points at the user's SQL.
--
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]
