iframe-proxy

groeneai · 2026-06-12T22:14:18Z

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 a Bad cast from type DB::ColumnString to DB::ColumnLowCardinality logical error (server crash in debug builds) when a constant of a Variant type that has a LowCardinality member is used as a set element for an IN predicate over a MergeTree skip index.

Description

Found by the server-side AST fuzzer on master (Stress test (experimental, serverfuzz, arm_ubsan), commit c20a6032517588c99638787af78a476c8c40b3d0, 2026-06-12; first seen on amd_tsan 2026-05-24). STID 4054-600e. Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=c20a6032517588c99638787af78a476c8c40b3d0&name_0=MasterCI&name_1=Stress%20test%20%28experimental%2C%20serverfuzz%2C%20arm_ubsan%29

Minimal reproducer (debug build, aborts the server):

SET allow_suspicious_low_cardinality_types = 1, allow_experimental_variant_type = 1, allow_suspicious_variant_types = 1;
CREATE TABLE t (key Int, s String, INDEX i sipHash64(s) TYPE minmax GRANULARITY 1) ENGINE = MergeTree ORDER BY key;
INSERT INTO t SELECT number, toString(number) FROM numbers(16);
SELECT count() FROM t WHERE s IN (SELECT _CAST('5', 'Variant(Int256, LowCardinality(String))'));

Root cause: the set element column is materialized with IColumn::convertToFullIfNeeded. For a Const(Variant(..., LowCardinality(String))) the default implementation first strips the outer Const, then iterates the unwrapped ColumnVariant's members via forEachSubcolumn and converts each to a full column, stripping the inner LowCardinality. That bypasses ColumnVariant::convertToFullIfNeeded, which is overridden to a no-op (issue #97854) precisely to keep the column consistent with DataTypeVariant (whose members are sorted by name and cannot follow recursive stripping). The set element type, processed by recursiveRemoveLowCardinality, keeps the inner LowCardinality. The resulting column/type mismatch makes the Variant -> String cast wrapper expect a ColumnLowCardinality member but receive a ColumnString, aborting in KeyCondition::applyDeterministicDagToColumn (via tryPrepareSetIndexForIn -> tryPrepareSetColumnsForIndex).

This is the prepared-set sibling of the path fixed in #107111 (which handled the equals/tuple-literal path in KeyCondition but could not fix an input that is already column/type-asymmetric on arrival). It also completes the fix for issue #97854, which added the ColumnVariant/ColumnDynamic override but only protected a top-level ColumnVariant, not one reached after unwrapping an outer Const (or Tuple/Array/etc.).

Fix: in the default IColumn::convertToFullIfNeeded, when an outer wrapper (Const/Replicated/Sparse/LowCardinality) is stripped, delegate to the unwrapped column's own convertToFullIfNeeded so its override is honored, instead of recursing into its subcolumns here. For columns without an override the delegate's default implementation performs the same subcolumn recursion, so behavior is unchanged.

Regression test 06673_set_index_variant_low_cardinality added: crashes the server on the pre-fix binary, passes with the fix. Verified the existing Variant/LowCardinality/set-index test families (including 04329, 03991, 03987, 04141) and a broader sweep of convertToFullIfNeeded consumers still pass with no behavioral change.

groeneai · 2026-06-12T22:14:45Z

Pre-PR validation gate

