Nested aggregate functions (e.g. `sum(sum(x))`) are accepted at logical planning and fail at physical planning with an unhelpful error

[adriangb]

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 time.

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 entire coercion table, and never mentions nesting, the offending function name, or the offending column.

This is not specific to UDAFs or to any one function: built-in sum(sum(x)) and sum(count(x)) both reproduce it.

To Reproduce

One query in datafusion-cli, no table required:

$ datafusion-cli
DataFusion CLI v54.1.0

> SELECT sum(sum(1));
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: [Native(LogicalType(Native(UInt8), UInt8)), Native(LogicalType(Native(UInt16), UInt16)), Native(LogicalType(Native(UInt32), UInt32))], default_casted_type: UInt64 } }]), Coercible([Implicit { desired_type: Native(LogicalType(Native(Int64), Int64)), implicit_coercion: ImplicitCoercion { allowed_source_types: [Native(LogicalType(Native(Int8), Int8)), Native(LogicalType(Native(Int16), Int16)), Native(LogicalType(Native(Int32), Int32))], default_casted_type: Int64 } }]), Coercible([Implicit { desired_type: Native(LogicalType(Native(Float64), Float64)), implicit_coercion: ImplicitCoercion { allowed_source_types: [Float], default_casted_type: Float64 } }]), Coercible([Exact { desired_type: Duration }])]), volatility: Immutable, parameter_names: None } } }, params: AggregateFunctionParams { args: [Literal(Int64(1), None)], distinct: false, filter: None, order_by: [], null_treatment: None } })

The logical plan that gets built, with the nested aggregate visible inside aggr:

Projection: sum(sum(Int64(1)))
  Aggregate: groupBy=[[]], aggr=[[sum(sum(Int64(1)))]]
    EmptyRelation: rows=1

(EXPLAIN SELECT sum(sum(1)); cannot show this, since it fails the same way even with datafusion.explain.logical_plan_only = true. The plan above is from DataFrame::logical_plan on the same statement.)

Other variants, all one-liners in datafusion-cli:

Query	Result
SELECT sum(sum(1));	fails at physical planning
SELECT sum(count(1));	fails at physical planning
SELECT a, sum(sum(a)) FROM (VALUES (1),(2)) t(a) GROUP BY a;	fails at physical planning
SELECT a FROM (VALUES (1),(2)) t(a) GROUP BY a HAVING sum(sum(a)) > 0;	fails at physical planning
SELECT abs(sum(1));	works (correct: scalar over aggregate)
SELECT sum(sum(1)) OVER ();	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 sum(sum(1));
ERROR:  aggregate function calls cannot be nested
LINE 1: SELECT sum(sum(1));
                   ^

Note the last two rows of the table above already behave correctly, so this is narrowly about rejecting the illegal nesting and not about changing anything else. In particular sum(sum(1)) OVER () is legal in PostgreSQL (a window function over an aggregate result, returning 1) and DataFusion already plans it correctly as WindowAggr over Aggregate and returns the same 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) 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) 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 is what makes the legal cases work, 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 that approach sounds right to the maintainers, I am happy to put up a PR.

Found while triaging user-reported query failures in 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.