From 31daed9aadf47a0e0d83a42df9dd3324e9c643ea Mon Sep 17 00:00:00 2001 From: sundyli <543950155@qq.com> Date: Mon, 23 Mar 2026 20:30:58 +0800 Subject: [PATCH 1/6] fix(query): return semantic error for invalid grouping (#19554) --- src/query/expression/src/constant_folder.rs | 7 ++++++ .../it/sql/planner/semantic/type_check.rs | 25 +++++++++++++++++++ src/query/sql/src/planner/binder/aggregate.rs | 6 +++++ .../group/group_by_grouping_sets.test | 12 +++++++++ 4 files changed, 50 insertions(+) diff --git a/src/query/expression/src/constant_folder.rs b/src/query/expression/src/constant_folder.rs index 8855356e9cf9b..5be030a4890be 100644 --- a/src/query/expression/src/constant_folder.rs +++ b/src/query/expression/src/constant_folder.rs @@ -517,6 +517,13 @@ impl<'a, Index: ColumnIndex> ConstantFolder<'a, Index> { ); } + // `grouping` is a placeholder before the aggregate rewriter rewrites it to + // `grouping<...>(_grouping_id)`. Folding it here can reach the dummy + // implementation and panic on invalid queries. + if function.signature.name == "grouping" { + return (func_expr, func_domain); + } + if all_args_is_scalar { let block = DataBlock::empty_with_rows(1); let evaluator = Evaluator::new(&block, self.func_ctx, self.fn_registry); diff --git a/src/query/service/tests/it/sql/planner/semantic/type_check.rs b/src/query/service/tests/it/sql/planner/semantic/type_check.rs index 8f16ac8e610fd..88b42b251df6e 100644 --- a/src/query/service/tests/it/sql/planner/semantic/type_check.rs +++ b/src/query/service/tests/it/sql/planner/semantic/type_check.rs @@ -15,6 +15,7 @@ use databend_common_catalog::table_context::TableContext; use databend_common_expression::ColumnIndex; use databend_common_expression::Expr; +use databend_common_sql::Planner; use databend_common_sql::parse_exprs; use databend_query::test_kits::TestFixture; @@ -85,6 +86,30 @@ async fn test_inlist_with_null_builds_shallow_or_tree() -> anyhow::Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread")] +async fn test_invalid_grouping_returns_semantic_error() -> anyhow::Result<()> { + let fixture = TestFixture::setup().await?; + let ctx = fixture.new_query_ctx().await?; + let mut planner = Planner::new(ctx.clone()); + fixture + .execute_command("CREATE TABLE students(course STRING, type STRING)") + .await?; + + for sql in ["SELECT GROUPING()", "SELECT GROUPING() FROM students"] { + let err = planner + .plan_sql(sql) + .await + .expect_err("invalid grouping() should return a semantic error"); + assert!( + err.message() + .contains("grouping requires at least one argument"), + "unexpected error for `{sql}`: {err}" + ); + } + + Ok(()) +} + fn max_or_depth(expr: &Expr) -> usize { match expr { Expr::Cast(cast) => max_or_depth(&cast.expr), diff --git a/src/query/sql/src/planner/binder/aggregate.rs b/src/query/sql/src/planner/binder/aggregate.rs index 43188e18fd78d..76466549f86ed 100644 --- a/src/query/sql/src/planner/binder/aggregate.rs +++ b/src/query/sql/src/planner/binder/aggregate.rs @@ -427,6 +427,12 @@ impl<'a> AggregateRewriter<'a> { } fn replace_grouping(&mut self, function: &FunctionCall) -> Result { + if function.arguments.is_empty() { + return Err(ErrorCode::BadArguments( + "grouping requires at least one argument", + )); + } + let agg_info = &mut self.bind_context.aggregate_info; if agg_info.grouping_sets.is_none() { return Err(ErrorCode::SemanticError( diff --git a/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test b/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test index 4d25e44286081..3b2d7909de581 100644 --- a/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test +++ b/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test @@ -66,6 +66,18 @@ create or replace table t (a string, b string, c int); statement ok insert into t values ('a','A',1),('a','A',2),('a','B',1),('a','B',3),('b','A',1),('b','A',4),('b','B',1),('b','B',5); +statement error +SELECT GROUPING() + +statement error +SELECT GROUPING() FROM t + +statement error +SELECT GROUPING(NULL) FROM t + +statement error +SELECT GROUPING(a) FROM t GROUP BY () + query TTI select a, b, sum(c) as sc from t group by grouping sets ((a,b),(),(b),(a)) order by sc; ---- From 7846cf10e54391f66838fb95beb882eed479cbd1 Mon Sep 17 00:00:00 2001 From: sundyli <543950155@qq.com> Date: Tue, 24 Mar 2026 23:08:07 +0800 Subject: [PATCH 2/6] fix(query): reject raw grouping in filters (#19554) --- .../it/sql/planner/semantic/type_check.rs | 7 ++++- src/query/sql/src/planner/binder/qualify.rs | 8 +++++ .../sql/src/planner/binder/scalar_common.rs | 30 +++++++++++++++++++ src/query/sql/src/planner/binder/select.rs | 12 ++++++-- .../group/group_by_grouping_sets.test | 6 ++++ 5 files changed, 60 insertions(+), 3 deletions(-) diff --git a/src/query/service/tests/it/sql/planner/semantic/type_check.rs b/src/query/service/tests/it/sql/planner/semantic/type_check.rs index 88b42b251df6e..7bf30ce968cfc 100644 --- a/src/query/service/tests/it/sql/planner/semantic/type_check.rs +++ b/src/query/service/tests/it/sql/planner/semantic/type_check.rs @@ -95,7 +95,12 @@ async fn test_invalid_grouping_returns_semantic_error() -> anyhow::Result<()> { .execute_command("CREATE TABLE students(course STRING, type STRING)") .await?; - for sql in ["SELECT GROUPING()", "SELECT GROUPING() FROM students"] { + for sql in [ + "SELECT GROUPING()", + "SELECT GROUPING() FROM students", + "SELECT count() FROM students WHERE GROUPING() = 0 GROUP BY course", + "SELECT count() OVER () FROM students GROUP BY course QUALIFY GROUPING() = 0", + ] { let err = planner .plan_sql(sql) .await diff --git a/src/query/sql/src/planner/binder/qualify.rs b/src/query/sql/src/planner/binder/qualify.rs index f4e81dceb58e6..f89105babcd68 100644 --- a/src/query/sql/src/planner/binder/qualify.rs +++ b/src/query/sql/src/planner/binder/qualify.rs @@ -19,6 +19,8 @@ use databend_common_exception::ErrorCode; use databend_common_exception::Result; use super::Finder; +use super::grouping_clause_error; +use super::is_raw_grouping_function; use crate::BindContext; use crate::Binder; use crate::binder::ColumnBindingBuilder; @@ -78,6 +80,12 @@ impl Binder { .set_span(qualify.span())); } + let mut grouping_finder = Finder::new(&is_raw_grouping_function); + grouping_finder.visit(&qualify)?; + if let Some(ScalarExpr::FunctionCall(func)) = grouping_finder.scalars().first() { + return Err(grouping_clause_error(func, "Qualify")); + } + let scalar = { let mut qualify = qualify; if bind_context.in_grouping { diff --git a/src/query/sql/src/planner/binder/scalar_common.rs b/src/query/sql/src/planner/binder/scalar_common.rs index 05a37a62f2327..d8f8e95664d2d 100644 --- a/src/query/sql/src/planner/binder/scalar_common.rs +++ b/src/query/sql/src/planner/binder/scalar_common.rs @@ -15,6 +15,7 @@ use std::borrow::Cow; use std::collections::HashSet; +use databend_common_exception::ErrorCode; use databend_common_exception::Result; use databend_common_expression::Scalar; use databend_common_expression::types::DataType; @@ -28,6 +29,8 @@ use crate::plans::ScalarExpr; use crate::plans::Visitor; use crate::plans::walk_expr; +pub const GROUPING_FUNC_NAME: &str = "grouping"; + // Visitor that find Expressions that match a particular predicate pub struct Finder<'a, F> where F: Fn(&ScalarExpr) -> bool @@ -75,6 +78,33 @@ where F: Fn(&ScalarExpr) -> bool } } +pub fn is_grouping_function(scalar: &ScalarExpr) -> bool { + matches!( + scalar, + ScalarExpr::FunctionCall(func) if func.func_name.eq_ignore_ascii_case(GROUPING_FUNC_NAME) + ) +} + +pub fn is_raw_grouping_function(scalar: &ScalarExpr) -> bool { + matches!( + scalar, + ScalarExpr::FunctionCall(func) + if func.func_name.eq_ignore_ascii_case(GROUPING_FUNC_NAME) && func.params.is_empty() + ) +} + +pub fn grouping_clause_error(function: &FunctionCall, clause_name: &str) -> ErrorCode { + let err = if function.params.is_empty() && function.arguments.is_empty() { + ErrorCode::BadArguments("grouping requires at least one argument") + } else { + ErrorCode::SemanticError(format!( + "{clause_name} clause can't contain grouping functions" + )) + }; + + err.set_span(function.span) +} + pub fn split_conjunctions(scalar: &ScalarExpr) -> Vec { match scalar { ScalarExpr::FunctionCall(func) if func.func_name == "and" => [ diff --git a/src/query/sql/src/planner/binder/select.rs b/src/query/sql/src/planner/binder/select.rs index 6854fa62e4e6b..3e068d5bb0911 100644 --- a/src/query/sql/src/planner/binder/select.rs +++ b/src/query/sql/src/planner/binder/select.rs @@ -32,6 +32,8 @@ use databend_common_expression::types::DataType; use databend_common_functions::BUILTIN_FUNCTIONS; use super::Finder; +use super::grouping_clause_error; +use super::is_grouping_function; use super::sort::OrderItem; use crate::ColumnEntry; use crate::ColumnSet; @@ -87,14 +89,14 @@ impl Binder { aliases, ); let (scalar, _) = scalar_binder.bind(expr)?; - let f = |scalar: &ScalarExpr| { + let contains_agg_or_window = |scalar: &ScalarExpr| { matches!( scalar, ScalarExpr::AggregateFunction(_) | ScalarExpr::WindowFunction(_) ) }; - let mut finder = Finder::new(&f); + let mut finder = Finder::new(&contains_agg_or_window); finder.visit(&scalar)?; if !finder.scalars().is_empty() { return Err(ErrorCode::SemanticError( @@ -103,6 +105,12 @@ impl Binder { .set_span(scalar.span())); } + let mut grouping_finder = Finder::new(&is_grouping_function); + grouping_finder.visit(&scalar)?; + if let Some(ScalarExpr::FunctionCall(func)) = grouping_finder.scalars().first() { + return Err(grouping_clause_error(func, "Where")); + } + let filter_plan = Filter { predicates: split_conjunctions(&scalar), }; diff --git a/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test b/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test index 3b2d7909de581..6e85419e53f78 100644 --- a/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test +++ b/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test @@ -78,6 +78,12 @@ SELECT GROUPING(NULL) FROM t statement error SELECT GROUPING(a) FROM t GROUP BY () +statement error +SELECT count() FROM t WHERE GROUPING() = 0 GROUP BY a + +statement error +SELECT count() OVER () FROM t GROUP BY a QUALIFY GROUPING() = 0 + query TTI select a, b, sum(c) as sc from t group by grouping sets ((a,b),(),(b),(a)) order by sc; ---- From dc9bff0feedc5311b88fe4c0ab84685b543d8f8a Mon Sep 17 00:00:00 2001 From: sundyli <543950155@qq.com> Date: Wed, 25 Mar 2026 05:13:18 +0800 Subject: [PATCH 3/6] fix(query): reject grouping outside rewrite clauses (#19554) --- .../it/sql/planner/semantic/type_check.rs | 45 +++++++++++++++---- src/query/sql/src/planner/binder/aggregate.rs | 10 +++++ .../binder/bind_table_reference/bind_join.rs | 3 ++ src/query/sql/src/planner/binder/qualify.rs | 9 +--- .../sql/src/planner/binder/scalar_common.rs | 19 ++++++-- src/query/sql/src/planner/binder/select.rs | 9 +--- .../group/group_by_grouping_sets.test | 9 ++++ 7 files changed, 79 insertions(+), 25 deletions(-) diff --git a/src/query/service/tests/it/sql/planner/semantic/type_check.rs b/src/query/service/tests/it/sql/planner/semantic/type_check.rs index 7bf30ce968cfc..d7c0f7908e394 100644 --- a/src/query/service/tests/it/sql/planner/semantic/type_check.rs +++ b/src/query/service/tests/it/sql/planner/semantic/type_check.rs @@ -95,20 +95,49 @@ async fn test_invalid_grouping_returns_semantic_error() -> anyhow::Result<()> { .execute_command("CREATE TABLE students(course STRING, type STRING)") .await?; - for sql in [ - "SELECT GROUPING()", - "SELECT GROUPING() FROM students", - "SELECT count() FROM students WHERE GROUPING() = 0 GROUP BY course", - "SELECT count() OVER () FROM students GROUP BY course QUALIFY GROUPING() = 0", + for (sql, expected) in [ + ( + "SELECT GROUPING()", + "grouping requires at least one argument", + ), + ( + "SELECT GROUPING() FROM students", + "grouping requires at least one argument", + ), + ( + "SELECT count() FROM students WHERE GROUPING() = 0 GROUP BY course", + "grouping requires at least one argument", + ), + ( + "SELECT count() OVER () FROM students GROUP BY course QUALIFY GROUPING() = 0", + "grouping requires at least one argument", + ), + ( + "SELECT count() \ + FROM students s1 \ + JOIN students s2 ON GROUPING() = 0 \ + GROUP BY s1.course", + "grouping requires at least one argument", + ), + ( + "SELECT 1 FROM students GROUP BY GROUPING SETS ((GROUPING()))", + "grouping requires at least one argument", + ), + ( + "SELECT GROUPING(course) AS g, count() OVER () \ + FROM students \ + GROUP BY GROUPING SETS ((course), ()) \ + QUALIFY g = 0", + "Qualify clause can't contain grouping functions", + ), ] { let err = planner .plan_sql(sql) .await .expect_err("invalid grouping() should return a semantic error"); assert!( - err.message() - .contains("grouping requires at least one argument"), - "unexpected error for `{sql}`: {err}" + err.message().contains(expected), + "unexpected error for `{sql}`: {err}", ); } diff --git a/src/query/sql/src/planner/binder/aggregate.rs b/src/query/sql/src/planner/binder/aggregate.rs index 76466549f86ed..ff95ef848b7f7 100644 --- a/src/query/sql/src/planner/binder/aggregate.rs +++ b/src/query/sql/src/planner/binder/aggregate.rs @@ -34,6 +34,7 @@ use itertools::Itertools; use super::ExprContext; use super::Finder; use super::prune_by_children; +use super::reject_grouping_functions; use crate::BindContext; use crate::MetadataRef; use crate::Symbol; @@ -931,6 +932,15 @@ impl Binder { ) }; + reject_grouping_functions( + bind_context + .aggregate_info + .group_items + .iter() + .map(|item| &item.scalar), + "GROUP BY items", + )?; + for item in bind_context.aggregate_info.group_items.iter() { let mut finder = Finder::new(&f); finder.visit(&item.scalar)?; diff --git a/src/query/sql/src/planner/binder/bind_table_reference/bind_join.rs b/src/query/sql/src/planner/binder/bind_table_reference/bind_join.rs index 6028ab36cf897..2a8a172429e0e 100644 --- a/src/query/sql/src/planner/binder/bind_table_reference/bind_join.rs +++ b/src/query/sql/src/planner/binder/bind_table_reference/bind_join.rs @@ -32,6 +32,7 @@ use crate::MetadataRef; use crate::binder::Finder; use crate::binder::JoinPredicate; use crate::binder::Visibility; +use crate::binder::reject_grouping_functions; use crate::binder::wrap_nullable; use crate::normalize_identifier; use crate::optimizer::OptimizerContext; @@ -759,6 +760,8 @@ impl<'a> JoinConditionResolver<'a> { } fn check_join_allowed_scalar_expr(&mut self, scalars: &Vec) -> Result<()> { + reject_grouping_functions(scalars.iter(), "Join condition")?; + let f = |scalar: &ScalarExpr| { matches!( scalar, diff --git a/src/query/sql/src/planner/binder/qualify.rs b/src/query/sql/src/planner/binder/qualify.rs index f89105babcd68..a7729d3252fb8 100644 --- a/src/query/sql/src/planner/binder/qualify.rs +++ b/src/query/sql/src/planner/binder/qualify.rs @@ -19,8 +19,7 @@ use databend_common_exception::ErrorCode; use databend_common_exception::Result; use super::Finder; -use super::grouping_clause_error; -use super::is_raw_grouping_function; +use super::reject_grouping_functions; use crate::BindContext; use crate::Binder; use crate::binder::ColumnBindingBuilder; @@ -80,11 +79,7 @@ impl Binder { .set_span(qualify.span())); } - let mut grouping_finder = Finder::new(&is_raw_grouping_function); - grouping_finder.visit(&qualify)?; - if let Some(ScalarExpr::FunctionCall(func)) = grouping_finder.scalars().first() { - return Err(grouping_clause_error(func, "Qualify")); - } + reject_grouping_functions(std::iter::once(&qualify), "Qualify clause")?; let scalar = { let mut qualify = qualify; diff --git a/src/query/sql/src/planner/binder/scalar_common.rs b/src/query/sql/src/planner/binder/scalar_common.rs index d8f8e95664d2d..36bb0e295cd28 100644 --- a/src/query/sql/src/planner/binder/scalar_common.rs +++ b/src/query/sql/src/planner/binder/scalar_common.rs @@ -97,14 +97,27 @@ pub fn grouping_clause_error(function: &FunctionCall, clause_name: &str) -> Erro let err = if function.params.is_empty() && function.arguments.is_empty() { ErrorCode::BadArguments("grouping requires at least one argument") } else { - ErrorCode::SemanticError(format!( - "{clause_name} clause can't contain grouping functions" - )) + ErrorCode::SemanticError(format!("{clause_name} can't contain grouping functions")) }; err.set_span(function.span) } +pub fn reject_grouping_functions<'a>( + scalars: impl IntoIterator, + clause_name: &str, +) -> Result<()> { + for scalar in scalars { + let mut grouping_finder = Finder::new(&is_grouping_function); + grouping_finder.visit(scalar)?; + if let Some(ScalarExpr::FunctionCall(func)) = grouping_finder.scalars().first() { + return Err(grouping_clause_error(func, clause_name)); + } + } + + Ok(()) +} + pub fn split_conjunctions(scalar: &ScalarExpr) -> Vec { match scalar { ScalarExpr::FunctionCall(func) if func.func_name == "and" => [ diff --git a/src/query/sql/src/planner/binder/select.rs b/src/query/sql/src/planner/binder/select.rs index 3e068d5bb0911..fbe37b5c5c397 100644 --- a/src/query/sql/src/planner/binder/select.rs +++ b/src/query/sql/src/planner/binder/select.rs @@ -32,8 +32,7 @@ use databend_common_expression::types::DataType; use databend_common_functions::BUILTIN_FUNCTIONS; use super::Finder; -use super::grouping_clause_error; -use super::is_grouping_function; +use super::reject_grouping_functions; use super::sort::OrderItem; use crate::ColumnEntry; use crate::ColumnSet; @@ -105,11 +104,7 @@ impl Binder { .set_span(scalar.span())); } - let mut grouping_finder = Finder::new(&is_grouping_function); - grouping_finder.visit(&scalar)?; - if let Some(ScalarExpr::FunctionCall(func)) = grouping_finder.scalars().first() { - return Err(grouping_clause_error(func, "Where")); - } + reject_grouping_functions(std::iter::once(&scalar), "Where clause")?; let filter_plan = Filter { predicates: split_conjunctions(&scalar), diff --git a/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test b/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test index 6e85419e53f78..8f520116c7323 100644 --- a/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test +++ b/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test @@ -84,6 +84,15 @@ SELECT count() FROM t WHERE GROUPING() = 0 GROUP BY a statement error SELECT count() OVER () FROM t GROUP BY a QUALIFY GROUPING() = 0 +statement error +SELECT count() FROM t t1 JOIN t t2 ON GROUPING() = 0 GROUP BY t1.a + +statement error +SELECT 1 FROM t GROUP BY GROUPING SETS ((GROUPING())) + +statement error +SELECT GROUPING(a) AS g, count() OVER () FROM t GROUP BY GROUPING SETS ((a), ()) QUALIFY g = 0 + query TTI select a, b, sum(c) as sc from t group by grouping sets ((a,b),(),(b),(a)) order by sc; ---- From 4b6a979184bf04a083e841de17d46861dffa0ed1 Mon Sep 17 00:00:00 2001 From: sundyli <543950155@qq.com> Date: Wed, 25 Mar 2026 08:24:49 +0800 Subject: [PATCH 4/6] fix(query): rewrite grouping in qualify aliases (#19554) --- .../it/sql/planner/semantic/type_check.rs | 35 +++++++++++++++---- src/query/sql/src/planner/binder/aggregate.rs | 6 ++++ src/query/sql/src/planner/binder/qualify.rs | 6 ++-- .../group/group_by_grouping_sets.test | 19 ++++++++-- 4 files changed, 54 insertions(+), 12 deletions(-) diff --git a/src/query/service/tests/it/sql/planner/semantic/type_check.rs b/src/query/service/tests/it/sql/planner/semantic/type_check.rs index d7c0f7908e394..40c8a26c48098 100644 --- a/src/query/service/tests/it/sql/planner/semantic/type_check.rs +++ b/src/query/service/tests/it/sql/planner/semantic/type_check.rs @@ -123,13 +123,6 @@ async fn test_invalid_grouping_returns_semantic_error() -> anyhow::Result<()> { "SELECT 1 FROM students GROUP BY GROUPING SETS ((GROUPING()))", "grouping requires at least one argument", ), - ( - "SELECT GROUPING(course) AS g, count() OVER () \ - FROM students \ - GROUP BY GROUPING SETS ((course), ()) \ - QUALIFY g = 0", - "Qualify clause can't contain grouping functions", - ), ] { let err = planner .plan_sql(sql) @@ -144,6 +137,34 @@ async fn test_invalid_grouping_returns_semantic_error() -> anyhow::Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread")] +async fn test_grouping_qualify_rewrites_before_semantic_checks() -> anyhow::Result<()> { + let fixture = TestFixture::setup().await?; + let ctx = fixture.new_query_ctx().await?; + let mut planner = Planner::new(ctx.clone()); + fixture + .execute_command("CREATE TABLE students(course STRING, type STRING)") + .await?; + + for sql in [ + "SELECT count() OVER () \ + FROM students \ + GROUP BY GROUPING SETS ((course), ()) \ + QUALIFY GROUPING(course) = 0", + "SELECT GROUPING(course) AS g, count() OVER () \ + FROM students \ + GROUP BY GROUPING SETS ((course), ()) \ + QUALIFY g = 0", + ] { + planner + .plan_sql(sql) + .await + .unwrap_or_else(|err| panic!("expected valid grouping QUALIFY for `{sql}`: {err}")); + } + + Ok(()) +} + fn max_or_depth(expr: &Expr) -> usize { match expr { Expr::Cast(cast) => max_or_depth(&cast.expr), diff --git a/src/query/sql/src/planner/binder/aggregate.rs b/src/query/sql/src/planner/binder/aggregate.rs index ff95ef848b7f7..c1489c90057c4 100644 --- a/src/query/sql/src/planner/binder/aggregate.rs +++ b/src/query/sql/src/planner/binder/aggregate.rs @@ -428,6 +428,12 @@ impl<'a> AggregateRewriter<'a> { } fn replace_grouping(&mut self, function: &FunctionCall) -> Result { + // `grouping<...>(_grouping_id)` is the internal rewritten form. Alias-expanded + // QUALIFY expressions can bind to it directly and must not be rewritten again. + if !function.params.is_empty() { + return Ok(function.clone()); + } + if function.arguments.is_empty() { return Err(ErrorCode::BadArguments( "grouping requires at least one argument", diff --git a/src/query/sql/src/planner/binder/qualify.rs b/src/query/sql/src/planner/binder/qualify.rs index a7729d3252fb8..f8d134579d46c 100644 --- a/src/query/sql/src/planner/binder/qualify.rs +++ b/src/query/sql/src/planner/binder/qualify.rs @@ -19,13 +19,13 @@ use databend_common_exception::ErrorCode; use databend_common_exception::Result; use super::Finder; -use super::reject_grouping_functions; use crate::BindContext; use crate::Binder; use crate::binder::ColumnBindingBuilder; use crate::binder::ExprContext; use crate::binder::ScalarBinder; use crate::binder::Visibility; +use crate::binder::aggregate::AggregateRewriter; use crate::binder::split_conjunctions; use crate::binder::window::WindowRewriter; use crate::optimizer::ir::SExpr; @@ -56,6 +56,8 @@ impl Binder { aliases, ); let (mut scalar, _) = scalar_binder.bind(qualify)?; + let mut aggregate_rewriter = AggregateRewriter::new(bind_context, self.metadata.clone()); + aggregate_rewriter.visit(&mut scalar)?; let mut rewriter = WindowRewriter::new(bind_context, self.metadata.clone()); rewriter.visit(&mut scalar)?; Ok(scalar) @@ -79,8 +81,6 @@ impl Binder { .set_span(qualify.span())); } - reject_grouping_functions(std::iter::once(&qualify), "Qualify clause")?; - let scalar = { let mut qualify = qualify; if bind_context.in_grouping { diff --git a/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test b/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test index 8f520116c7323..3de26b70aae59 100644 --- a/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test +++ b/tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test @@ -90,8 +90,23 @@ SELECT count() FROM t t1 JOIN t t2 ON GROUPING() = 0 GROUP BY t1.a statement error SELECT 1 FROM t GROUP BY GROUPING SETS ((GROUPING())) -statement error -SELECT GROUPING(a) AS g, count() OVER () FROM t GROUP BY GROUPING SETS ((a), ()) QUALIFY g = 0 +query TII rowsort +SELECT a, GROUPING(a) AS g, count() OVER () AS w +FROM t +GROUP BY GROUPING SETS ((a), ()) +QUALIFY GROUPING(a) = 0 +---- +a 0 3 +b 0 3 + +query TII rowsort +SELECT a, GROUPING(a) AS g, count() OVER () AS w +FROM t +GROUP BY GROUPING SETS ((a), ()) +QUALIFY g = 0 +---- +a 0 3 +b 0 3 query TTI select a, b, sum(c) as sc from t group by grouping sets ((a,b),(),(b),(a)) order by sc; From 83e633d0eb625c8ad7adc3b3acfb4d884e55efe3 Mon Sep 17 00:00:00 2001 From: sundyli <543950155@qq.com> Date: Sun, 5 Apr 2026 01:11:44 +0000 Subject: [PATCH 5/6] test(query): cover grouping sets union qualify build --- .../it/sql/planner/semantic/type_check.rs | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/query/service/tests/it/sql/planner/semantic/type_check.rs b/src/query/service/tests/it/sql/planner/semantic/type_check.rs index 40c8a26c48098..f169387190dc0 100644 --- a/src/query/service/tests/it/sql/planner/semantic/type_check.rs +++ b/src/query/service/tests/it/sql/planner/semantic/type_check.rs @@ -17,6 +17,8 @@ use databend_common_expression::ColumnIndex; use databend_common_expression::Expr; use databend_common_sql::Planner; use databend_common_sql::parse_exprs; +use databend_common_sql::plans::Plan; +use databend_query::physical_plans::PhysicalPlanBuilder; use databend_query::test_kits::TestFixture; #[tokio::test(flavor = "multi_thread")] @@ -165,6 +167,41 @@ async fn test_grouping_qualify_rewrites_before_semantic_checks() -> anyhow::Resu Ok(()) } +#[tokio::test(flavor = "multi_thread")] +async fn test_grouping_sets_to_union_keeps_grouping_id_for_qualify_windows() -> anyhow::Result<()> { + let fixture = TestFixture::setup().await?; + let ctx = fixture.new_query_ctx().await?; + ctx.get_settings() + .set_setting("grouping_sets_to_union".to_string(), "1".to_string())?; + + fixture + .execute_command("CREATE TABLE students(course STRING, type STRING)") + .await?; + + let sql = "SELECT course, GROUPING(course) AS g, count() OVER () AS w \ + FROM students \ + GROUP BY GROUPING SETS ((course), ()) \ + QUALIFY GROUPING(course) = 0"; + + let mut planner = Planner::new(ctx.clone()); + let (plan, _) = planner.plan_sql(sql).await?; + + let Plan::Query { + s_expr, + metadata, + bind_context, + .. + } = plan + else { + panic!("expected query plan"); + }; + + let mut builder = PhysicalPlanBuilder::new(metadata, ctx, false); + builder.build(&s_expr, bind_context.column_set()).await?; + + Ok(()) +} + fn max_or_depth(expr: &Expr) -> usize { match expr { Expr::Cast(cast) => max_or_depth(&cast.expr), From af1d6728b29da3df30cf4f25e0d23bf9406aae72 Mon Sep 17 00:00:00 2001 From: sundyli <543950155@qq.com> Date: Sun, 5 Apr 2026 01:22:40 +0000 Subject: [PATCH 6/6] style(query): format grouping sets union plan --- .../rule/agg_rules/rule_hierarchical_grouping_sets.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/query/sql/src/planner/optimizer/optimizers/rule/agg_rules/rule_hierarchical_grouping_sets.rs b/src/query/sql/src/planner/optimizer/optimizers/rule/agg_rules/rule_hierarchical_grouping_sets.rs index 76df2263e97e0..b3299f90da1e4 100644 --- a/src/query/sql/src/planner/optimizer/optimizers/rule/agg_rules/rule_hierarchical_grouping_sets.rs +++ b/src/query/sql/src/planner/optimizer/optimizers/rule/agg_rules/rule_hierarchical_grouping_sets.rs @@ -402,7 +402,8 @@ impl RuleHierarchicalGroupingSetsToUnion { )?; // Step 4: Assemble the complete plan - let union_result = self.create_union_all(&union_branches, eval_scalar, grouping_id_index)?; + let union_result = + self.create_union_all(&union_branches, eval_scalar, grouping_id_index)?; // Step 5: Chain all CTEs in correct dependency order // Sequence semantics: left executes first, right executes after