#	Question	Answer
a	Deterministic repro?	Yes. `SELECT count() FROM t WHERE s IN (SELECT _CAST('5', 'Variant(Int256, LowCardinality(String))'))` over a MergeTree table with a `sipHash64(s)` minmax skip index aborts the server every time on a pre-fix debug build (settings: `allow_suspicious_low_cardinality_types`, `allow_experimental_variant_type`, `allow_suspicious_variant_types`).
b	Root cause explained?	Yes. `IColumn::convertToFullIfNeeded` on the `Const(Variant(Int256, LowCardinality(String)))` set element strips the outer `Const`, then recurses into the unwrapped `ColumnVariant`'s members via `forEachSubcolumn` and converts the `LowCardinality(String)` member to full `String` — bypassing `ColumnVariant::convertToFullIfNeeded` (a no-op override from #97854). The set element type keeps the inner `LowCardinality` (`recursiveRemoveLowCardinality` has no Variant branch, by design). The column becomes `Variant(Int256, String)` while the type stays `Variant(Int256, LowCardinality(String))`; the `Variant -> String` cast wrapper then `typeid_cast`s a `ColumnString` to `ColumnLowCardinality` and aborts in `KeyCondition::applyDeterministicDagToColumn`. Confirmed with `LOG_FATAL` instrumentation showing the exact column/type at each stage.
c	Fix matches root cause?	Yes. The fix makes the default `convertToFullIfNeeded` delegate to the unwrapped column's own `convertToFullIfNeeded` when an outer wrapper was stripped, so the `ColumnVariant`/`ColumnDynamic` no-op override is honored instead of being bypassed by the manual subcolumn recursion. It addresses the asymmetry at its source (the column materialization), not the crash site.
d	Test intent preserved / new tests added?	New regression test `06673_set_index_variant_low_cardinality` added (subquery-`IN` prepared-set path with Variant-LC element). No existing test weakened.
e	Both directions demonstrated?	Yes. Pre-fix binary (Build ID `89b935109`): the test query aborts with `Bad cast from type DB::ColumnString to DB::ColumnLowCardinality`, server dies. Post-fix binary (Build ID `23bc8357`): clean `CANNOT_CONVERT_TYPE` for the String-vs-Variant cases, correct counts (`0, 0, 2`) for the valid cases, server stays alive. The regression test passes post-fix and crashes pre-fix.
f	Fix is general, not a narrow patch?	Yes. The fix is in the shared `IColumn::convertToFullIfNeeded`, so it covers every outer wrapper (`Const`/`Replicated`/`Sparse`/`LowCardinality`) over any self-contained column with an override (`ColumnVariant`, `ColumnDynamic`), not just the one fuzzed crash site. It traces the bad state to its origin (column/type asymmetry born during set element materialization) rather than guarding the crash site. For columns without an override the delegate's default implementation does the identical subcolumn recursion, so behavior is unchanged; verified against 60+ `convertToFullIfNeeded`-consuming tests (LowCardinality/Variant/Dynamic/set/IN/const/tuple/nullable) with zero crashes and exact-reference matches on deterministic ones.

Session id: cron:clickhouse-worker-slot-1:20260612-205900

groeneai · 2026-06-12T22:15:21Z

cc @Algunenano @Avogar — could you review this? It completes the ColumnVariant/ColumnDynamic convertToFullIfNeeded fix from #97854: the no-op override was bypassed when the Variant is reached through the default IColumn::convertToFullIfNeeded recursion after unwrapping an outer Const (e.g. a Variant(..., LowCardinality(String)) constant used as an IN-set element over a skip index), stripping the inner LowCardinality and causing a Bad cast in KeyCondition. Fix delegates to the unwrapped column's own convertToFullIfNeeded so the override is honored.

clickhouse-gh · 2026-06-12T22:17:26Z

Workflow [PR], commit [333de7f]

Summary: ✅

Performance Comparison: Performance dashboard

AI Review

Summary

This PR fixes IColumn::convertToFullIfNeeded so an outer ColumnConst, ColumnReplicated, ColumnSparse, or ColumnLowCardinality delegates to the unwrapped column's own convertToFullIfNeeded, preserving the ColumnVariant and ColumnDynamic invariants while keeping container columns such as ColumnArray on the normal subcolumn-recursion path. The current code addresses the prior review thread and the regression test covers both the Variant/LowCardinality mismatch and the ColumnArray recursion case; I did not find unresolved issues that need inline comments.

Final Verdict

Status: ✅ Approve

groeneai · 2026-06-12T23:38:29Z

