Skip to content

Commit e48d69e

Browse files
committed
Fix logical error in correlated subqueries with group_by_use_nulls
A correlated scalar subquery referencing an outer `GROUP BY` key that becomes `Nullable` via `group_by_use_nulls` (`GROUPING SETS` / `ROLLUP` / `CUBE` / `WITH TOTALS`) aborted with: Logical error: Unexpected return type from toString. Expected String. Got Nullable(String) The `group_by_use_nulls` nullability walk in `resolveExpressionNode` stops at the subquery's own query scope, so a correlated column kept its non-`Nullable` type while decorrelation fed in the real `Nullable` column, mismatching a baked function result type (`toString` -> `String` vs `Nullable(String)`). Continue the walk past a subquery's query boundary for correlated columns and convert the column to `Nullable` in place, preserving the node identity shared with the subquery's correlated-columns set (which the planner matches by identity). Discovered by the AST fuzzer in the `amd_msan` stress test: https://github.com/ClickHouse/ClickHouse/actions/runs/28286443407/job/83839128485?pr=108685 Related: ClickHouse#108685
1 parent b17aed5 commit e48d69e

3 files changed

Lines changed: 120 additions & 9 deletions

File tree

‎src/Analyzer/Resolve/QueryAnalyzer.cpp‎

Lines changed: 54 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3075,6 +3075,24 @@ ProjectionNames QueryAnalyzer::resolveLambda(const QueryTreeNodePtr & lambda_nod
30753075
*
30763076
* 4. If node has alias, update its value in scope alias map. Deregister alias from expression_aliases_in_resolve_process.
30773077
*/
3078+
3079+
/** Return true if `scope` owns the source of the column `node`, i.e. the column's source table
3080+
* expression is registered (or being resolved) in this scope's FROM section. A column whose
3081+
* source is not owned by `scope` but by an ancestor query scope is a correlated column.
3082+
*
3083+
* Read-only: unlike `checkCorrelatedColumn`, this does not register the column in any
3084+
* `addCorrelatedColumn` set; it only inspects scope ownership.
3085+
*/
3086+
static bool scopeOwnsColumnSource(const IdentifierResolveScope * scope, const QueryTreeNodePtr & node)
3087+
{
3088+
if (node->getNodeType() != QueryTreeNodeType::COLUMN)
3089+
return true;
3090+
3091+
const auto column_source = node->as<ColumnNode>()->getColumnSource();
3092+
return scope->registered_table_expression_nodes.contains(column_source)
3093+
|| scope->table_expressions_in_resolve_process.contains(column_source.get());
3094+
}
3095+
30783096
ProjectionNames QueryAnalyzer::resolveExpressionNode(
30793097
QueryTreeNodePtr & node,
30803098
IdentifierResolveScope & scope,
@@ -3519,27 +3537,54 @@ ProjectionNames QueryAnalyzer::resolveExpressionNode(
35193537

35203538
if (!in_aggregate_or_grouping_function_scope)
35213539
{
3540+
/// Set once the scope walk passes a subquery's own query scope to reach the outer query
3541+
/// that owns a correlated column's source. Distinguishes a correlated reference (handled
3542+
/// in place below) from an ordinary one (cloned).
3543+
bool crossed_query_boundary = false;
35223544
for (const auto * scope_ptr = &scope; scope_ptr; scope_ptr = scope_ptr->parent_scope)
35233545
{
35243546
if (!scope_ptr->nullable_group_by_keys.empty())
35253547
{
35263548
auto it = scope_ptr->nullable_group_by_keys.find(node);
35273549
if (it != scope_ptr->nullable_group_by_keys.end())
35283550
{
3529-
/// Clone the GROUP BY key and convert it to Nullable. For a constant we clone the
3530-
/// matched node itself rather than the stored key `it->second`: two constants equal
3531-
/// in value and type but with different source expressions share a single map entry,
3532-
/// and the source expression determines the action node name (hence which aggregation
3533-
/// key column the projection reads), so the matched node's own one must be preserved.
3534-
node = (node->getNodeType() == QueryTreeNodeType::CONSTANT ? node : it->second)->clone();
3535-
node->convertToNullable();
3551+
if (crossed_query_boundary)
3552+
{
3553+
/// Correlated column referencing an outer nullable GROUP BY key. Convert it
3554+
/// in place: the same node object is shared with the subquery's
3555+
/// correlated-columns set (populated during identifier resolution), and the
3556+
/// planner matches that set against the decorrelated input by identity.
3557+
/// Cloning here (as in the ordinary case) would desynchronize the set from
3558+
/// the usage and break decorrelation ("Not found column ...").
3559+
node->convertToNullable();
3560+
}
3561+
else
3562+
{
3563+
/// Clone the GROUP BY key and convert it to Nullable. For a constant we clone the
3564+
/// matched node itself rather than the stored key `it->second`: two constants equal
3565+
/// in value and type but with different source expressions share a single map entry,
3566+
/// and the source expression determines the action node name (hence which aggregation
3567+
/// key column the projection reads), so the matched node's own one must be preserved.
3568+
node = (node->getNodeType() == QueryTreeNodeType::CONSTANT ? node : it->second)->clone();
3569+
node->convertToNullable();
3570+
}
35363571
break;
35373572
}
35383573
}
35393574

3540-
/// Check parent scopes until find current query scope.
3575+
/// Normally we stop at the current query scope: a column belonging to this query must
3576+
/// not pick up an ancestor query's GROUP BY nullability. A correlated column is the
3577+
/// exception — its source lives in an outer query, so `group_by_use_nulls` on that
3578+
/// outer GROUP BY makes the correlated value Nullable and we must keep walking up to
3579+
/// the scope that owns it. Without this, a correlated reference to an outer nullable
3580+
/// GROUP BY key stays non-Nullable while the decorrelated plan feeds in the real
3581+
/// Nullable column, producing a type mismatch ("Unexpected return type ...").
35413582
if (scope_ptr->scope_node->getNodeType() == QueryTreeNodeType::QUERY)
3542-
break;
3583+
{
3584+
if (scopeOwnsColumnSource(scope_ptr, node))
3585+
break;
3586+
crossed_query_boundary = true;
3587+
}
35433588
}
35443589
}
35453590

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
Nullable(String)
2+
0 s0
3+
1 s1
4+
2 s2
5+
0 s0
6+
1 s1
7+
2 s2
8+
9+
\N \N
10+
0 0
11+
1 1
12+
2 2
13+
\N \N
14+
0 0
15+
1 1
16+
\N \N
17+
0 42
18+
1 42
19+
2 42
20+
\N 42
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
-- Regression test: a correlated scalar subquery that references an outer GROUP BY key
2+
-- used to abort with a logical error
3+
-- "Unexpected return type from toString. Expected String. Got Nullable(String)"
4+
-- when `group_by_use_nulls = 1` made the grouping key Nullable (GROUPING SETS / ROLLUP /
5+
-- CUBE / WITH TOTALS). The correlated column stayed non-Nullable while the decorrelated
6+
-- plan fed in the real Nullable column, so the baked result type no longer matched.
7+
8+
SET enable_analyzer = 1;
9+
SET allow_experimental_correlated_subqueries = 1;
10+
SET group_by_use_nulls = 1;
11+
12+
-- The correlated column must be Nullable, hence toString over it must be Nullable(String).
13+
SELECT DISTINCT toTypeName((SELECT toString(number))) AS t
14+
FROM numbers(3)
15+
GROUP BY GROUPING SETS ((number));
16+
17+
-- GROUPING SETS
18+
SELECT number, concat('s', (SELECT toString(number))) AS c
19+
FROM numbers(3)
20+
GROUP BY GROUPING SETS ((number))
21+
ORDER BY number;
22+
23+
-- GROUPING SETS WITH TOTALS
24+
SELECT number, concat('s', (SELECT toString(number))) AS c
25+
FROM numbers(3)
26+
GROUP BY GROUPING SETS ((number))
27+
WITH TOTALS
28+
ORDER BY number;
29+
30+
-- ROLLUP: the super-aggregate row has number = NULL, so the correlated value is NULL.
31+
SELECT number, (SELECT toString(number)) AS c
32+
FROM numbers(3)
33+
GROUP BY number WITH ROLLUP
34+
ORDER BY number NULLS LAST;
35+
36+
-- CUBE
37+
SELECT number, (SELECT toString(number)) AS c
38+
FROM numbers(2)
39+
GROUP BY number WITH CUBE
40+
ORDER BY number NULLS LAST;
41+
42+
-- A non-correlated subquery in the same shape must be unaffected by group_by_use_nulls.
43+
SELECT number, (SELECT 42) AS c
44+
FROM numbers(3)
45+
GROUP BY number WITH ROLLUP
46+
ORDER BY number NULLS LAST;

0 commit comments

Comments
 (0)