From 2c453b8348bff124cfb5c29dfd69a889735eb2d8 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:42:28 -0500 Subject: [PATCH 1/6] fix: reject nested aggregate functions during logical planning `sum(sum(x))` was accepted by the planner and only failed at physical planning with a `NotImplemented` error that dumped the `Debug` formatting of the aggregate's `Signature` without mentioning nesting, the function or the column. Add `check_no_nested_aggregates`, called from `Aggregate::try_new`, which rejects an aggregate function call that contains another aggregate call in its arguments, `FILTER` or `ORDER BY`. The error names both expressions and carries a `Diagnostic` pointing back into the SQL. Aggregates used as arguments of a window function (`sum(sum(x)) OVER ()`) or computed in a subquery remain legal and are unaffected. Co-Authored-By: Claude Opus 4.8 --- datafusion/expr/src/logical_plan/builder.rs | 19 +++ datafusion/expr/src/logical_plan/plan.rs | 9 +- datafusion/expr/src/utils.rs | 149 +++++++++++++++++- datafusion/sql/tests/cases/diagnostic.rs | 18 ++- datafusion/sql/tests/sql_integration.rs | 44 ++++++ .../sqllogictest/test_files/aggregate.slt | 49 ++++++ 6 files changed, 283 insertions(+), 5 deletions(-) diff --git a/datafusion/expr/src/logical_plan/builder.rs b/datafusion/expr/src/logical_plan/builder.rs index 2ecb12c30afad..3cf1641763c91 100644 --- a/datafusion/expr/src/logical_plan/builder.rs +++ b/datafusion/expr/src/logical_plan/builder.rs @@ -2900,6 +2900,25 @@ mod tests { Ok(()) } + #[test] + fn plan_builder_aggregate_rejects_nested_aggregates() -> Result<()> { + // https://github.com/apache/datafusion/issues/23812 + let err = table_scan( + Some("employee_csv"), + &employee_schema(), + Some(vec![0, 3, 4]), + )? + .aggregate(vec![col("id")], vec![sum(sum(col("salary")))]) + .expect_err("nested aggregates should be rejected"); + + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Aggregate function calls cannot be nested: 'sum(employee_csv.salary)' is nested inside 'sum(sum(employee_csv.salary))'" + ); + + Ok(()) + } + #[test] fn test_join_metadata() -> Result<()> { let left_schema = DFSchema::new_with_metadata( diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs index 9cfab21a0395e..a289c5abb2b5a 100644 --- a/datafusion/expr/src/logical_plan/plan.rs +++ b/datafusion/expr/src/logical_plan/plan.rs @@ -41,8 +41,9 @@ use crate::logical_plan::display::{GraphvizVisitor, IndentVisitor}; use crate::logical_plan::extension::UserDefinedLogicalNode; use crate::logical_plan::{DmlStatement, Statement}; use crate::utils::{ - enumerate_grouping_sets, exprlist_to_fields, find_out_reference_exprs, - grouping_set_expr_count, grouping_set_to_exprlist, merge_schema, split_conjunction, + check_no_nested_aggregates, enumerate_grouping_sets, exprlist_to_fields, + find_out_reference_exprs, grouping_set_expr_count, grouping_set_to_exprlist, + merge_schema, split_conjunction, }; use crate::{ BinaryExpr, CreateMemoryTable, CreateView, Execute, Expr, ExprSchemable, GroupingSet, @@ -3892,6 +3893,10 @@ impl Aggregate { group_expr: Vec, aggr_expr: Vec, ) -> Result { + // Reject e.g. `sum(sum(x))` here rather than letting it reach physical + // planning, which has no equivalent for a nested aggregate. + check_no_nested_aggregates(group_expr.iter().chain(aggr_expr.iter()))?; + let group_expr = enumerate_grouping_sets(group_expr)?; let is_grouping_set = matches!(group_expr.as_slice(), [Expr::GroupingSet(_)]); diff --git a/datafusion/expr/src/utils.rs b/datafusion/expr/src/utils.rs index 22abb454d4e6b..46b17d6ce0530 100644 --- a/datafusion/expr/src/utils.rs +++ b/datafusion/expr/src/utils.rs @@ -34,8 +34,8 @@ use datafusion_common::tree_node::{ }; use datafusion_common::utils::get_at_indices; use datafusion_common::{ - Column, DFSchema, DFSchemaRef, HashMap, Result, TableReference, internal_err, - plan_err, + Column, DFSchema, DFSchemaRef, Diagnostic, HashMap, Result, Span, TableReference, + internal_err, plan_err, }; #[cfg(not(feature = "sql"))] @@ -652,6 +652,88 @@ pub fn find_aggregate_exprs<'a>(exprs: impl IntoIterator) -> Ve }) } +/// Returns an error if any of `exprs` contains an aggregate function call +/// nested inside another aggregate function call, such as `sum(sum(x))`. +/// +/// Such expressions are not valid SQL (PostgreSQL reports `aggregate function +/// calls cannot be nested`) and have no physical equivalent, 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. +/// +/// The nested aggregate is searched for in the arguments, `FILTER` and +/// `ORDER BY` of each aggregate function call. Note that an aggregate used as +/// the argument of a *window* function, such as `sum(sum(x)) OVER ()`, is +/// legal and accepted: there the inner aggregate is evaluated by the +/// `Aggregate` node and the window function is evaluated on top of its result. +pub fn check_no_nested_aggregates<'a>( + exprs: impl IntoIterator, +) -> Result<()> { + for expr in exprs { + expr.apply(|outer| { + if !matches!(outer, Expr::AggregateFunction(_)) { + return Ok(TreeNodeRecursion::Continue); + } + + // `outer` is an aggregate, so no other aggregate may appear + // anywhere below it. + let mut nested: Option<&Expr> = None; + outer.apply_children(|child| { + child.apply(|inner| { + if matches!(inner, Expr::AggregateFunction(_)) { + nested = Some(inner); + Ok(TreeNodeRecursion::Stop) + } else { + Ok(TreeNodeRecursion::Continue) + } + }) + })?; + + match nested { + Some(inner) => nested_aggregate_err(outer, inner), + // The whole subtree below `outer` has just been checked + None => Ok(TreeNodeRecursion::Jump), + } + })?; + } + Ok(()) +} + +fn nested_aggregate_err(outer: &Expr, inner: &Expr) -> Result { + let diagnostic = Diagnostic::new_error( + "aggregate function calls cannot be nested", + first_span(inner), + ) + .with_help( + format!( + "Compute '{inner}' in an inner query and aggregate its result, or use a window function such as '{outer} OVER ()'" + ), + None, + ); + + plan_err!( + "Aggregate function calls cannot be nested: '{inner}' is nested inside '{outer}'"; + diagnostic = diagnostic + ) +} + +/// Best effort source location for `expr`: the first [`Span`] found in its +/// subtree. Only some expressions (currently columns) carry spans, so pointing +/// at e.g. the column of `sum(x)` is the closest we can get to the location of +/// the whole expression. +fn first_span(expr: &Expr) -> Option { + let mut span = None; + expr.apply(|e| { + span = e.spans().and_then(|spans| spans.first()); + if span.is_some() { + Ok(TreeNodeRecursion::Stop) + } else { + Ok(TreeNodeRecursion::Continue) + } + }) + .ok()?; + span +} + /// Collect all deeply nested `Expr::WindowFunction`. They are returned in order of occurrence /// (depth first), with duplicates omitted. pub fn find_window_exprs<'a>(exprs: impl IntoIterator) -> Vec { @@ -1917,4 +1999,67 @@ mod tests { substr(string: String, start_pos: Int64, length: Int64) "); } + + #[test] + fn test_check_no_nested_aggregates_ok() -> Result<()> { + use crate::test::function_stub::{count, sum}; + + // a plain aggregate, an aggregate wrapped in a scalar expression and a + // window function over an aggregate are all legal + let window_over_aggregate = Expr::from(WindowFunction::new( + WindowFunctionDefinition::AggregateUDF(sum_udaf()), + vec![sum(col("a"))], + )); + let exprs = [ + sum(col("a")), + count(col("a")) + lit(1), + window_over_aggregate, + ]; + + check_no_nested_aggregates(exprs.iter())?; + Ok(()) + } + + #[test] + fn test_check_no_nested_aggregates_err() { + use crate::test::function_stub::{count, sum}; + use insta::assert_snapshot; + + // directly nested + let err = check_no_nested_aggregates([&sum(sum(col("a")))]).unwrap_err(); + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Aggregate function calls cannot be nested: 'sum(a)' is nested inside 'sum(sum(a))'" + ); + + // nested below another expression in the arguments + let err = + check_no_nested_aggregates([&sum(col("a") + count(col("b")))]).unwrap_err(); + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Aggregate function calls cannot be nested: 'COUNT(b)' is nested inside 'sum(a + COUNT(b))'" + ); + + // nested in the FILTER of an aggregate + let filtered = sum(col("a")) + .filter(sum(col("b")).gt(lit(0))) + .build() + .unwrap(); + let err = check_no_nested_aggregates([&filtered]).unwrap_err(); + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Aggregate function calls cannot be nested: 'sum(b)' is nested inside 'sum(a) FILTER (WHERE sum(b) > Int32(0))'" + ); + + // nested in the ORDER BY of an aggregate + let ordered = sum(col("a")) + .order_by(vec![Sort::new(sum(col("b")), true, false)]) + .build() + .unwrap(); + let err = check_no_nested_aggregates([&ordered]).unwrap_err(); + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Aggregate function calls cannot be nested: 'sum(b)' is nested inside 'sum(a) ORDER BY [sum(b) ASC NULLS LAST]'" + ); + } } diff --git a/datafusion/sql/tests/cases/diagnostic.rs b/datafusion/sql/tests/cases/diagnostic.rs index df46a48d88579..5fcc461b2cd42 100644 --- a/datafusion/sql/tests/cases/diagnostic.rs +++ b/datafusion/sql/tests/cases/diagnostic.rs @@ -16,6 +16,7 @@ // under the License. use datafusion_functions::string; +use datafusion_functions_aggregate::sum::sum_udaf; use insta::assert_snapshot; use std::{collections::HashMap, ops::ControlFlow, sync::Arc}; @@ -44,7 +45,8 @@ fn do_query(sql: &'static str) -> Diagnostic { ..ParserOptions::default() }; let state = MockSessionState::default() - .with_scalar_function(Arc::new(string::concat().as_ref().clone())); + .with_scalar_function(Arc::new(string::concat().as_ref().clone())) + .with_aggregate_function(sum_udaf()); let context = MockContextProvider { state }; let sql_to_rel = SqlToRel::new_with_options(&context, options); match sql_to_rel.statement_to_plan(statement) { @@ -671,3 +673,17 @@ fn test_multiple_null_comparison_warnings() -> Result<()> { ); Ok(()) } + +#[test] +fn test_nested_aggregate() -> Result<()> { + let query = "SELECT sum(sum(/*a*/age/*a*/)) FROM person"; + let spans = get_spans(query); + let diag = do_query(query); + assert_snapshot!(diag.message, @"aggregate function calls cannot be nested"); + assert_eq!(diag.span, Some(spans["a"])); + assert_snapshot!( + diag.helps[0].message, + @"Compute 'sum(person.age)' in an inner query and aggregate its result, or use a window function such as 'sum(sum(person.age)) OVER ()'" + ); + Ok(()) +} diff --git a/datafusion/sql/tests/sql_integration.rs b/datafusion/sql/tests/sql_integration.rs index 54480a4224992..a0bfba3b8699a 100644 --- a/datafusion/sql/tests/sql_integration.rs +++ b/datafusion/sql/tests/sql_integration.rs @@ -1771,6 +1771,50 @@ fn select_simple_aggregate_with_groupby_position_out_of_range() { ); } +#[test] +fn select_nested_aggregate() { + // https://github.com/apache/datafusion/issues/23812 + let err = logical_plan("SELECT sum(sum(age)) FROM person") + .expect_err("query should have failed"); + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Aggregate function calls cannot be nested: 'sum(person.age)' is nested inside 'sum(sum(person.age))'" + ); + + let err = logical_plan("SELECT state, sum(count(age)) FROM person GROUP BY state") + .expect_err("query should have failed"); + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Aggregate function calls cannot be nested: 'count(person.age)' is nested inside 'sum(count(person.age))'" + ); + + let err = + logical_plan("SELECT state FROM person GROUP BY state HAVING sum(sum(age)) > 0") + .expect_err("query should have failed"); + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Aggregate function calls cannot be nested: 'sum(person.age)' is nested inside 'sum(sum(person.age))'" + ); +} + +#[test] +fn select_aggregate_inside_window_function() { + // an aggregate as the argument of a window function is legal: the window + // function is evaluated on top of the aggregate + let plan = + logical_plan("SELECT state, sum(sum(age)) OVER () FROM person GROUP BY state") + .unwrap(); + assert_snapshot!( + plan, + @r" + Projection: person.state, sum(sum(person.age)) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING + WindowAggr: windowExpr=[[sum(sum(person.age)) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING]] + Aggregate: groupBy=[[person.state]], aggr=[[sum(person.age)]] + TableScan: person + " + ); +} + #[test] fn select_simple_aggregate_with_groupby_can_use_alias() { let plan = diff --git a/datafusion/sqllogictest/test_files/aggregate.slt b/datafusion/sqllogictest/test_files/aggregate.slt index cbb9c5d0317dc..787be8f3d143a 100644 --- a/datafusion/sqllogictest/test_files/aggregate.slt +++ b/datafusion/sqllogictest/test_files/aggregate.slt @@ -9607,3 +9607,52 @@ 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 scalar function applied to an aggregate is legal +query IR +SELECT column1, abs(sum(column2)) FROM nested_agg_t GROUP BY column1 ORDER BY column1; +---- +1 30 +2 30 + +# A window function applied to an aggregate is legal +query IR +SELECT column1, sum(sum(column2)) OVER () FROM nested_agg_t GROUP BY column1 ORDER BY column1; +---- +1 60 +2 60 + +# An aggregate over the result of an aggregate computed in a subquery is legal +query R +SELECT sum(s) FROM (SELECT sum(column2) AS s FROM nested_agg_t GROUP BY column1); +---- +60 + +statement ok +DROP TABLE nested_agg_t; From d284a9d51cc24d36c73bc39cc1e0970283a5542d Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 22 Jul 2026 15:33:34 -0500 Subject: [PATCH 2/6] fix: reject window functions nested in aggregates, and nested windows Both classes fail at physical planning today with the same unhelpful `NotImplemented` error as `sum(sum(x))`: SELECT sum(sum(x) OVER ()) FROM t -- window in aggregate SELECT sum(sum(x) OVER ()) OVER () FROM t -- nested window SELECT row_number() OVER (ORDER BY row_number() OVER ()) -- nested window Generalize the nesting check to a shared `check_nesting` helper and expose it as `check_aggregate_nesting` (no aggregate or window call inside an aggregate, called from `Aggregate::try_new`) and `check_window_nesting` (no window call inside a window call, called from `Window::try_new`). Error and diagnostic messages follow PostgreSQL's wording per case. An aggregate used as the argument of a window function (`sum(sum(x)) OVER ()`) stays legal. Co-Authored-By: Claude Opus 4.8 --- datafusion/expr/src/logical_plan/plan.rs | 13 +- datafusion/expr/src/utils.rs | 187 +++++++++++++----- datafusion/sql/tests/cases/diagnostic.rs | 25 ++- datafusion/sql/tests/sql_integration.rs | 31 +++ .../sqllogictest/test_files/aggregate.slt | 17 ++ 5 files changed, 224 insertions(+), 49 deletions(-) diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs index a289c5abb2b5a..555e8226c732c 100644 --- a/datafusion/expr/src/logical_plan/plan.rs +++ b/datafusion/expr/src/logical_plan/plan.rs @@ -41,9 +41,9 @@ use crate::logical_plan::display::{GraphvizVisitor, IndentVisitor}; use crate::logical_plan::extension::UserDefinedLogicalNode; use crate::logical_plan::{DmlStatement, Statement}; use crate::utils::{ - check_no_nested_aggregates, enumerate_grouping_sets, exprlist_to_fields, - find_out_reference_exprs, grouping_set_expr_count, grouping_set_to_exprlist, - merge_schema, split_conjunction, + check_aggregate_nesting, check_window_nesting, enumerate_grouping_sets, + exprlist_to_fields, find_out_reference_exprs, grouping_set_expr_count, + grouping_set_to_exprlist, merge_schema, split_conjunction, }; use crate::{ BinaryExpr, CreateMemoryTable, CreateView, Execute, Expr, ExprSchemable, GroupingSet, @@ -2780,6 +2780,11 @@ pub struct Window { impl Window { /// Create a new window operator. pub fn try_new(window_expr: Vec, input: Arc) -> Result { + // Reject e.g. `sum(sum(x) OVER ()) OVER ()` here rather than letting it + // reach physical planning, which has no equivalent for a nested window + // function. + check_window_nesting(window_expr.iter())?; + let fields: Vec<(Option, Arc)> = input .schema() .iter() @@ -3895,7 +3900,7 @@ impl Aggregate { ) -> Result { // Reject e.g. `sum(sum(x))` here rather than letting it reach physical // planning, which has no equivalent for a nested aggregate. - check_no_nested_aggregates(group_expr.iter().chain(aggr_expr.iter()))?; + check_aggregate_nesting(group_expr.iter().chain(aggr_expr.iter()))?; let group_expr = enumerate_grouping_sets(group_expr)?; diff --git a/datafusion/expr/src/utils.rs b/datafusion/expr/src/utils.rs index 46b17d6ce0530..eb0d25c3c7d47 100644 --- a/datafusion/expr/src/utils.rs +++ b/datafusion/expr/src/utils.rs @@ -652,45 +652,91 @@ pub fn find_aggregate_exprs<'a>(exprs: impl IntoIterator) -> Ve }) } -/// Returns an error if any of `exprs` contains an aggregate function call -/// nested inside another aggregate function call, such as `sum(sum(x))`. +/// Either an aggregate function call or a window function call, the two kinds +/// of expression whose nesting is restricted by [`check_aggregate_nesting`] and +/// [`check_window_nesting`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum FunctionCallKind { + Aggregate, + Window, +} + +impl FunctionCallKind { + fn of(expr: &Expr) -> Option { + match expr { + Expr::AggregateFunction(_) => Some(Self::Aggregate), + Expr::WindowFunction(_) => Some(Self::Window), + _ => None, + } + } +} + +/// Returns an error if any of `exprs` contains an aggregate or window function +/// call nested inside an aggregate function call, such as `sum(sum(x))` or +/// `sum(sum(x) OVER ())`. /// /// Such expressions are not valid SQL (PostgreSQL reports `aggregate function -/// calls cannot be nested`) and have no physical equivalent, 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. +/// calls cannot be nested` and `aggregate function calls cannot contain window +/// function calls`) and have no physical equivalent, 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. /// -/// The nested aggregate is searched for in the arguments, `FILTER` and -/// `ORDER BY` of each aggregate function call. Note that an aggregate used as -/// the argument of a *window* function, such as `sum(sum(x)) OVER ()`, is -/// legal and accepted: there the inner aggregate is evaluated by the +/// The nested call is searched for in the arguments, `FILTER` and `ORDER BY` of +/// each aggregate function call. Note that the reverse nesting, an aggregate +/// used as the argument of a *window* function such as `sum(sum(x)) OVER ()`, +/// is legal and accepted: there the inner aggregate is evaluated by the /// `Aggregate` node and the window function is evaluated on top of its result. -pub fn check_no_nested_aggregates<'a>( +pub fn check_aggregate_nesting<'a>( + exprs: impl IntoIterator, +) -> Result<()> { + check_nesting( + exprs, + FunctionCallKind::Aggregate, + &[FunctionCallKind::Aggregate, FunctionCallKind::Window], + ) +} + +/// Returns an error if any of `exprs` contains a window function call nested +/// inside another window function call, such as `sum(sum(x) OVER ()) OVER ()`. +/// +/// Such expressions are not valid SQL (PostgreSQL reports `window function +/// calls cannot be nested`) and have no physical equivalent. The nested call is +/// searched for in the arguments, `PARTITION BY`, `ORDER BY` and `FILTER` of +/// each window function call. +pub fn check_window_nesting<'a>(exprs: impl IntoIterator) -> Result<()> { + check_nesting(exprs, FunctionCallKind::Window, &[FunctionCallKind::Window]) +} + +/// Returns an error if any expression of kind `outer_kind` in `exprs` has an +/// expression of one of the `illegal_inner_kinds` anywhere below it. +fn check_nesting<'a>( exprs: impl IntoIterator, + outer_kind: FunctionCallKind, + illegal_inner_kinds: &[FunctionCallKind], ) -> Result<()> { for expr in exprs { expr.apply(|outer| { - if !matches!(outer, Expr::AggregateFunction(_)) { + if FunctionCallKind::of(outer) != Some(outer_kind) { return Ok(TreeNodeRecursion::Continue); } - // `outer` is an aggregate, so no other aggregate may appear - // anywhere below it. - let mut nested: Option<&Expr> = None; + let mut nested = None; outer.apply_children(|child| { - child.apply(|inner| { - if matches!(inner, Expr::AggregateFunction(_)) { - nested = Some(inner); + child.apply(|inner| match FunctionCallKind::of(inner) { + Some(kind) if illegal_inner_kinds.contains(&kind) => { + nested = Some((inner, kind)); Ok(TreeNodeRecursion::Stop) - } else { - Ok(TreeNodeRecursion::Continue) } + _ => Ok(TreeNodeRecursion::Continue), }) })?; match nested { - Some(inner) => nested_aggregate_err(outer, inner), - // The whole subtree below `outer` has just been checked + Some((inner, inner_kind)) => { + nested_call_err(outer, outer_kind, inner, inner_kind) + } + // Nothing below `outer` can be an `outer_kind` expression, as + // that kind is always one of the illegal inner kinds None => Ok(TreeNodeRecursion::Jump), } })?; @@ -698,20 +744,34 @@ pub fn check_no_nested_aggregates<'a>( Ok(()) } -fn nested_aggregate_err(outer: &Expr, inner: &Expr) -> Result { - let diagnostic = Diagnostic::new_error( - "aggregate function calls cannot be nested", - first_span(inner), - ) - .with_help( - format!( - "Compute '{inner}' in an inner query and aggregate its result, or use a window function such as '{outer} OVER ()'" +fn nested_call_err( + outer: &Expr, + outer_kind: FunctionCallKind, + inner: &Expr, + inner_kind: FunctionCallKind, +) -> Result { + let (message, help) = match (outer_kind, inner_kind) { + (FunctionCallKind::Aggregate, FunctionCallKind::Aggregate) => ( + "Aggregate function calls cannot be nested", + format!( + "Compute '{inner}' in an inner query and aggregate its result, or use a window function such as '{outer} OVER ()'" + ), + ), + (FunctionCallKind::Aggregate, FunctionCallKind::Window) => ( + "Aggregate function calls cannot contain window function calls", + format!("Compute '{inner}' in an inner query and aggregate its result"), ), - None, - ); + (FunctionCallKind::Window, _) => ( + "Window function calls cannot be nested", + format!("Compute '{inner}' in an inner query and use its result here"), + ), + }; + + let diagnostic = + Diagnostic::new_error(message, first_span(inner)).with_help(help, None); plan_err!( - "Aggregate function calls cannot be nested: '{inner}' is nested inside '{outer}'"; + "{message}: '{inner}' is nested inside '{outer}'"; diagnostic = diagnostic ) } @@ -2000,33 +2060,37 @@ mod tests { "); } + /// `sum() OVER ()` + fn sum_over(args: Vec) -> Expr { + Expr::from(WindowFunction::new( + WindowFunctionDefinition::AggregateUDF(sum_udaf()), + args, + )) + } + #[test] - fn test_check_no_nested_aggregates_ok() -> Result<()> { + fn test_check_aggregate_nesting_ok() -> Result<()> { use crate::test::function_stub::{count, sum}; // a plain aggregate, an aggregate wrapped in a scalar expression and a // window function over an aggregate are all legal - let window_over_aggregate = Expr::from(WindowFunction::new( - WindowFunctionDefinition::AggregateUDF(sum_udaf()), - vec![sum(col("a"))], - )); let exprs = [ sum(col("a")), count(col("a")) + lit(1), - window_over_aggregate, + sum_over(vec![sum(col("a"))]), ]; - check_no_nested_aggregates(exprs.iter())?; + check_aggregate_nesting(exprs.iter())?; Ok(()) } #[test] - fn test_check_no_nested_aggregates_err() { + fn test_check_aggregate_nesting_err() { use crate::test::function_stub::{count, sum}; use insta::assert_snapshot; // directly nested - let err = check_no_nested_aggregates([&sum(sum(col("a")))]).unwrap_err(); + let err = check_aggregate_nesting([&sum(sum(col("a")))]).unwrap_err(); assert_snapshot!( err.strip_backtrace(), @"Error during planning: Aggregate function calls cannot be nested: 'sum(a)' is nested inside 'sum(sum(a))'" @@ -2034,7 +2098,7 @@ mod tests { // nested below another expression in the arguments let err = - check_no_nested_aggregates([&sum(col("a") + count(col("b")))]).unwrap_err(); + check_aggregate_nesting([&sum(col("a") + count(col("b")))]).unwrap_err(); assert_snapshot!( err.strip_backtrace(), @"Error during planning: Aggregate function calls cannot be nested: 'COUNT(b)' is nested inside 'sum(a + COUNT(b))'" @@ -2045,7 +2109,7 @@ mod tests { .filter(sum(col("b")).gt(lit(0))) .build() .unwrap(); - let err = check_no_nested_aggregates([&filtered]).unwrap_err(); + let err = check_aggregate_nesting([&filtered]).unwrap_err(); assert_snapshot!( err.strip_backtrace(), @"Error during planning: Aggregate function calls cannot be nested: 'sum(b)' is nested inside 'sum(a) FILTER (WHERE sum(b) > Int32(0))'" @@ -2056,10 +2120,45 @@ mod tests { .order_by(vec![Sort::new(sum(col("b")), true, false)]) .build() .unwrap(); - let err = check_no_nested_aggregates([&ordered]).unwrap_err(); + let err = check_aggregate_nesting([&ordered]).unwrap_err(); assert_snapshot!( err.strip_backtrace(), @"Error during planning: Aggregate function calls cannot be nested: 'sum(b)' is nested inside 'sum(a) ORDER BY [sum(b) ASC NULLS LAST]'" ); + + // a window function nested inside an aggregate + let err = check_aggregate_nesting([&sum(sum_over(vec![col("a")]))]).unwrap_err(); + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Aggregate function calls cannot contain window function calls: 'sum(a) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING' is nested inside 'sum(sum(a) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING)'" + ); + } + + #[test] + fn test_check_window_nesting_ok() -> Result<()> { + use crate::test::function_stub::sum; + + // a window function over a column, and a window function over the + // result of an aggregate, are both legal + let exprs = [ + sum_over(vec![col("a")]), + sum_over(vec![sum(col("a"))]), + sum(col("a")), + ]; + + check_window_nesting(exprs.iter())?; + Ok(()) + } + + #[test] + fn test_check_window_nesting_err() { + use insta::assert_snapshot; + + let err = check_window_nesting([&sum_over(vec![sum_over(vec![col("a")])])]) + .unwrap_err(); + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Window function calls cannot be nested: 'sum(a) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING' is nested inside 'sum(sum(a) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING'" + ); } } diff --git a/datafusion/sql/tests/cases/diagnostic.rs b/datafusion/sql/tests/cases/diagnostic.rs index 5fcc461b2cd42..9109eb8c3e199 100644 --- a/datafusion/sql/tests/cases/diagnostic.rs +++ b/datafusion/sql/tests/cases/diagnostic.rs @@ -679,7 +679,7 @@ fn test_nested_aggregate() -> Result<()> { let query = "SELECT sum(sum(/*a*/age/*a*/)) FROM person"; let spans = get_spans(query); let diag = do_query(query); - assert_snapshot!(diag.message, @"aggregate function calls cannot be nested"); + assert_snapshot!(diag.message, @"Aggregate function calls cannot be nested"); assert_eq!(diag.span, Some(spans["a"])); assert_snapshot!( diag.helps[0].message, @@ -687,3 +687,26 @@ fn test_nested_aggregate() -> Result<()> { ); Ok(()) } + +#[test] +fn test_window_function_inside_aggregate() -> Result<()> { + let query = "SELECT sum(sum(/*a*/age/*a*/) OVER ()) FROM person"; + let spans = get_spans(query); + let diag = do_query(query); + assert_snapshot!( + diag.message, + @"Aggregate function calls cannot contain window function calls" + ); + assert_eq!(diag.span, Some(spans["a"])); + Ok(()) +} + +#[test] +fn test_nested_window_function() -> Result<()> { + let query = "SELECT sum(sum(/*a*/age/*a*/) OVER ()) OVER () FROM person"; + let spans = get_spans(query); + let diag = do_query(query); + assert_snapshot!(diag.message, @"Window function calls cannot be nested"); + assert_eq!(diag.span, Some(spans["a"])); + Ok(()) +} diff --git a/datafusion/sql/tests/sql_integration.rs b/datafusion/sql/tests/sql_integration.rs index a0bfba3b8699a..a4bf0db910774 100644 --- a/datafusion/sql/tests/sql_integration.rs +++ b/datafusion/sql/tests/sql_integration.rs @@ -1797,6 +1797,37 @@ fn select_nested_aggregate() { ); } +#[test] +fn select_window_function_inside_aggregate() { + // https://github.com/apache/datafusion/issues/23812 + let err = logical_plan("SELECT sum(sum(age) OVER ()) FROM person") + .expect_err("query should have failed"); + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Aggregate function calls cannot contain window function calls: 'sum(person.age) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING' is nested inside 'sum(sum(person.age) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING)'" + ); +} + +#[test] +fn select_nested_window_function() { + // https://github.com/apache/datafusion/issues/23812 + let err = logical_plan("SELECT sum(sum(age) OVER ()) OVER () FROM person") + .expect_err("query should have failed"); + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Window function calls cannot be nested: 'sum(person.age) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING' is nested inside 'sum(sum(person.age) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING'" + ); + + let err = logical_plan( + "SELECT rank() OVER (ORDER BY rank() OVER (ORDER BY age)) FROM person", + ) + .expect_err("query should have failed"); + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Window function calls cannot be nested: 'rank() ORDER BY [person.age ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW' is nested inside 'rank() ORDER BY [rank() ORDER BY [person.age ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW'" + ); +} + #[test] fn select_aggregate_inside_window_function() { // an aggregate as the argument of a window function is legal: the window diff --git a/datafusion/sqllogictest/test_files/aggregate.slt b/datafusion/sqllogictest/test_files/aggregate.slt index 787be8f3d143a..6ac63584aac4d 100644 --- a/datafusion/sqllogictest/test_files/aggregate.slt +++ b/datafusion/sqllogictest/test_files/aggregate.slt @@ -9634,6 +9634,17 @@ 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 +SELECT row_number() OVER (ORDER BY row_number() OVER ()) FROM nested_agg_t; + # A scalar function applied to an aggregate is legal query IR SELECT column1, abs(sum(column2)) FROM nested_agg_t GROUP BY column1 ORDER BY column1; @@ -9654,5 +9665,11 @@ SELECT sum(s) FROM (SELECT sum(column2) AS s FROM nested_agg_t GROUP BY column1) ---- 60 +# An aggregate over the result of a window function computed in a subquery is legal +query R +SELECT sum(s) FROM (SELECT sum(column2) OVER () AS s FROM nested_agg_t); +---- +180 + statement ok DROP TABLE nested_agg_t; From 4c245426cda76d74f806fa7ced772f4dc9a3f944 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 22 Jul 2026 16:25:35 -0500 Subject: [PATCH 3/6] refactor: make the nesting checks crate-private `check_aggregate_nesting` and `check_window_nesting` have no callers outside `datafusion-expr`: `Aggregate::try_new` and `Window::try_new` enforce the invariant for every way of building those nodes, so nothing downstream needs to run the checks itself. Keep them `pub(crate)` rather than adding public API that would have to be supported forever. Co-Authored-By: Claude Opus 4.8 --- datafusion/expr/src/utils.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/datafusion/expr/src/utils.rs b/datafusion/expr/src/utils.rs index eb0d25c3c7d47..59fb1febefe40 100644 --- a/datafusion/expr/src/utils.rs +++ b/datafusion/expr/src/utils.rs @@ -686,7 +686,12 @@ impl FunctionCallKind { /// used as the argument of a *window* function such as `sum(sum(x)) OVER ()`, /// is legal and accepted: there the inner aggregate is evaluated by the /// `Aggregate` node and the window function is evaluated on top of its result. -pub fn check_aggregate_nesting<'a>( +/// +/// Callers do not need to invoke this directly: [`Aggregate::try_new`] enforces +/// the invariant for every way of building an `Aggregate`. +/// +/// [`Aggregate::try_new`]: crate::logical_plan::Aggregate::try_new +pub(crate) fn check_aggregate_nesting<'a>( exprs: impl IntoIterator, ) -> Result<()> { check_nesting( @@ -703,7 +708,14 @@ pub fn check_aggregate_nesting<'a>( /// calls cannot be nested`) and have no physical equivalent. The nested call is /// searched for in the arguments, `PARTITION BY`, `ORDER BY` and `FILTER` of /// each window function call. -pub fn check_window_nesting<'a>(exprs: impl IntoIterator) -> Result<()> { +/// +/// Callers do not need to invoke this directly: [`Window::try_new`] enforces +/// the invariant for every way of building a `Window`. +/// +/// [`Window::try_new`]: crate::logical_plan::Window::try_new +pub(crate) fn check_window_nesting<'a>( + exprs: impl IntoIterator, +) -> Result<()> { check_nesting(exprs, FunctionCallKind::Window, &[FunctionCallKind::Window]) } From 84a90acd1395425b26763d1eeeb27acf222602b1 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:47:46 -0500 Subject: [PATCH 4/6] refactor: collapse the nesting checks into one traversal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two checks were the same walk with different kind parameters, which needed a `FunctionCallKind` enum, an `illegal_inner_kinds` slice per call site and a separate error constructor. Replace all of it with a single `check_aggregate_and_window_nesting` plus `illegal_nesting_err`, which decides legality and builds the error from one match over the (outer, inner) expression pair — so the rules and their messages live in one place. Both `Aggregate::try_new` and `Window::try_new` call the same function; the `Jump` optimization is dropped because the guard already skips non-call nodes cheaply. Behavior and error messages are unchanged. Also add a `LogicalPlanBuilder::window` test, the non-SQL counterpart of the existing aggregate one, which mutation testing showed was the only path with no in-crate coverage. Co-Authored-By: Claude Opus 4.8 --- datafusion/expr/src/logical_plan/builder.rs | 23 +++ datafusion/expr/src/logical_plan/plan.rs | 10 +- datafusion/expr/src/utils.rs | 197 +++++++------------- 3 files changed, 92 insertions(+), 138 deletions(-) diff --git a/datafusion/expr/src/logical_plan/builder.rs b/datafusion/expr/src/logical_plan/builder.rs index 3cf1641763c91..1f32d9c6da445 100644 --- a/datafusion/expr/src/logical_plan/builder.rs +++ b/datafusion/expr/src/logical_plan/builder.rs @@ -2919,6 +2919,29 @@ mod tests { Ok(()) } + #[test] + fn plan_builder_window_rejects_nested_window_functions() -> Result<()> { + // https://github.com/apache/datafusion/issues/23812 + let sum_over = |arg| { + Expr::from(expr::WindowFunction::new( + crate::WindowFunctionDefinition::AggregateUDF( + crate::test::function_stub::sum_udaf(), + ), + vec![arg], + )) + }; + let err = table_scan(Some("employee_csv"), &employee_schema(), Some(vec![4]))? + .window(vec![sum_over(sum_over(col("salary")))]) + .expect_err("nested window functions should be rejected"); + + assert_snapshot!( + err.strip_backtrace(), + @"Error during planning: Window function calls cannot be nested: 'sum(employee_csv.salary) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING' is nested inside 'sum(sum(employee_csv.salary) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING'" + ); + + Ok(()) + } + #[test] fn test_join_metadata() -> Result<()> { let left_schema = DFSchema::new_with_metadata( diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs index 555e8226c732c..9ac27b46a78e6 100644 --- a/datafusion/expr/src/logical_plan/plan.rs +++ b/datafusion/expr/src/logical_plan/plan.rs @@ -41,9 +41,9 @@ use crate::logical_plan::display::{GraphvizVisitor, IndentVisitor}; use crate::logical_plan::extension::UserDefinedLogicalNode; use crate::logical_plan::{DmlStatement, Statement}; use crate::utils::{ - check_aggregate_nesting, check_window_nesting, enumerate_grouping_sets, - exprlist_to_fields, find_out_reference_exprs, grouping_set_expr_count, - grouping_set_to_exprlist, merge_schema, split_conjunction, + check_aggregate_and_window_nesting, enumerate_grouping_sets, exprlist_to_fields, + find_out_reference_exprs, grouping_set_expr_count, grouping_set_to_exprlist, + merge_schema, split_conjunction, }; use crate::{ BinaryExpr, CreateMemoryTable, CreateView, Execute, Expr, ExprSchemable, GroupingSet, @@ -2783,7 +2783,7 @@ impl Window { // Reject e.g. `sum(sum(x) OVER ()) OVER ()` here rather than letting it // reach physical planning, which has no equivalent for a nested window // function. - check_window_nesting(window_expr.iter())?; + check_aggregate_and_window_nesting(window_expr.iter())?; let fields: Vec<(Option, Arc)> = input .schema() @@ -3900,7 +3900,7 @@ impl Aggregate { ) -> Result { // Reject e.g. `sum(sum(x))` here rather than letting it reach physical // planning, which has no equivalent for a nested aggregate. - check_aggregate_nesting(group_expr.iter().chain(aggr_expr.iter()))?; + check_aggregate_and_window_nesting(group_expr.iter().chain(aggr_expr.iter()))?; let group_expr = enumerate_grouping_sets(group_expr)?; diff --git a/datafusion/expr/src/utils.rs b/datafusion/expr/src/utils.rs index 59fb1febefe40..7fec58421a0ae 100644 --- a/datafusion/expr/src/utils.rs +++ b/datafusion/expr/src/utils.rs @@ -34,8 +34,8 @@ use datafusion_common::tree_node::{ }; use datafusion_common::utils::get_at_indices; use datafusion_common::{ - Column, DFSchema, DFSchemaRef, Diagnostic, HashMap, Result, Span, TableReference, - internal_err, plan_err, + Column, DFSchema, DFSchemaRef, DataFusionError, Diagnostic, HashMap, Result, Span, + TableReference, internal_err, plan_datafusion_err, plan_err, }; #[cfg(not(feature = "sql"))] @@ -652,139 +652,85 @@ pub fn find_aggregate_exprs<'a>(exprs: impl IntoIterator) -> Ve }) } -/// Either an aggregate function call or a window function call, the two kinds -/// of expression whose nesting is restricted by [`check_aggregate_nesting`] and -/// [`check_window_nesting`]. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum FunctionCallKind { - Aggregate, - Window, -} - -impl FunctionCallKind { - fn of(expr: &Expr) -> Option { - match expr { - Expr::AggregateFunction(_) => Some(Self::Aggregate), - Expr::WindowFunction(_) => Some(Self::Window), - _ => None, - } - } -} - -/// Returns an error if any of `exprs` contains an aggregate or window function -/// call nested inside an aggregate function call, such as `sum(sum(x))` or -/// `sum(sum(x) OVER ())`. -/// -/// Such expressions are not valid SQL (PostgreSQL reports `aggregate function -/// calls cannot be nested` and `aggregate function calls cannot contain window -/// function calls`) and have no physical equivalent, 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. +/// 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. /// -/// The nested call is searched for in the arguments, `FILTER` and `ORDER BY` of -/// each aggregate function call. Note that the reverse nesting, an aggregate -/// used as the argument of a *window* function such as `sum(sum(x)) OVER ()`, -/// is legal and accepted: there the inner 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`] enforces -/// the invariant for every way of building an `Aggregate`. +/// Callers do not need to invoke this directly: [`Aggregate::try_new`] and +/// [`Window::try_new`] enforce the invariant for every way of building those +/// nodes. /// /// [`Aggregate::try_new`]: crate::logical_plan::Aggregate::try_new -pub(crate) fn check_aggregate_nesting<'a>( - exprs: impl IntoIterator, -) -> Result<()> { - check_nesting( - exprs, - FunctionCallKind::Aggregate, - &[FunctionCallKind::Aggregate, FunctionCallKind::Window], - ) -} - -/// Returns an error if any of `exprs` contains a window function call nested -/// inside another window function call, such as `sum(sum(x) OVER ()) OVER ()`. -/// -/// Such expressions are not valid SQL (PostgreSQL reports `window function -/// calls cannot be nested`) and have no physical equivalent. The nested call is -/// searched for in the arguments, `PARTITION BY`, `ORDER BY` and `FILTER` of -/// each window function call. -/// -/// Callers do not need to invoke this directly: [`Window::try_new`] enforces -/// the invariant for every way of building a `Window`. -/// /// [`Window::try_new`]: crate::logical_plan::Window::try_new -pub(crate) fn check_window_nesting<'a>( +pub(crate) fn check_aggregate_and_window_nesting<'a>( exprs: impl IntoIterator, -) -> Result<()> { - check_nesting(exprs, FunctionCallKind::Window, &[FunctionCallKind::Window]) -} - -/// Returns an error if any expression of kind `outer_kind` in `exprs` has an -/// expression of one of the `illegal_inner_kinds` anywhere below it. -fn check_nesting<'a>( - exprs: impl IntoIterator, - outer_kind: FunctionCallKind, - illegal_inner_kinds: &[FunctionCallKind], ) -> Result<()> { for expr in exprs { expr.apply(|outer| { - if FunctionCallKind::of(outer) != Some(outer_kind) { + if !matches!(outer, Expr::AggregateFunction(_) | Expr::WindowFunction(_)) { return Ok(TreeNodeRecursion::Continue); } - let mut nested = None; + // Look for an illegally nested call in the arguments, `FILTER`, + // `ORDER BY` and `PARTITION BY` of this call + let mut err = None; outer.apply_children(|child| { - child.apply(|inner| match FunctionCallKind::of(inner) { - Some(kind) if illegal_inner_kinds.contains(&kind) => { - nested = Some((inner, kind)); + child.apply(|inner| { + err = illegal_nesting_err(outer, inner); + if err.is_some() { Ok(TreeNodeRecursion::Stop) + } else { + Ok(TreeNodeRecursion::Continue) } - _ => Ok(TreeNodeRecursion::Continue), }) })?; - match nested { - Some((inner, inner_kind)) => { - nested_call_err(outer, outer_kind, inner, inner_kind) - } - // Nothing below `outer` can be an `outer_kind` expression, as - // that kind is always one of the illegal inner kinds - None => Ok(TreeNodeRecursion::Jump), + match err { + Some(err) => Err(err), + None => Ok(TreeNodeRecursion::Continue), } })?; } Ok(()) } -fn nested_call_err( - outer: &Expr, - outer_kind: FunctionCallKind, - inner: &Expr, - inner_kind: FunctionCallKind, -) -> Result { - let (message, help) = match (outer_kind, inner_kind) { - (FunctionCallKind::Aggregate, FunctionCallKind::Aggregate) => ( +/// The planning error for a call to `inner` nested inside a call to `outer`, or +/// `None` if that nesting is legal. +fn illegal_nesting_err(outer: &Expr, inner: &Expr) -> Option { + // Messages follow PostgreSQL, which rejects the same three cases + let (message, help) = match (outer, inner) { + (Expr::AggregateFunction(_), Expr::AggregateFunction(_)) => ( "Aggregate function calls cannot be nested", format!( "Compute '{inner}' in an inner query and aggregate its result, or use a window function such as '{outer} OVER ()'" ), ), - (FunctionCallKind::Aggregate, FunctionCallKind::Window) => ( + (Expr::AggregateFunction(_), Expr::WindowFunction(_)) => ( "Aggregate function calls cannot contain window function calls", format!("Compute '{inner}' in an inner query and aggregate its result"), ), - (FunctionCallKind::Window, _) => ( + (Expr::WindowFunction(_), Expr::WindowFunction(_)) => ( "Window function calls cannot be nested", format!("Compute '{inner}' in an inner query and use its result here"), ), + // Anything else, including an aggregate inside a window call + _ => return None, }; - let diagnostic = - Diagnostic::new_error(message, first_span(inner)).with_help(help, None); - - plan_err!( - "{message}: '{inner}' is nested inside '{outer}'"; - diagnostic = diagnostic + Some( + plan_datafusion_err!("{message}: '{inner}' is nested inside '{outer}'") + .with_diagnostic( + Diagnostic::new_error(message, first_span(inner)).with_help(help, None), + ), ) } @@ -2081,36 +2027,37 @@ mod tests { } #[test] - fn test_check_aggregate_nesting_ok() -> Result<()> { + fn test_check_aggregate_and_window_nesting_ok() -> Result<()> { use crate::test::function_stub::{count, sum}; - // a plain aggregate, an aggregate wrapped in a scalar expression and a - // window function over an aggregate are all legal let exprs = [ + // a plain aggregate, and one wrapped in a scalar expression sum(col("a")), count(col("a")) + lit(1), + // a window function over a column, and over an aggregate + sum_over(vec![col("a")]), sum_over(vec![sum(col("a"))]), ]; - check_aggregate_nesting(exprs.iter())?; + check_aggregate_and_window_nesting(exprs.iter())?; Ok(()) } #[test] - fn test_check_aggregate_nesting_err() { + fn test_check_aggregate_and_window_nesting_err() { use crate::test::function_stub::{count, sum}; use insta::assert_snapshot; - // directly nested - let err = check_aggregate_nesting([&sum(sum(col("a")))]).unwrap_err(); + // an aggregate directly inside an aggregate + let err = check_aggregate_and_window_nesting([&sum(sum(col("a")))]).unwrap_err(); assert_snapshot!( err.strip_backtrace(), @"Error during planning: Aggregate function calls cannot be nested: 'sum(a)' is nested inside 'sum(sum(a))'" ); // nested below another expression in the arguments - let err = - check_aggregate_nesting([&sum(col("a") + count(col("b")))]).unwrap_err(); + let err = check_aggregate_and_window_nesting([&sum(col("a") + count(col("b")))]) + .unwrap_err(); assert_snapshot!( err.strip_backtrace(), @"Error during planning: Aggregate function calls cannot be nested: 'COUNT(b)' is nested inside 'sum(a + COUNT(b))'" @@ -2121,7 +2068,7 @@ mod tests { .filter(sum(col("b")).gt(lit(0))) .build() .unwrap(); - let err = check_aggregate_nesting([&filtered]).unwrap_err(); + let err = check_aggregate_and_window_nesting([&filtered]).unwrap_err(); assert_snapshot!( err.strip_backtrace(), @"Error during planning: Aggregate function calls cannot be nested: 'sum(b)' is nested inside 'sum(a) FILTER (WHERE sum(b) > Int32(0))'" @@ -2132,41 +2079,25 @@ mod tests { .order_by(vec![Sort::new(sum(col("b")), true, false)]) .build() .unwrap(); - let err = check_aggregate_nesting([&ordered]).unwrap_err(); + let err = check_aggregate_and_window_nesting([&ordered]).unwrap_err(); assert_snapshot!( err.strip_backtrace(), @"Error during planning: Aggregate function calls cannot be nested: 'sum(b)' is nested inside 'sum(a) ORDER BY [sum(b) ASC NULLS LAST]'" ); - // a window function nested inside an aggregate - let err = check_aggregate_nesting([&sum(sum_over(vec![col("a")]))]).unwrap_err(); + // a window function inside an aggregate + let err = check_aggregate_and_window_nesting([&sum(sum_over(vec![col("a")]))]) + .unwrap_err(); assert_snapshot!( err.strip_backtrace(), @"Error during planning: Aggregate function calls cannot contain window function calls: 'sum(a) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING' is nested inside 'sum(sum(a) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING)'" ); - } - #[test] - fn test_check_window_nesting_ok() -> Result<()> { - use crate::test::function_stub::sum; - - // a window function over a column, and a window function over the - // result of an aggregate, are both legal - let exprs = [ - sum_over(vec![col("a")]), - sum_over(vec![sum(col("a"))]), - sum(col("a")), - ]; - - check_window_nesting(exprs.iter())?; - Ok(()) - } - - #[test] - fn test_check_window_nesting_err() { - use insta::assert_snapshot; - - let err = check_window_nesting([&sum_over(vec![sum_over(vec![col("a")])])]) + // a window function inside a window function + let err = + check_aggregate_and_window_nesting([&sum_over(vec![sum_over(vec![col( + "a", + )])])]) .unwrap_err(); assert_snapshot!( err.strip_backtrace(), From ecabe8ab5f8e4a5b79f7464d557eb6f669a6f569 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Thu, 23 Jul 2026 10:42:45 -0500 Subject: [PATCH 5/6] docs: clarify which constructors run the nesting check; test PARTITION BY Address review feedback: - The doc comment overstated the guarantee. Note that the `try_new_with_schema` constructors and building a `Window` from its public fields bypass the check, so a hand-built node needs the caller to run it. - Add a regression test for a window call nested in `PARTITION BY`, exercising that traversal path. Co-Authored-By: Claude Opus 4.8 --- datafusion/expr/src/utils.rs | 8 +++++--- datafusion/sqllogictest/test_files/aggregate.slt | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/datafusion/expr/src/utils.rs b/datafusion/expr/src/utils.rs index 7fec58421a0ae..4a1515570cb01 100644 --- a/datafusion/expr/src/utils.rs +++ b/datafusion/expr/src/utils.rs @@ -665,9 +665,11 @@ pub fn find_aggregate_exprs<'a>(exprs: impl IntoIterator) -> Ve /// 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 -/// [`Window::try_new`] enforce the invariant for every way of building those -/// nodes. +/// [`Aggregate::try_new`] and [`Window::try_new`] call this, so the SQL planner +/// and the `DataFrame`/`LogicalPlanBuilder` paths are checked without callers +/// invoking it directly. The lower-level `try_new_with_schema` constructors and +/// building a `Window` from its public fields bypass the check, so a caller +/// that constructs those nodes by hand should call this itself. /// /// [`Aggregate::try_new`]: crate::logical_plan::Aggregate::try_new /// [`Window::try_new`]: crate::logical_plan::Window::try_new diff --git a/datafusion/sqllogictest/test_files/aggregate.slt b/datafusion/sqllogictest/test_files/aggregate.slt index 6ac63584aac4d..d307ddc066460 100644 --- a/datafusion/sqllogictest/test_files/aggregate.slt +++ b/datafusion/sqllogictest/test_files/aggregate.slt @@ -9645,6 +9645,10 @@ SELECT sum(sum(column2) OVER ()) OVER () FROM nested_agg_t; statement error DataFusion error: Error during planning: Window function calls cannot be nested SELECT row_number() OVER (ORDER BY row_number() OVER ()) FROM nested_agg_t; +# ... including a window call nested in `PARTITION BY` +statement error DataFusion error: Error during planning: Window function calls cannot be nested +SELECT row_number() OVER (PARTITION BY row_number() OVER ()) FROM nested_agg_t; + # A scalar function applied to an aggregate is legal query IR SELECT column1, abs(sum(column2)) FROM nested_agg_t GROUP BY column1 ORDER BY column1; From 1c190c1b2749914ff17d90b9cab7b31006421a21 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Thu, 23 Jul 2026 10:55:52 -0500 Subject: [PATCH 6/6] fix: drop misleading window-function hint from nested-aggregate error The aggregate-in-aggregate help text suggested `'{outer} OVER ()'`, which is legal only when the inner aggregate is a plain argument (`sum(sum(x))`). Because the outer expr's Display includes the FILTER/ORDER BY clauses, the suggestion rendered still-illegal syntax for those cases, e.g. `sum(x) FILTER (WHERE sum(x) > 0) OVER ()`. Drop the clause, leaving "Compute '{inner}' in an inner query and aggregate its result", which is always valid and consistent with the other two branches. Co-Authored-By: Claude Opus 4.8 --- datafusion/expr/src/utils.rs | 4 +--- datafusion/sql/tests/cases/diagnostic.rs | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/datafusion/expr/src/utils.rs b/datafusion/expr/src/utils.rs index 4a1515570cb01..7f79c5cf18c4a 100644 --- a/datafusion/expr/src/utils.rs +++ b/datafusion/expr/src/utils.rs @@ -712,9 +712,7 @@ fn illegal_nesting_err(outer: &Expr, inner: &Expr) -> Option { let (message, help) = match (outer, inner) { (Expr::AggregateFunction(_), Expr::AggregateFunction(_)) => ( "Aggregate function calls cannot be nested", - format!( - "Compute '{inner}' in an inner query and aggregate its result, or use a window function such as '{outer} OVER ()'" - ), + format!("Compute '{inner}' in an inner query and aggregate its result"), ), (Expr::AggregateFunction(_), Expr::WindowFunction(_)) => ( "Aggregate function calls cannot contain window function calls", diff --git a/datafusion/sql/tests/cases/diagnostic.rs b/datafusion/sql/tests/cases/diagnostic.rs index 9109eb8c3e199..1f2cefdec0629 100644 --- a/datafusion/sql/tests/cases/diagnostic.rs +++ b/datafusion/sql/tests/cases/diagnostic.rs @@ -683,7 +683,7 @@ fn test_nested_aggregate() -> Result<()> { assert_eq!(diag.span, Some(spans["a"])); assert_snapshot!( diag.helps[0].message, - @"Compute 'sum(person.age)' in an inner query and aggregate its result, or use a window function such as 'sum(sum(person.age)) OVER ()'" + @"Compute 'sum(person.age)' in an inner query and aggregate its result" ); Ok(()) }