fix(query): return semantic error for invalid grouping()

[sundy-li]

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

• fixes bug: invalid GROUPING() query panics during constant folding instead of returning semantic error #19554
• stop constant folding from evaluating the placeholder grouping scalar before aggregate rewrite
• reject zero-argument GROUPING() calls with a semantic error instead of letting them reach the dummy implementation
• add planner and sqllogictest regressions for invalid GROUPING() usage

Tests

• Unit Test
• Logic Test
• Benchmark Test
• No Test - Explain why

Type of change

• Bug Fix (non-breaking change which fixes an issue)
• New Feature (non-breaking change which adds functionality)
• Breaking Change (fix or feature that could cause existing functionality not to work as expected)
• Documentation Update
• Refactoring
• Performance Improvement
• Other (please describe):

Validation

• cargo test -p databend-query --test it sql::planner::semantic::type_check::test_invalid_grouping_returns_semantic_error -- --exact
• cargo test -p databend-query --test it sql::planner::semantic::type_check -- --nocapture
• cargo fmt --all --check
• cargo clippy -p databend-common-expression -p databend-common-sql --lib -- -D warnings

Added sqllogictest coverage in tests/sqllogictests/suites/duckdb/sql/aggregate/group/group_by_grouping_sets.test, but did not run the standalone sqllogictest harness locally.

---

This change is 

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 31daed9aad

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[sundy-li]

The current fix is still incomplete for invalid grouping() outside the rewrite paths.

bind_where() only rejects AggregateFunction / window nodes, so grouping() is still treated as an ordinary scalar function there. bind_qualify() has the same problem when it delegates to GroupingChecker, because GroupingChecker explicitly leaves FunctionCall(grouping) untouched.

That means queries like SELECT count() FROM students WHERE GROUPING() = 0 GROUP BY course can still get past planning as a dummy scalar grouping call instead of returning a semantic error, and can still hit the dummy grouping implementation later.

Please extend the validation/rewrite so the non-rewritten clauses are covered as well (at least WHERE, and likely QUALIFY), and add regression coverage for one of those forms.

[sundy-li]

Verified the fix is still incomplete outside the aggregate-rewrite paths.

replace_grouping() adds the zero-arg guard in src/query/sql/src/planner/binder/aggregate.rs, but bind_where() in src/query/sql/src/planner/binder/select.rs never runs AggregateRewriter; it only rejects aggregate/window nodes, so a raw GROUPING() call is still treated as an ordinary scalar there. Grouped QUALIFY has the same gap because bind_qualify() delegates to GroupingChecker, and GroupingChecker explicitly leaves grouping function calls untouched.

Please extend the semantic validation/rewrite to the non-rewritten clauses and add regression coverage for cases like:

• SELECT count() FROM students WHERE GROUPING() = 0 GROUP BY course
• SELECT count() OVER () FROM students GROUP BY course QUALIFY GROUPING() = 0

[sundy-li]

The WHERE/QUALIFY fix is in place, but invalid raw GROUPING() is still not rejected in JOIN ... ON. Join predicates use a separate binder path and never hit AggregateRewriter, so this still needs one more semantic check before the PR is ready.

[sundy-li]

bind_where() and bind_qualify() are covered now, but raw GROUPING() can still bypass validation through JOIN ... ON.

src/query/sql/src/planner/binder/bind_table_reference/bind_join.rs still binds ON predicates with ScalarBinder only, and check_join_allowed_scalar_expr() only rejects aggregate/window/async nodes. There is no grouping guard there, and join predicates never go through AggregateRewriter, so queries like:

SELECT count()
FROM students s1
JOIN students s2 ON GROUPING() = 0
GROUP BY s1.course;

still leave a raw FunctionCall(grouping) in the join condition instead of returning a semantic error. This path still needs to be rejected before the PR is ready.

[sundy-li]

Two blockers remain:

1. src/query/sql/src/planner/binder/qualify.rs only checks is_raw_grouping_function(). QUALIFY resolves select-list aliases after analyze_aggregate_select(), so an alias like SELECT GROUPING(course) AS g, count() OVER () FROM students GROUP BY GROUPING SETS ((course), ()) QUALIFY g = 0 binds g to the rewritten grouping<...>(_grouping_id) form and bypasses the new guard. The helper already says Qualify clause can't contain grouping functions, but this path still slips through.

2. src/query/sql/src/planner/binder/bind_table_reference/bind_join.rs is still missing the same validation for join predicates. check_join_allowed_scalar_expr() only rejects aggregate/window/async nodes, and join conditions never run through AggregateRewriter, so invalid GROUPING() calls in JOIN ... ON still avoid the intended semantic error path.

[sundy-li]

A blocking path is still uncovered: JOIN ... ON never validates raw GROUPING() calls.

src/query/sql/src/planner/binder/bind_table_reference/bind_join.rs still binds join predicates with ScalarBinder only, and check_join_allowed_scalar_expr() only rejects aggregate/window/async nodes. Because join predicates never run through AggregateRewriter, a query like:

