fix: rank rollup window results after union

[LeftHandCold]

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue #25499

What this PR does / why we need it:

GROUP BY ... WITH ROLLUP is expanded into multiple UNION ALL branches. When the select list also contains window functions, the previous plan copied those window expressions into every rollup branch, so ROW_NUMBER, RANK, and DENSE_RANK were computed per grouping level instead of over the complete rollup result.

This change rewrites supported rollup/cube window queries as an inner rollup union without window expressions plus an outer select that computes the windows over the full union output. The rewrite is conservative: it only triggers when window dependencies can be mapped to inner output aliases or bare columns; unsupported shapes keep the existing path.

Tested with:

go test -count=1 -mod=mod ./pkg/sql/plan -run 'TestRollupWindowRanksAfterRollupUnion|TestSingleTableSQLBuilder'

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[XuPeng-SH]

Findings

High: the rewrite conflates source-column identity with SELECT/output aliases and can silently change the window key or reject a previously valid query

lookupExprAlias falls back from a structural-expression lookup to nameAliases. buildRollupWindowSelectExprs populates that map with every SELECT alias, then uses the same lookup from ensureInnerExpr and while rewriting the window specification. A SELECT alias is not a source-column identity, and a window ORDER BY is bound in the source/group scope.

This accepted query demonstrates both effects:

select n_name as n_regionkey, n_regionkey, sum(n_nationkey),
       row_number() over (order by n_regionkey) as rn
from nation
group by n_name, n_regionkey with rollup;

Without the rollup rewrite, the window key resolves to nation.n_regionkey (an integer). On c4cf8a5e, the alias map makes the second selected n_regionkey reuse the synthetic column for n_name; the outer window consequently orders by that varchar column, and the final result projects n_name twice. This is a silent wrong-result regression, not just an explain-shape difference.

There is also a hard failure when the window output uses the same name as the source column:

select n_name, n_regionkey, sum(n_nationkey),
       row_number() over (order by n_regionkey) as n_regionkey
from nation
group by n_name, n_regionkey with rollup;

The base commit 71aa96600 builds plans for both queries. The PR head returns invalid input: column n_regionkey does not exist for the second one because the output-alias entry prevents the source column from being materialized in the inner UNION.

First-principles fix: keep separate, scope-aware mappings. Structural/source expressions may share a synthetic inner projection, while SELECT labels may be resolved only in clauses where the normal binder permits output aliases. Do not use the output-name map to deduplicate a selected source expression or to rewrite window arguments, PARTITION BY, ORDER BY, or frame expressions. Preserve the binder's source-scope resolution for those expressions; use a distinct output-alias mapping for final ORDER BY/other alias-aware clauses as appropriate.

Please add regression coverage that validates both the result columns and the window sort key for a source-column/SELECT-alias collision, plus the same-name window-output case. The existing tests cover generated aliases and hidden aggregates, but neither collision path.

[XuPeng-SH]

Blocking issue:

• The new rollup-window rewrite changes HAVING name resolution precedence. In normal planning, HAVING uses AliasAfterColumn, so a bare name resolves to a real grouped/source column before falling back to a select alias. But the rewrite path feeds HAVING through rewriteRollupWindowExprWithNameAliases(..., rewriteState.havingAliases), and lookupExprAlias() consults those aliases first for one-part names.

That means a query like:

select
  sum(n_nationkey) as n_regionkey,
  n_regionkey,
  row_number() over (order by n_regionkey)
from nation
group by n_regionkey with rollup
having n_regionkey > 0;

can have the rewritten inner HAVING bound to the aliased aggregate sum(n_nationkey) instead of the original grouped column n_regionkey. So the rewrite is not semantics-preserving for alias/source-name collisions.

Suggestion:

• Preserve the normal AliasAfterColumn behavior when rewriting HAVING: only use the select-alias remap if the bare name does not resolve to an original grouped/source expression.
• Add a regression for a rollup window query where a select alias intentionally collides with a grouped column name and HAVING references that bare name.

[XuPeng-SH]

This still needs changes.

The new HAVING precedence fix only handles source expressions recorded under one-part names. In pkg/sql/plan/query_builder.go, both addGroupedSourceExprs and addExprAlias populate sourceNameAliases only when the grouped/source expression is an UnresolvedName with NumParts == 1. rewriteRollupWindowExprWithFallbackNameAliases then relies on that map to preserve AliasAfterColumn in HAVING.

That leaves a valid query misbound whenever the grouped source column is written as a qualified name but HAVING references the bare column name:

select sum(t.n_nationkey) as n_regionkey,
t.n_regionkey,
row_number() over (order by t.n_regionkey) as rn
from nation t
group by t.n_regionkey with rollup
having n_regionkey > 0;

On latest head afc33b6, I reproduced this with a scratch planner test: the rollup FILTER left argument is typed as T_int64 (the aggregate alias) instead of T_int32 (the grouped source column). So the rewrite still changes HAVING semantics for alias/source collisions; it just no longer does so for the unqualified case covered by the new tests.

Suggestion: preserve bare-name source resolution for qualified grouped/source columns as well. Record the terminal column name from materialized grouped/source expressions into the AliasAfterColumn source map, but only when that bare name is unambiguous; otherwise leave it unresolved so ambiguity is surfaced normally instead of silently picking one alias. Please add a regression for qualified source columns plus a colliding SELECT alias in HAVING.

[XuPeng-SH]

Blocking correctness issue on the latest head c8a9c20667:

HAVING source-first resolution only sees grouped expressions, not the complete FROM scope

addGroupedSourceExprs records terminal names from materialized GROUP BY expressions in sourceNameAliases, and lookupExprAlias then treats a name present in that map as an unambiguous source-column match before consulting the SELECT alias. This approximates AliasAfterColumn using only the grouped expressions. The normal binder resolves against every binding in the FROM scope first, including ambiguous and non-grouped source columns.

A deterministic counterexample is:

select n1.n_regionkey,
       row_number() over (order by n1.n_regionkey) as rn
from nation n1
join nation n2 on n1.n_nationkey = n2.n_nationkey
group by n1.n_regionkey with rollup
having n_regionkey > 0;

On this head, the same query without the window function correctly returns an ambiguous column reference error for n_regionkey.

Adding only row_number() activates the rewrite and the query is accepted. The rewrite has seen only grouped n1.n_regionkey, marks the terminal name as unique, and replaces the bare HAVING reference with the internal alias for n1.n_regionkey. The original ambiguity with n2.n_regionkey never reaches BindContext.bindingByCol.

This is a silent semantic change: adding a window function changes name resolution and turns an ambiguous query into a plan that implicitly selects one source column.

Please preserve AliasAfterColumn using the actual pre-rewrite binding scope rather than reconstructing source identity from GROUP BY AST nodes. A SELECT alias may be used only after normal source lookup reports no matching column; ambiguity and grouped-column validation errors must propagate unchanged. Please add a regression with two joined tables exposing the same bare column name where only one qualified column is grouped, and verify that adding the window function does not change the error.

The previous blockers around unaliased aggregates, hidden window dependencies, SELECT/source alias collisions, qualified grouped columns, and branch-local ranking look fixed. The focused rollup-window suite passes, and I found no additional high-confidence performance, leak, hang, or unbounded-growth blocker.