Re-pushed 41bc5b5 to fix the Style check (test_numbers_check) failure on the prior commit: the regression test had been numbered 06673, which the gap check rejected (Gap (4336, 6673) > 100). Renamed it to the next free number, 04337_set_index_variant_low_cardinality, and rebased onto current master (absorbs #107364, which had poisoned arm_tidy on the old base).

The C++ fix in src/Columns/IColumn.h is unchanged. Re-validated against the post-fix binary: the IN-set crash query no longer aborts (clean CANNOT_CONVERT_TYPE), the valid-Variant cases return correct counts, and 04337 matches its reference (0/0/2) with the server alive throughout.

…ant with LowCardinality member A constant of a Variant type that has a LowCardinality member, used as a prepared-set element for an IN predicate over a MergeTree skip index, crashed the server with "Bad cast from type DB::ColumnString to DB::ColumnLowCardinality" in KeyCondition::applyDeterministicDagToColumn (via tryPrepareSetIndexForIn -> tryPrepareSetColumnsForIndex). The set element column is materialized with IColumn::convertToFullIfNeeded. For a Const(Variant(..., LowCardinality(String))) it first strips the outer Const, then iterates the unwrapped ColumnVariant's members via forEachSubcolumn and converts each to full, stripping the inner LowCardinality. That bypasses ColumnVariant::convertToFullIfNeeded, which is overridden to a no-op precisely to keep the column consistent with DataTypeVariant (whose members are sorted by name and cannot follow recursive stripping). The set element type keeps the inner LowCardinality, so the Variant->String cast wrapper expected a ColumnLowCardinality member but received a ColumnString and aborted. Fix: in the default IColumn::convertToFullIfNeeded, when an outer wrapper is stripped, delegate to the unwrapped column's own convertToFullIfNeeded so its override is honored, instead of recursing into its subcolumns here. For columns without an override the delegate's default implementation performs the same subcolumn recursion, so behavior is unchanged. This completes the fix from issue ClickHouse#97854, which only protected a top-level ColumnVariant. The delegation is gated on the column actually being an outer wrapper (isConst/isReplicated/isSparse/lowCardinality), not on a pointer difference after the strip chain. ColumnArray::convertToFullColumnIfConst always allocates a fresh ColumnArray even for a plain array, so a pointer-difference guard made any array column delegate to an equivalent fresh array forever (stack overflow on array IN-sets via Set::insertFromColumns). Container columns now fall through to the normal subcolumn recursion as before. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-13T00:00:09Z

Pre-PR validation gate (follow-up: ColumnArray recursion fix, commit `4e46276`)

#	Question	Answer
a	Deterministic repro?	Yes. `SELECT [1,2] IN (SELECT [number, number + 1] FROM numbers(3))` segfaults (stack overflow) every time on the pre-fix binary; passes with the fix.
b	Root cause explained?	The guard used `converted.get() != this` to mean "an outer wrapper was stripped". `ColumnArray::convertToFullColumnIfConst` unconditionally returns a freshly created `ColumnArray`, so for any array the pointer always differs, the code delegates to an equivalent fresh array, and recurses forever. Array `IN`-sets reach this via `Set::insertFromColumns -> convertToFullIfNeeded`.
c	Fix matches root cause?	Yes. Delegation is now gated on the column actually being a wrapper: `isConst() \|\| isReplicated() \|\| isSparse() \|\| lowCardinality()`. The four predicates map 1:1 to the four strip calls. `ColumnArray` overrides none, so it falls through to the normal subcolumn recursion (pre-PR behavior). `&& converted.get() != this` is kept for guaranteed termination.
d	Test intent preserved / new tests added?	Preserved. Added three array `IN`-set cases plus a liveness check to the regression test; the original `Variant`/`LowCardinality` assertions are unchanged.
e	Both directions demonstrated?	Yes. Pre-fix: segfault on the array `IN`-set, server dies. Post-fix: returns `1`, server alive, 30/30 in a loop; `clickhouse-test` 10/10 under full randomization.
f	Fix is general, not a narrow patch?	Yes. The fix targets the wrapper-detection logic itself, so it covers all container columns, not just `ColumnArray`. Checked `Tuple`/`Map` inherit the no-op `convertToFullColumnIfConst` (no fresh alloc) and `Nullable` strips only its nested column; none take the delegation branch. Sibling test `04329` and the `variant`/`low_cardinality`/`set_index` families pass with no behavior change.

Session id: cron:clickhouse-worker-slot-3:20260612-224000

groeneai · 2026-06-13T05:13:24Z

CI finished on 4e46276. All red checks are pre-existing failures unrelated to this change (this PR only edits src/Columns/IColumn.h convertToFullIfNeeded plus the 04337 regression test):

BuzzHouse (arm_asan_ubsan): Mutation of Memory table produced incomplete output (STID 2380-3043). Chronic StorageMemory APPLY PATCHES trunk bug (16 master hits / 30d across many unrelated PRs). Fix in flight: Fix arm_tidy: initialize variables in Iceberg::alter (caused by #106102) #106615.
Fast test (arm_darwin): 04336_parallel_blocks_marshalling_low_cardinality_native_format. Known macOS connect-timeout to 127.0.0.2 (the loopback range is not auto-bound on Darwin). Fix in flight: Skip macOS-incompatible distributed tests 04327 and 04336 #107376.
Stress test (arm_tsan): possible deadlock on shutdown. Chronic Hung-check trunk bug (leaked local ThreadPool); retried green on this run.

The convertToFullIfNeeded fix itself is green across all stateless and sanitizer jobs and AI-review approved.

…-lowcardinality-set-index

clickhouse-gh · 2026-06-13T17:28:02Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	84.70%	84.70%	+0.00%
Functions	92.30%	92.40%	+0.10%
Branches	77.30%	77.30%	+0.00%

Changed lines: Changed C/C++ lines covered by tests: 9/9 (100.00%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

groeneai · 2026-06-13T20:21:24Z

CI re-validated on HEAD 333de7f05f47 after the automatic master-merge into the branch: green. 148 checks pass, Finish Workflow and Mergeable Check both SUCCESS, zero test-level failures in CIDB. The single red mark is CH Inc sync (internal ClickHouse-Inc fork-sync bot, always red for external contributors, not a test).

The convertToFullIfNeeded wrapper-type-guard fix (Variant+LC Bad cast STID 4054-600e plus the ColumnArray infinite-recursion bot finding) holds across all stateless + sanitizer flaky-check jobs. Ready for review.

Avogar · 2026-06-17T18:36:11Z

I don't like this solution. And current implementation of convertToFullColumnIfNeeded, so I decided to change it here - #107773. It will also fix attached issue.

groeneai · 2026-06-17T19:15:31Z

nikitamikhaylov added the can be tested Allows running workflows for external contributors label Jun 12, 2026

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jun 12, 2026

clickhouse-gh Bot reviewed Jun 12, 2026

View reviewed changes

Comment thread src/Columns/IColumn.h

groeneai force-pushed the groeneai/fix-convertToFullIfNeeded-variant-lowcardinality-set-index branch from d37269f to 41bc5b5 Compare June 12, 2026 23:38

groeneai force-pushed the groeneai/fix-convertToFullIfNeeded-variant-lowcardinality-set-index branch from 41bc5b5 to 4e46276 Compare June 12, 2026 23:59

Merge branch 'master' into groeneai/fix-convertToFullIfNeeded-variant…

333de7f

…-lowcardinality-set-index

alexey-milovidov mentioned this pull request Jun 13, 2026

Skip macOS-incompatible distributed tests 04327 and 04336 #107376

Merged

Avogar self-assigned this Jun 15, 2026

Avogar mentioned this pull request Jun 17, 2026

Refactor convertToFullIfNeeded to avoid issues with recursive LowCardinality stripping #107773

Open

Avogar closed this Jun 17, 2026

groeneai mentioned this pull request Jul 1, 2026

Logical error: Bad cast from type A to B (STID: 4054-600e) #107598

Open

Sunbelt Computer Software

PL/B Language Development and Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix Bad cast for Const-wrapped Variant with LowCardinality member in set index analysis#107373

Fix Bad cast for Const-wrapped Variant with LowCardinality member in set index analysis#107373
groeneai wants to merge 2 commits into
ClickHouse:masterfrom
groeneai:groeneai/fix-convertToFullIfNeeded-variant-lowcardinality-set-index

groeneai commented Jun 12, 2026

Uh oh!

groeneai commented Jun 12, 2026

Uh oh!

groeneai commented Jun 12, 2026

Uh oh!

clickhouse-gh Bot commented Jun 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

groeneai commented Jun 12, 2026

Uh oh!

groeneai commented Jun 13, 2026

Uh oh!

groeneai commented Jun 13, 2026

Uh oh!

clickhouse-gh Bot commented Jun 13, 2026

Uh oh!

groeneai commented Jun 13, 2026

Uh oh!

Avogar commented Jun 17, 2026

Uh oh!

groeneai commented Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

groeneai commented Jun 12, 2026

Changelog category (leave one):

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

Description

Uh oh!

groeneai commented Jun 12, 2026

Pre-PR validation gate

Uh oh!

groeneai commented Jun 12, 2026

Uh oh!

clickhouse-gh Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Final Verdict

Uh oh!

Uh oh!

groeneai commented Jun 12, 2026

Uh oh!

groeneai commented Jun 13, 2026

Pre-PR validation gate (follow-up: ColumnArray recursion fix, commit 4e46276)

Uh oh!

groeneai commented Jun 13, 2026

Uh oh!

clickhouse-gh Bot commented Jun 13, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Jun 13, 2026

Uh oh!

Avogar commented Jun 17, 2026

Uh oh!

groeneai commented Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

clickhouse-gh Bot commented Jun 12, 2026 •

edited

Loading

Pre-PR validation gate (follow-up: ColumnArray recursion fix, commit `4e46276`)