Fix inconsistent AST formatting for GROUP BY CUBE(...) WITH ROLLUP by alexey-milovidov · Pull Request #100376 · ClickHouse/ClickHouse · GitHub
Skip to content

Fix inconsistent AST formatting for GROUP BY CUBE(...) WITH ROLLUP#100376

Merged
alexey-milovidov merged 11 commits into
masterfrom
fix-group-by-cube-with-rollup-ast-formatting
Apr 10, 2026
Merged

Fix inconsistent AST formatting for GROUP BY CUBE(...) WITH ROLLUP#100376
alexey-milovidov merged 11 commits into
masterfrom
fix-group-by-cube-with-rollup-ast-formatting

Conversation

@alexey-milovidov

Copy link
Copy Markdown
Member

GROUP BY CUBE(x) WITH ROLLUP was parsed into an AST with both group_by_with_cube and group_by_with_rollup flags set. The formatter then produced GROUP BY x WITH ROLLUP WITH CUBE, but the parser could not parse this back because the second WITH clause only accepted TOTALS. This caused an "Inconsistent AST formatting" exception in debug builds.

Fix by replacing the two separate WITH parser blocks in ParserSelectQuery with a single loop that accepts any combination of WITH ROLLUP, WITH CUBE, and WITH TOTALS modifiers (rejecting duplicates).

Fixes #100320

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix "Inconsistent AST formatting" exception in debug builds when using GROUP BY CUBE(...) WITH ROLLUP or similar combinations.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

`GROUP BY CUBE(x) WITH ROLLUP` was parsed into an AST with both
`group_by_with_cube` and `group_by_with_rollup` flags set. The formatter
then produced `GROUP BY x WITH ROLLUP WITH CUBE`, but the parser could
not parse this back because the second `WITH` clause only accepted
`TOTALS`. This caused a "Inconsistent AST formatting" exception in
debug builds.

Fix by replacing the two separate `WITH` parser blocks with a single
loop that accepts any combination of `WITH ROLLUP`, `WITH CUBE`, and
`WITH TOTALS` modifiers (rejecting duplicates).

Fixes #100320

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Mar 22, 2026

Copy link
Copy Markdown
Contributor

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Mar 22, 2026
…ests

Syntax errors are caught by the client-side parser before being sent
to the server, so they must use `clientError` instead of `serverError`.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@alexey-milovidov

Copy link
Copy Markdown
Member Author

#100298

alexey-milovidov added a commit that referenced this pull request Mar 29, 2026
The test checked specific part names after `OPTIMIZE`, but with
randomized settings (e.g. `enable_parallel_replicas`), the merge
selector can pick different pairs of parts to merge, resulting in
different part names. Check the count of active parts instead, which
is always 2 regardless of merge order.

Seen in #100376

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alexey-milovidov added a commit that referenced this pull request Mar 29, 2026
The test checked specific part names after `OPTIMIZE`, but with
randomized settings the merge selector can pick different pairs of
parts to merge, resulting in different part names. Check the count
of active parts instead, which is always 2 regardless of merge order.

Seen in #100376

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@alexey-milovidov

Copy link
Copy Markdown
Member Author

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

@clickhouse-gh

clickhouse-gh Bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.00% 84.00% +0.00%
Functions 90.90% 90.90% +0.00%
Branches 76.60% 76.50% -0.10%

Changed lines: 88.89% (24/27) | lost baseline coverage: 2 line(s) · Uncovered code

Full report · Diff report

@alexey-milovidov alexey-milovidov left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good, clean change.

@alexey-milovidov alexey-milovidov self-assigned this Apr 10, 2026
@alexey-milovidov alexey-milovidov added this pull request to the merge queue Apr 10, 2026
Merged via the queue into master with commit 9710673 Apr 10, 2026
163 checks passed
@alexey-milovidov alexey-milovidov deleted the fix-group-by-cube-with-rollup-ast-formatting branch April 10, 2026 01:37
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

executeQueryImpl_aborted

2 participants