SELECT count()
FROM students s1
JOIN students s2 ON GROUPING() = 0
GROUP BY s1.course;

still plans successfully instead of returning the semantic error this PR is meant to enforce. I verified that locally by temporarily extending the new semantic test with that case; plan_sql() returned Ok(...) with a raw FunctionCall(grouping) left in the pushed-down join predicates. This needs the same kind of guard that was added for WHERE/QUALIFY, plus regression coverage for the join path.

[sundy-li]

Two blockers still remain in the current tip:

1. src/query/sql/src/planner/binder/qualify.rs only checks is_raw_grouping_function(). QUALIFY resolves select-list aliases before that check, so an alias like SELECT GROUPING(course) AS g, count() OVER () FROM students GROUP BY GROUPING SETS ((course), ()) QUALIFY g = 0 binds g to the rewritten grouping<...>(_grouping_id) form and bypasses the new guard.

2. src/query/sql/src/planner/binder/bind_table_reference/bind_join.rs still only rejects aggregate/window/async nodes in join predicates. JOIN ... ON never goes through AggregateRewriter, so raw GROUPING() in join conditions is still accepted instead of returning the semantic error this PR is meant to enforce.

[sundy-li]

Blocking issue:

JOIN ... ON still accepts raw GROUPING() calls. The new checks cover WHERE and QUALIFY, but src/query/sql/src/planner/binder/bind_table_reference/bind_join.rs still validates join predicates with check_join_allowed_scalar_expr(), which only rejects aggregate/window/async nodes. Join predicates never go through AggregateRewriter, so a query like:

SELECT count()
FROM students s1
JOIN students s2 ON GROUPING() = 0
GROUP BY s1.course;

still leaves a raw FunctionCall(grouping) in the join condition instead of returning the semantic error this PR is meant to enforce. This path needs the same kind of guard as WHERE, plus regression coverage for the join case.

[sundy-li]

I found two remaining gaps in the current diff:

1. bind_qualify() still only looks for raw grouping calls. Because QUALIFY resolves select-list aliases after analyze_aggregate_select(), a query like:

SELECT GROUPING(course) AS g, count() OVER ()
FROM students
GROUP BY GROUPING SETS ((course), ())
QUALIFY g = 0

still binds to the already-rewritten grouping<...>(_grouping_id) expression and slips past is_raw_grouping_function(). This needs the same full is_grouping_function() check that bind_where() now uses, plus a regression covering the alias form.

2. JOIN ... ON is still unguarded. JoinConditionResolver::check_join_allowed_scalar_expr() in src/query/sql/src/planner/binder/bind_table_reference/bind_join.rs still only rejects aggregate/window/async nodes, and join predicates never go through AggregateRewriter, so queries like:

SELECT count()
FROM students s1
JOIN students s2 ON GROUPING() = 0
GROUP BY s1.course

still leave a raw FunctionCall(grouping) in the join predicate instead of returning a semantic error. That path needs the same semantic validation before this is ready.

[sundy-li]

I don't think this is complete yet. The new checks only cover AggregateRewriter plus WHERE/QUALIFY, so invalid raw grouping() calls can still enter other binder contexts without being turned into a semantic error.

Two concrete gaps from the current code:

• JOIN ... ON: check_join_allowed_scalar_expr() in src/query/sql/src/planner/binder/bind_table_reference/bind_join.rs still only rejects aggregate/window/async functions, not grouping().
• GROUP BY items: the validation in src/query/sql/src/planner/binder/aggregate.rs only rejects aggregate/window functions as well.

Because src/query/expression/src/constant_folder.rs now short-circuits grouping, queries like SELECT * FROM t1 JOIN t2 ON GROUPING() = 0 or SELECT 1 FROM t GROUP BY GROUPING() will no longer hit the old folding panic, but they also still are not rejected with a semantic error here. I think this needs one more common raw-grouping() guard (or equivalent clause coverage) before the fix is complete.

[sundy-li]

Review notes from the current head:

• src/query/sql/src/planner/binder/bind_table_reference/bind_join.rs:761 still validates join predicates by rejecting only aggregate/window/async nodes. JOIN ... ON never runs through AggregateRewriter, so raw GROUPING() in a join condition still bypasses the semantic-error path this PR is meant to add.

• src/query/sql/src/planner/binder/qualify.rs:83 only checks is_raw_grouping_function(). QUALIFY resolves select-list aliases after analyze_aggregate_select(), so aliased or already-rewritten grouping<...>(_grouping_id) expressions can still slip through even though this clause is supposed to reject grouping functions.

These two paths still need coverage before this should pass review.

[github-actions]

🤖 CI Job Analysis (Retry 1)

Workflow: 23518802533

📊 Summary

• Total Jobs: 86
• Failed Jobs: 4
• Retryable: 0
• Code Issues: 4

