Skip to content

Commit b4533e7

Browse files
robot-clickhouserobot-clickhouse
committed
Backport #97831 to 26.1: Fix LOGICAL_ERROR exceptions from recursive convertToFullIfNeeded with LowCardinality inside compound types
1 parent 46d34b4 commit b4533e7

13 files changed

+161
-7
lines changed

‎src/Columns/ColumnDynamic.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,12 @@ class ColumnDynamic final : public COWHelper<IColumnHelper<ColumnDynamic>, Colum
332332

333333
void forEachSubcolumn(ColumnCallback callback) const override { callback(variant_column); }
334334

335+
/// Dynamic columns manage their own variant_info type metadata.
336+
/// The default convertToFullIfNeeded recurses into subcolumns and strips LowCardinality
337+
/// from variant columns, but cannot update variant_info, creating column/type mismatches.
338+
/// Override to skip recursion — Dynamic is a self-contained typed container.
339+
[[nodiscard]] IColumn::Ptr convertToFullIfNeeded() const override { return getPtr(); }
340+
335341
void forEachMutableSubcolumnRecursively(RecursiveMutableColumnCallback callback) override
336342
{
337343
callback(*variant_column);

‎src/Columns/ColumnVariant.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,12 @@ class ColumnVariant final : public COWHelper<IColumnHelper<ColumnVariant>, Colum
260260
void forEachMutableSubcolumnRecursively(RecursiveMutableColumnCallback callback) override;
261261
void forEachSubcolumn(ColumnCallback callback) const override;
262262
void forEachSubcolumnRecursively(RecursiveColumnCallback callback) const override;
263+
264+
/// Variant columns pair variant sub-columns with DataTypeVariant's sorted type list.
265+
/// The default convertToFullIfNeeded recurses into sub-columns and strips LowCardinality
266+
/// from variant columns, but cannot update the corresponding DataTypeVariant, creating
267+
/// column/type position mismatches. Override to skip recursion.
268+
[[nodiscard]] IColumn::Ptr convertToFullIfNeeded() const override { return getPtr(); }
263269
bool structureEquals(const IColumn & rhs) const override;
264270
ColumnPtr compress(bool force_compression) const override;
265271
double getRatioOfDefaultRows(double sample_ratio) const override;

‎src/Functions/concatWithSeparator.cpp‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,12 @@ class ConcatWithSeparatorImpl : public IFunction
132132
}
133133
else
134134
{
135-
/// A non-String/non-FixedString-type argument: use the default serialization to convert it to String
136-
auto full_column = column->convertToFullIfNeeded();
135+
/// A non-String/non-FixedString-type argument: use the default serialization to convert it to String.
136+
/// Only strip top-level wrappers (Const, Sparse, LowCardinality) without recursing into subcolumns.
137+
/// Using the recursive convertToFullIfNeeded would strip LowCardinality from inside
138+
/// compound types like Variant while the type is not updated, creating a type/column mismatch.
139+
auto full_column
140+
= column->convertToFullColumnIfConst()->convertToFullColumnIfSparse()->convertToFullColumnIfLowCardinality();
137141

138142
chassert(full_column->size() == input_rows_count);
139143

‎src/Functions/format.cpp‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,12 @@ class FormatFunction : public IFunction
100100
}
101101
else
102102
{
103-
/// A non-String/non-FixedString-type argument: use the default serialization to convert it to String
104-
auto full_column = column->convertToFullIfNeeded();
103+
/// A non-String/non-FixedString-type argument: use the default serialization to convert it to String.
104+
/// Only strip top-level wrappers (Const, Sparse, LowCardinality) without recursing into subcolumns.
105+
/// Using the recursive convertToFullIfNeeded would strip LowCardinality from inside
106+
/// compound types like Variant while the type is not updated, creating a type/column mismatch.
107+
auto full_column
108+
= column->convertToFullColumnIfConst()->convertToFullColumnIfSparse()->convertToFullColumnIfLowCardinality();
105109
auto serialization = arguments[i].type->getDefaultSerialization();
106110
auto converted_col_str = ColumnString::create();
107111
ColumnStringHelpers::WriteHelper<ColumnString> write_helper(*converted_col_str, column->size());

‎src/Interpreters/Set.cpp‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,10 @@ DataTypes Set::getElementTypes(DataTypes types, bool transform_null_in)
116116
{
117117
for (auto & type : types)
118118
{
119-
if (const auto * low_cardinality_type = typeid_cast<const DataTypeLowCardinality *>(type.get()))
120-
type = low_cardinality_type->getDictionaryType();
119+
/// Strip LowCardinality recursively to match what setHeader/insertFromColumns do:
120+
/// insertFromColumns calls convertToFullIfNeeded which recursively strips LC from
121+
/// compound types like Tuple(LowCardinality(T), ...).
122+
type = recursiveRemoveLowCardinality(type);
121123

122124
if (!transform_null_in)
123125
type = removeNullable(type);
@@ -155,10 +157,15 @@ void Set::setHeader(const ColumnsWithTypeAndName & header)
155157
if (const auto * low_cardinality_type = typeid_cast<const DataTypeLowCardinality *>(data_types.back().get()))
156158
{
157159
data_types.back() = low_cardinality_type->getDictionaryType();
158-
set_elements_types.back() = low_cardinality_type->getDictionaryType();
159160
materialized_columns.emplace_back(key_columns.back()->convertToFullColumnIfLowCardinality());
160161
key_columns.back() = materialized_columns.back().get();
161162
}
163+
164+
/// Strip LowCardinality recursively from set_elements_types so they match what
165+
/// convertToFullIfNeeded (which is recursive) does to columns in insertFromColumns.
166+
/// Without this, compound types like Tuple(LowCardinality(T), ...) keep LowCardinality
167+
/// in the type while the column has it stripped, causing type/column mismatches later.
168+
set_elements_types.back() = recursiveRemoveLowCardinality(set_elements_types.back());
162169
}
163170

164171
/// We will insert to the Set only keys, where all components are not NULL.

‎tests/queries/0_stateless/03988_concat_with_separator_format_variant_low_cardinality.reference‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
{0:(0,1),(4):5},x,y
2+
{1:(1,1),(4):5},x,y
3+
{2:(2,1),(4):5},x,y
4+
[(0,2),3],x,y
5+
[(1,2),3],x,y
6+
[(2,2),3],x,y
7+
{0:(0,1),(4):5} x y
8+
{1:(1,1),(4):5} x y
9+
{2:(2,1),(4):5} x y
10+
[(0,2),3] x y
11+
[(1,2),3] x y
12+
[(2,2),3] x y

‎tests/queries/0_stateless/03988_concat_with_separator_format_variant_low_cardinality.sql‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
-- concatWithSeparator and format had the same bug as concat (fixed in #97654):
2+
-- convertToFullIfNeeded recursively stripped LowCardinality from inside a Variant column
3+
-- while the type was not updated, creating a type/column mismatch in serialization.
4+
5+
SET allow_suspicious_low_cardinality_types = 1;
6+
7+
SELECT concatWithSeparator(',', map(number, tuple(number, toLowCardinality(toUInt128(1))), tuple(4), 5), 'x', 'y') FROM numbers(3);
8+
SELECT concatWithSeparator(',', [(number, 2), toLowCardinality(3)], 'x', 'y') FROM numbers(3);
9+
10+
SELECT format('{} {} {}', map(number, tuple(number, toLowCardinality(toUInt128(1))), tuple(4), 5), 'x', 'y') FROM numbers(3);
11+
SELECT format('{} {} {}', [(number, 2), toLowCardinality(3)], 'x', 'y') FROM numbers(3);

‎tests/queries/0_stateless/03989_set_low_cardinality_in_tuple.reference‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
hello 1
2+
world 42
3+
simple
4+
nullable 5
5+
kept 1
6+
multi 10

‎tests/queries/0_stateless/03989_set_low_cardinality_in_tuple.sql‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
-- Regression test: LowCardinality inside Tuple in IN subquery caused LOGICAL_ERROR
2+
-- because Set::setHeader only stripped top-level LowCardinality from set_elements_types
3+
-- while convertToFullIfNeeded (recursive since #97493) stripped it from inner columns,
4+
-- creating a column/type mismatch in KeyCondition::tryPrepareSetColumnsForIndex.
5+
SET allow_suspicious_low_cardinality_types = 1;
6+
7+
-- Exact reproducer from AST fuzzer CI report
8+
DROP TABLE IF EXISTS table2__fuzz_36;
9+
DROP TABLE IF EXISTS table1__fuzz_47;
10+
CREATE TABLE table2__fuzz_36 (id1 Nullable(Date), id2 LowCardinality(Int8)) ENGINE = Memory AS SELECT 1, 1;
11+
CREATE TABLE table1__fuzz_47 (id1 Nullable(Int8), id2 Decimal(76, 58)) ENGINE = MergeTree ORDER BY id1 SETTINGS allow_nullable_key = 1 AS SELECT 1, 1;
12+
SELECT * FROM table1__fuzz_47 WHERE (id1, id2) IN (SELECT tuple(id2, id1) FROM table2__fuzz_36); -- { serverError ILLEGAL_COLUMN }
13+
DROP TABLE table2__fuzz_36;
14+
DROP TABLE table1__fuzz_47;
15+
16+
-- LowCardinality(String) inside tuple from subquery
17+
DROP TABLE IF EXISTS t1;
18+
DROP TABLE IF EXISTS t2;
19+
CREATE TABLE t2 (a LowCardinality(String), b Int32) ENGINE = Memory AS SELECT 'hello', 1;
20+
CREATE TABLE t1 (a String, b Int32) ENGINE = MergeTree ORDER BY a AS SELECT 'hello', 1;
21+
SELECT * FROM t1 WHERE (a, b) IN (SELECT tuple(a, b) FROM t2);
22+
DROP TABLE t1;
23+
DROP TABLE t2;
24+
25+
-- Multiple LowCardinality columns inside tuple, composite key
26+
DROP TABLE IF EXISTS t1;
27+
DROP TABLE IF EXISTS t2;
28+
CREATE TABLE t2 (a LowCardinality(String), b LowCardinality(Int32)) ENGINE = Memory AS SELECT 'world', 42;
29+
CREATE TABLE t1 (a String, b Int32) ENGINE = MergeTree ORDER BY (a, b) AS SELECT 'world', 42;
30+
SELECT * FROM t1 WHERE (a, b) IN (SELECT tuple(a, b) FROM t2);
31+
DROP TABLE t1;
32+
DROP TABLE t2;
33+
34+
-- Simple LowCardinality column in IN subquery (non-tuple case)
35+
DROP TABLE IF EXISTS t1;
36+
DROP TABLE IF EXISTS t2;
37+
CREATE TABLE t2 (a LowCardinality(String)) ENGINE = Memory AS SELECT 'simple';
38+
CREATE TABLE t1 (a String) ENGINE = MergeTree ORDER BY a AS SELECT 'simple';
39+
SELECT * FROM t1 WHERE a IN (SELECT a FROM t2);
40+
DROP TABLE t1;
41+
DROP TABLE t2;
42+
43+
-- LowCardinality(Nullable) inside tuple
44+
DROP TABLE IF EXISTS t1;
45+
DROP TABLE IF EXISTS t2;
46+
CREATE TABLE t2 (a LowCardinality(Nullable(String)), b Int32) ENGINE = Memory AS SELECT 'nullable', 5;
47+
CREATE TABLE t1 (a Nullable(String), b Int32) ENGINE = MergeTree ORDER BY b AS SELECT 'nullable', 5;
48+
SELECT * FROM t1 WHERE (a, b) IN (SELECT tuple(a, b) FROM t2);
49+
DROP TABLE t1;
50+
DROP TABLE t2;
51+
52+
-- NOT IN with LowCardinality in tuple
53+
DROP TABLE IF EXISTS t1;
54+
DROP TABLE IF EXISTS t2;
55+
CREATE TABLE t2 (a LowCardinality(String), b Int32) ENGINE = Memory AS SELECT 'excluded', 99;
56+
CREATE TABLE t1 (a String, b Int32) ENGINE = MergeTree ORDER BY a AS SELECT 'kept', 1;
57+
SELECT * FROM t1 WHERE (a, b) NOT IN (SELECT tuple(a, b) FROM t2);
58+
DROP TABLE t1;
59+
DROP TABLE t2;
60+
61+
-- Non-tuple subquery with multiple LowCardinality columns
62+
DROP TABLE IF EXISTS t1;
63+
DROP TABLE IF EXISTS t2;
64+
CREATE TABLE t2 (a LowCardinality(String), b Int32) ENGINE = Memory AS SELECT 'multi', 10;
65+
CREATE TABLE t1 (a String, b Int32) ENGINE = MergeTree ORDER BY a AS SELECT 'multi', 10;
66+
SELECT * FROM t1 WHERE (a, b) IN (SELECT a, b FROM t2);
67+
DROP TABLE t1;
68+
DROP TABLE t2;

‎tests/queries/0_stateless/03990_set_dynamic_low_cardinality_join_runtime_filter.reference‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
102

0 commit comments

Comments
 (0)