Fix NOT_FOUND_COLUMN_IN_BLOCK for GROUP BY ALL over a tuple with ORDER BY#110206
Conversation
…R BY `GROUP BY ALL` adds the SELECT expressions as grouping keys in `expandGroupByAll`, which runs after `resolveGroupByNode` has already unwrapped tuples in the GROUP BY list. So a projection like `tuple(a, b)` became a single grouping key, unlike an explicit `GROUP BY tuple(a, b)` (or `GROUP BY (a, b)`) which `expandTuplesInList` turns into the separate keys `a` and `b`. `OrderByTupleEliminationPass` later rewrites `ORDER BY tuple(a, b)` into `ORDER BY a, b`. With the tuple kept whole as a grouping key, only the tuple column is available after aggregation, so the rewritten `ORDER BY` referenced the missing elements `a` and `b` and threw `NOT_FOUND_COLUMN_IN_BLOCK`. This only reproduced with the analyzer and with `optimize_injective_functions_in_group_by = 0` (with the setting enabled, `OptimizeGroupByInjectiveFunctionsPass` unwraps the tuple key anyway). Unwrap tuples in the `GROUP BY ALL` keys too, so `GROUP BY ALL` produces the same normalized grouping keys as an explicit `GROUP BY`. Closes: #83433 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Workflow [PR], commit [9d67075] Summary: ✅
AI ReviewSummaryThis PR makes the new analyzer normalize Final VerdictStatus: ✅ Approve |
clickhouse-gh
Bot
added
the
pr-bugfix
The Style check `test_numbers_check` reported `Gap (4515, 5020) > 100`: the new stateless test was numbered `05020`, far above the current maximum on `master` (`04515`), which the numbering gap check rejects. Renumber it to `04518` (within the allowed gap and clear of the numbers reserved by other in-flight pull requests) so the workflow no longer aborts at the Style check, letting the downstream Bugfix validation jobs run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| /// rewrites `ORDER BY tuple(a, b)` into `ORDER BY a, b`) would reference columns missing from the | ||
| /// aggregated block. See https://github.com/ClickHouse/ClickHouse/issues/83433. | ||
| if (was_group_by_all) | ||
| expandTuplesInList(query_node_typed.getGroupBy().getNodes()); |
There was a problem hiding this comment.
Late tuple expansion here still leaves GROUP BY ALL different from an explicit GROUP BY tuple(...) for the supported WITH ROLLUP / WITH CUBE forms. resolveGroupByNode is where group_by_use_nulls registers nullable versions of the keys and forces projection/order-by re-resolution; by the time we reach this block those steps have already run against an empty GROUP BY list.
That means queries such as:
SELECT (number, number + 1), count()
FROM numbers(1)
GROUP BY ALL WITH ROLLUP
SETTINGS enable_analyzer = 1, group_by_use_nulls = 1;still diverge from explicit GROUP BY (number, number + 1): the projection keeps non-nullable children, but validateAggregates later compares them against nullable rollup keys and rejects the query. To make GROUP BY ALL actually match explicit GROUP BY, the expansion needs to happen before resolveGroupByNode, or we need to rerun the same key-validation / nullable-key bookkeeping after expanding here.
There was a problem hiding this comment.
The diagnosis of the mechanism is correct — for GROUP BY ALL the GROUP BY list is empty until expandGroupByAll, so hasGroupBy() is false and resolveGroupByNode (where group_by_use_nulls registers the nullable keys and forces the projection/ORDER BY re-resolution) is skipped entirely. But this is a separate, pre-existing bug, unrelated to this PR's change and unrelated to tuples.
It reproduces on master (no tuple, plain scalar key), with the same NOT_AN_AGGREGATE rejection:
SELECT number, count() FROM numbers(1)
GROUP BY ALL WITH ROLLUP
SETTINGS enable_analyzer = 1, group_by_use_nulls = 1;
-- Code: 215. DB::Exception: Column 'number' is not under aggregate function
-- and not in GROUP BY keys ... (NOT_AN_AGGREGATE)while the explicit GROUP BY number WITH ROLLUP form works. So the divergence is GROUP BY ALL + group_by_use_nulls, not tuple expansion: it fires with a scalar key and without any tuple, and this PR's added code (expandTuplesInList gated on was_group_by_all) runs after the nullable-key machinery and only touches tuple keys, so it neither causes nor could fix it.
This PR is a minimal, targeted fix for #83433, whose repro uses neither group_by_use_nulls nor WITH ROLLUP/CUBE. Making GROUP BY ALL participate in the full resolveGroupByNode bookkeeping is a genuinely separate change: expandGroupByAll reads the resolved projection, and with group_by_use_nulls the projection is only resolved after GROUP BY is processed, so the expansion cannot simply be moved before resolveGroupByNode — it needs a second nullable-key-registration + re-resolution pass, which warrants its own PR and dedicated group_by_use_nulls regression coverage rather than being folded into this surgical fix.
…ple-order-by # Conflicts: # src/Analyzer/Resolve/QueryAnalyzer.cpp
|
Merged On the previous run the only red was the which aborts the server in the coverage build, so the following tests failed with Locally after merging |
|
The only red on the previous run,
Merged |
|
Merged Both reds on the previous run are known master-side flakes, unrelated to this analyzer-only change:
|
|
Investigated #110133. This is a regression from #108728 (merged 2026-07-07 20:55 UTC), not a long-standing flake: CIDB shows Root cause matches your #110048 analysis: #108728 added freshness checks to the per-query storage cache ( A fix is already in progress, so no new PR is needed:
|
`GROUP BY ALL` expands the SELECT expressions into grouping keys in `expandGroupByAll`, which runs after `resolveGroupByNode` already validated the equivalent explicit `GROUP BY` keys via `validateGroupByKeyType`. So a suspicious key type such as `Dynamic`/`Variant` -- rejected by an explicit `GROUP BY` under `allow_suspicious_types_in_group_by = 0` -- was silently accepted by `GROUP BY ALL`, including for such a type nested inside a tuple grouping key. Run the same `validateGroupByKeyType` check over the expanded `GROUP BY ALL` keys, so both forms reject the same types. Addresses review feedback on this PR.
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 9/9 (100.00%) · Uncovered code |
|
Found via ClickGap automated review. Please close or comment if this is incorrect or needs adjustment. TL;DRFix NOT_FOUND_COLUMN_IN_BLOCK for GROUP BY ALL over a tuple with ORDER BY ClickGap verified: reproduced it with the script below on ReproductionSET enable_analyzer = 1;
SELECT ((v0.c0, v0.c1)) FROM (SELECT 1 c0, 2 c1) v0 GROUP BY ALL ORDER BY ALL
SETTINGS optimize_injective_functions_in_group_by = 0;
-- Expected: (1,2)
-- Before the fix: Code: 10. DB::Exception: Not found column __table1.c0 in block tuple(__table1.c0, __table1.c1) (NOT_FOUND_COLUMN_IN_BLOCK)Bisect
Affects
Suggested next stepThe fix has landed on master; the affected release branches above still need the backport. Component: ClickGap · Finding: Triage metadata
|
robot-clickhouse-ci-2
added
the
pr-synced-to-cloud
4 participants
Closes: #83433
GROUP BY ALLover a tuple projection combined withORDER BYthrewNOT_FOUND_COLUMN_IN_BLOCKwith the analyzer:GROUP BY ALLadds the SELECT expressions as grouping keys inexpandGroupByAll, which runs afterresolveGroupByNodehas already unwrapped tuples in theGROUP BYlist. So a projection liketuple(a, b)became a single grouping key, unlike an explicitGROUP BY tuple(a, b)(orGROUP BY (a, b)), whichexpandTuplesInListturns into the separate keysaandb.OrderByTupleEliminationPassthen rewritesORDER BY tuple(a, b)intoORDER BY a, b. With the tuple kept whole as a grouping key, only the tuple column is available after aggregation, so the rewrittenORDER BYreferenced the missing elementsaandband threw the exception. This only reproduced withoptimize_injective_functions_in_group_by = 0; with the setting enabled,OptimizeGroupByInjectiveFunctionsPassunwraps the tuple key anyway.The fix unwraps tuples in the
GROUP BY ALLkeys too, soGROUP BY ALLproduces the same normalized grouping keys as an explicitGROUP BY.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed a
NOT_FOUND_COLUMN_IN_BLOCKerror when usingGROUP BY ALLover a tuple expression together withORDER BY.Documentation entry for user-facing changes
Version info
26.7.1.1037(included in26.7and later)