❌ NO RETRY NEEDED

All failures appear to be code/test issues requiring manual fixes.

🔍 Job Details

• ❌ linux / sqllogic / cluster (duckdb, 4c, hybrid): Not retryable (Code/Test)
• ❌ linux / sqllogic / standalone (duckdb, 4c, http): Not retryable (Code/Test)
• ❌ linux / sqllogic / cluster (duckdb, 4c, http): Not retryable (Code/Test)
• ❌ linux / sqllogic / standalone (duckdb, 4c, hybrid): Not retryable (Code/Test)

---

🤖 About

Automated analysis using job annotations to distinguish infrastructure issues (auto-retried) from code/test issues (manual fixes needed).

[sundy-li]

Blocking issue:

src/query/sql/src/planner/binder/qualify.rs:82 now rejects every grouping(...) call in QUALIFY. That turns the current rewrite gap into a behavior regression instead of just fixing the panic path.

The repo already supports grouping(...) in other post-aggregate/window expressions. For example, tests/sqllogictests/suites/query/window_function/window_basic.test:623 uses grouping(...) inside a window partition/order expression after ROLLUP, which shows the planner can handle grouping after aggregation. But this patch adds SELECT GROUPING(a) AS g, count() OVER () FROM t GROUP BY GROUPING SETS ((a), ()) QUALIFY g = 0 as a negative case and rejects it via the new blanket reject_grouping_functions(...) check.

QUALIFY is evaluated after grouping/windowing, so grouping(...) should be rewritten/validated there like other post-aggregate uses, not rejected outright. Please handle the QUALIFY path with the rewrite/validation logic instead of banning the function in that clause.

[sundy-li]

Blocking issue:

src/query/sql/src/planner/binder/qualify.rs now rejects every grouping(...) call via reject_grouping_functions(...). That fixes the raw GROUPING() panic path, but it also rejects already-rewritten grouping<...>(_grouping_id) expressions that the planner can otherwise carry safely through post-aggregate/window phases.

This is a behavior regression for alias-expanded QUALIFY, e.g. SELECT GROUPING(a) AS g, count() OVER () FROM t GROUP BY GROUPING SETS ((a), ()) QUALIFY g = 0. In this path g resolves after analyze_aggregate_select() to the rewritten form, and bind_select() applies QUALIFY after both aggregation and window binding, so blanket rejection is stronger than necessary. The existing positive coverage in tests/sqllogictests/suites/query/window_function/window_basic.test also shows rewritten grouping(...) can already participate in post-aggregate/window expressions.

I think the fix here needs to distinguish raw/unrewritten GROUPING() calls in QUALIFY from already-rewritten ones, rather than banning the function for the whole clause.

[sundy-li]

Updated in 4b6a979.

Resolved items from review:

• QUALIFY now runs AggregateRewriter, so direct GROUPING(...) there is rewritten/validated instead of being blanket-rejected.
• Already-rewritten internal grouping<...>(_grouping_id) aliases are no longer rewritten a second time, which fixes QUALIFY g = 0 for SELECT GROUPING(...) AS g ....
• The existing non-rewrite clause guards remain in place for WHERE, JOIN ... ON, and GROUP BY items.

Fresh verification on this commit:

• cargo test -p databend-query --test it sql::planner::semantic::type_check -- --nocapture
• cargo build --bin databend-query --bin databend-sqllogictests
• TEST_HANDLERS=http TEST_PARALLEL=1 BUILD_PROFILE=debug TEST_EXT_ARGS='--run_file group_by_grouping_sets.test' bash ./scripts/ci/ci-run-sqllogic-tests.sh duckdb
• cargo fmt --all --check
• cargo clippy -p databend-common-expression -p databend-common-sql --lib -- -D warnings

[sundy-li]

Using AggregateRewriter here rewrites ordinary aggregate expressions too, not just grouping(). That means bind_qualify() no longer sees ScalarExpr::AggregateFunction for queries like SELECT count() OVER () FROM t QUALIFY count() > 0, so the existing Qualify clause must not contain aggregate functions check is bypassed and the query can plan through the aggregate path. We need a narrower rewrite here that only handles grouping() or another way to preserve the aggregate rejection.

[sundy-li]

Running AggregateRewriter here looks unsafe. bind_qualify() still relies on seeing ScalarExpr::AggregateFunction at lines 74-81 to reject aggregates in QUALIFY, but this rewrite turns them into bound columns before that check runs. That means queries like SELECT 1 FROM t QUALIFY sum(x) > 0 can start planning successfully instead of raising Qualify clause must not contain aggregate functions. Can we keep the grouping() fix without rewriting general aggregates before the existing semantic check?

[sundy-li]

Marking this PR as changes requested from the agent side. The new QUALIFY rewrite can hide aggregate functions before the existing semantic check runs, which appears to allow aggregates in QUALIFY unexpectedly. See the inline comment on src/query/sql/src/planner/binder/qualify.rs for details.