Skip to content

fix(plan): support GROUPING expressions in ROLLUP ORDER BY#25335

Open
ck89119 wants to merge 14 commits into
matrixorigin:mainfrom
ck89119:issue-25287-main
Open

fix(plan): support GROUPING expressions in ROLLUP ORDER BY#25335
ck89119 wants to merge 14 commits into
matrixorigin:mainfrom
ck89119:issue-25287-main

Conversation

@ck89119

@ck89119 ck89119 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #25287

What this PR does / why we need it:

ROLLUP, CUBE, and GROUPING SETS are expanded into an internal UNION ALL plan. The original ORDER BY is bound above that union, where source columns referenced by expressions such as GROUPING(region) are no longer visible even when the same expression is already projected.

This change rewrites an ORDER BY expression that exactly matches a select-list expression to its projection ordinal before building the internal grouping-set union. It reuses the projected value, preserves the final output schema, and does not relax the rules for explicit user-written set operations.

The change also adds planner unit coverage and a distributed ROLLUP regression case with real NULL values.

Validation:

  • go test -count=1 ./pkg/sql/plan
  • go build ./pkg/sql/plan/...
  • go vet ./pkg/sql/plan/...
  • modified helper coverage: 100%
  • make
  • rollup BVT: 112 passed, 0 failed, 2 ignored
  • git diff --check

make static-check remains blocked by existing pkg/common/docfilter CGo unsafe molint violations; the same failure was reproduced on a clean main checkout.

@qodo-code-review

Copy link
Copy Markdown

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 →

@matrix-meow matrix-meow added size/L Denotes a PR that changes [500,999] lines and removed size/M Denotes a PR that changes [100,499] lines labels Jul 3, 2026

@aunjgr aunjgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrite by AST text is fragile — GROUPING(a) vs GROUPING(t.a) will always mismatch. Should resolve to canonicalized columns after name binding, not before.

The rollup projection-order fix removed duplicate subtotal/grand-total
rows, so the CTE+ROLLUP case in hint_cte now returns 10 rows instead of
the stale 15. Update the expected result accordingly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on the latest head. The earlier qualified/nested GROUPING() issues look much better, but the new hidden-order-key approach currently bypasses the existing SELECT DISTINCT ORDER BY rule and can change visible DISTINCT results.

Repro on this head:

select distinct grouping(a)
from select_test.bind_select
group by a with rollup
order by grouping(a) + a;

For comparison, the ordinary path correctly rejects the same shape:

select distinct a
from select_test.bind_select
order by a + 1;
-- for SELECT DISTINCT, ORDER BY expressions must appear in select list

The reason is that prepareGroupingSetOrderByProjects appends every ORDER BY expression containing GROUPING() as a hidden branch projection and rewrites the UNION ORDER BY to an ordinal (query_builder.go:3843-3883). Those hidden projections are then included in each generated branch while preserving Distinct: selectClause.Distinct (query_builder.go:3101-3103). By the time buildUnionWithResultLen binds ORDER BY, it sees only an ordinal, so the normal distinct guard in OrderBinder (order_binder.go:140-143) is never reached.

This is not only an error-policy mismatch: if the query is allowed, the hidden key participates in branch-level DISTINCT and is then trimmed from the final output, so different hidden keys can become duplicate visible rows after trimming.

Suggested fix: preserve the existing DISTINCT contract before injecting hidden grouping-set order keys. For SELECT DISTINCT, only rewrite to a hidden key when the ORDER BY expression is already a visible select-list expression (or rewrite to that visible ordinal); otherwise return the same error as the ordinary ORDER BY path. Please add regression coverage for the repro above and the allowed case where the full ORDER BY expression is selected.

ck89119 and others added 2 commits July 6, 2026 10:57
For SELECT DISTINCT over ROLLUP/CUBE, prepareGroupingSetOrderByProjects
injected every GROUPING() ORDER BY expression as a hidden branch
projection. That hidden key participated in branch-level DISTINCT and was
then trimmed, bypassing the OrderBinder DISTINCT guard and letting
distinct hidden keys collapse into duplicate visible rows.

Now, for DISTINCT, an ORDER BY expression is only rewritten when it
already matches a visible select-list expression (rewritten to that
visible ordinal); otherwise it is rejected with the same error as the
ordinary ORDER BY path. Non-distinct behavior is unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
For SELECT DISTINCT over ROLLUP/CUBE, the ORDER BY expression key used to
match against visible select-list expressions was computed via tree.String
without stripping table qualifiers or unwrapping redundant parentheses.
This meant GROUPING(t.a) and GROUPING(a) produced different keys, so the
DISTINCT guard could reject queries where the ORDER BY expression was
already in the select list under a different spelling.

Add normalizeGroupingArgsForComparison which strips table qualifiers
(UnresolvedName.NumParts → 1) and unwraps ParenExpr from GROUPING()
arguments before computing the comparison key.

Note: the MO parser currently rejects GROUPING(t.a) and GROUPING((a)),
so these cannot be reached through SQL today. The normalization serves as
defense-in-depth for when the parser is extended, and for any scenario
where qualifyColumnNames introduces qualifiers after alias expansion.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants