fix: reject nested aggregate functions (e.g. sum(sum(x))) during logical planning

[adriangb]

Which issue does this PR close?

• Closes Nested aggregate functions (e.g. sum(sum(x))) are accepted at logical planning and fail at physical planning with an unhelpful error #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.

Two neighbouring classes of illegal nesting (raised in review) fail exactly the same way, so this
PR covers them too:

SELECT sum(sum(x) OVER ()) FROM t;                          -- window call inside an aggregate
SELECT sum(sum(x) OVER ()) OVER () FROM t;                  -- nested window calls
SELECT row_number() OVER (ORDER BY row_number() OVER ());   -- nested window calls

PostgreSQL rejects all three at parse/analysis time with aggregate function calls cannot be nested, aggregate function calls cannot contain window function calls and window function calls cannot be nested respectively; this PR uses the same wording per case.

What changes are included in this PR?

• One crate-private check_aggregate_and_window_nesting, which walks the expressions and, for every
  aggregate or window call it finds, rejects an illegally nested call below it (in the arguments,
  FILTER, PARTITION BY or ORDER BY). Which pairs are illegal, and the message for each, live
  in a single match over the (outer, inner) pair.
• Aggregate::try_new and Window::try_new call it, so both the SQL path and the
  DataFrame/LogicalPlanBuilder path are covered, and the failure happens while the logical plan
  is built.
• The errors name both expressions and carry 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 nest one call inside another of a
restricted kind: 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 or window result computed in a subquery.

Are these changes tested?

Yes:

• unit tests for the check (arguments, FILTER, ORDER BY, window-in-aggregate, window-in-window,
  and the legal window-over-aggregate case) in datafusion/expr/src/utils.rs;
• LogicalPlanBuilder::aggregate and LogicalPlanBuilder::window tests covering the non-SQL path;
• SQL planner tests for SELECT, GROUP BY and HAVING, for window-in-aggregate and nested
  windows, plus a test asserting the sum(sum(x)) OVER () plan is unchanged;
• diagnostic tests asserting the span and help message;
• sqllogictests in aggregate.slt covering every row of the table in the issue plus the window
  cases.

Are there any user-facing changes?

Queries with these nestings now fail during planning with an actionable error instead of failing
during physical planning with a NotImplemented error. No previously working query changes
behavior.

No new public API: the check is pub(crate), since Aggregate::try_new and Window::try_new
enforce the invariant for every way of building those nodes.

Changed lines are at 99.3% coverage (cargo llvm-cov; the one uncovered line is a formatting
artifact inside a passing test), and cargo mutants --in-diff generates 11 mutants of which 4 are
unviable and all 7 viable ones are killed.

[adriangb]

@neilconway would you mind taking a look at this since we're trying to match postgres behavior 😉 ?

[Copilot]

Pull request overview

This PR adds an explicit logical-planning validation to reject nested aggregate function calls (e.g. sum(sum(x))), ensuring such queries fail early with an actionable planning error (including a Diagnostic span/help) instead of failing later during physical planning with an unhelpful NotImplemented error.

Changes:

• Introduces datafusion_expr::utils::check_no_nested_aggregates to detect aggregates nested inside aggregate arguments / FILTER / ORDER BY and emit a targeted planning error + diagnostic.
• Enforces the invariant centrally by calling the check from Aggregate::try_new, covering both SQL-planner and non-SQL (LogicalPlanBuilder / DataFrame) plan construction.
• Adds broad test coverage (expr unit tests, logical plan builder test, SQL integration + diagnostic span/help assertions, and sqllogictest cases), including verification that the legal window-over-aggregate case remains unchanged.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
datafusion/expr/src/utils.rs	Adds check_no_nested_aggregates, diagnostic construction, and unit tests for legal/illegal nesting scenarios.
datafusion/expr/src/logical_plan/plan.rs	Calls check_no_nested_aggregates from Aggregate::try_new to reject nested aggregates during logical plan construction.
datafusion/expr/src/logical_plan/builder.rs	Adds a non-SQL path test ensuring LogicalPlanBuilder::aggregate rejects nested aggregates.
datafusion/sql/tests/sql_integration.rs	Adds SQL planner integration tests for nested aggregates and for the legal window-over-aggregate plan shape.
datafusion/sql/tests/cases/diagnostic.rs	Adds a diagnostic test asserting error message, span location, and help text for nested aggregates.
datafusion/sqllogictest/test_files/aggregate.slt	Adds sqllogictest coverage for nested-aggregate rejection and confirms legal scalar/window/subquery aggregate patterns remain valid.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[neilconway]

Overall lgtm! Great test coverage, nice and clear implementation, and good PR commentary.

Seems like we could potentially check for and reject two additional related classes of illegal queries:

• Window functions nested inside aggregates (SELECT sum(sum(x) OVER ()) FROM t)
• Nested window functions

We can take that on as a separate PR if you prefer.

[neilconway]

Seems like this suggests illegal syntax if the problematic construct occurs in a FILTER or ORDER BY clause.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 96.52778% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.75%. Comparing base (5de7f1d) to head (1c190c1).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/expr/src/utils.rs	97.34%	0 Missing and 3 partials ⚠️
datafusion/expr/src/logical_plan/builder.rs	93.10%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #23813      +/-   ##
==========================================
+ Coverage   80.71%   80.75%   +0.03%     
==========================================
  Files        1089     1089              
  Lines      368760   368968     +208     
  Branches   368760   368968     +208     
==========================================
+ Hits       297647   297961     +314     
+ Misses      53372    53250     -122     
- Partials    17741    17757      +16     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kosiew]

@adriangb
Nice fix overall. Catching these invalid nesting cases during logical planning is a clear improvement, and the diagnostics and test coverage look good. I just have a couple of small suggestions that I think would make the change a bit more complete.

[kosiew]

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.

[adriangb]

I've reworded the doc comment to be accurate about which paths run the check. I'd lean toward not validating in the others, wdyt?

[kosiew]

The coverage here looks great. One additional case that might be worth adding is a nested window function in PARTITION BY, for example:

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.

[adriangb]

added this coverage in ecabe8a

[adriangb]

Thanks for the review @kosiew and @neilconway !