Fix crash in CrossTab aggregate functions during Variant to Variant merge

[groeneai]

Related: #104183
Related: #104277

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):

Fixed a server crash when the aggregate functions cramersV, cramersVBiasCorrected, contingency or theilsU states from a window context (OVER ()) and from a plain aggregation are combined through chained set operations (EXCEPT, INTERSECT) or nested UNION ALL and then read, for example via the -Merge combinator.

Description

These four aggregate functions have two internal state representations, Aggregation and Window, that are not encoded in the type name. When a Variant collapses two same-named AggregateFunction(f, ...) types that differ only in state representation into a single slot, the source column must be converted to the destination slot's representation before being stored, otherwise a later read interprets the bytes with the wrong layout and crashes.

PR #104277 fixed this for the scalar column -> Variant cast (createColumnToVariantWrapper). The Variant -> Variant cast (createVariantToVariantWrapper) had the same gap: it mapped variant slots purely by name and copied their subcolumns verbatim. Chained set operations (EXCEPT / INTERSECT) and nested UNION ALL build exactly this Variant -> Variant cast, so a subcolumn carrying one representation ended up under a slot expecting the other, and a subsequent finalize / serialize / -Merge read the bytes with the wrong layout and aborted the server.

This PR converts the subcolumn to the destination representation in createVariantToVariantWrapper when an old and a new slot match by name but are not equal (equals() is false), reusing the existing AggregateFunction representation-converting cast, instead of copying it verbatim. A regression test covering all four functions via EXCEPT, INTERSECT and nested UNION ALL is added.

How it was found: AST fuzzer, surfaced as a Stress test (arm_debug) server crash on an unrelated PR (#107899, unrelated carrier).

• STID: 2844-3b6e
• Check: Stress test (arm_debug)
• Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107899&sha=8065b57ecbabf38cd132a67d01c3d2f80b402295&name_0=PR&name_1=Stress%20test%20%28arm_debug%29
• Crash: SIGSEGV in DB::CrossTabCountsState::merge (src/AggregateFunctions/CrossTab.h:64) reached via AggregateFunctionMerge::addBatchSinglePlace.

Triggering query (from the crash log):

SELECT round(cramersVBiasCorrectedMerge(s.`AggregateFunction(cramersVBiasCorrected, UInt8, UInt8)`), 4)
FROM
(
    (SELECT cramersVBiasCorrectedState(toUInt8(number % 10), toUInt8(number % 6)) OVER () AS s FROM numbers_mt(100) LIMIT 1)
    EXCEPT DISTINCT
    (SELECT cramersVBiasCorrectedState(toUInt8(number % 10), toInt128OrDefault(number % 65535)) AS s FROM numbers(100))
    EXCEPT DISTINCT
    (SELECT cramersVBiasCorrectedState(toUInt8(number % 10), toUInt8(number % 65535)) AS s FROM numbers(100))
);

Version info

• Merged into: 26.7.1.477 (included in 26.7 and later)

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. The exact fuzzer query from the crash log SIGSEGVs clickhouse local on the first run (EXIT 139), no randomization needed. Minimal form: (WindowState EXCEPT DISTINCT AggState_Int128) EXCEPT DISTINCT AggState_UInt8 then cramersVBiasCorrectedMerge, or the equivalent nested UNION ALL.
b	Root cause explained?	Yes. cramersV*/contingency/theilsU have two state representations (Aggregation, Window) sharing one type name. A Variant collapses two same-named AggregateFunction(f,...) slots that differ only in representation. createColumnToVariantWrapper (fixed in #104277) converts the scalar→Variant case, but the Variant→Variant cast (createVariantToVariantWrapper) mapped slots by name and copied subcolumns verbatim. Chained EXCEPT/INTERSECT and nested UNION ALL build a Variant→Variant cast, so a Window-representation subcolumn landed under a slot expecting Aggregation; a later read (getSubcolumn + -Merge/serialize) interpreted the bytes with the wrong layout → SIGSEGV in CrossTabCountsState::merge (begin()/isZero).
c	Fix matches root cause?	Yes. In createVariantToVariantWrapper, when an old and a new slot match by name but equals() is false, the subcolumn is converted with prepareUnpackDictionaries(old, new) (reusing the existing AggregateFunction representation-converting cast) instead of being copied verbatim. This directly closes the same gap #104277 closed for the scalar path.
d	Test intent preserved / new tests added?	New regression test 04498_crosstab_variant_to_variant_merge added, covering all four functions (cramersV, cramersVBiasCorrected, contingency, theilsU) via EXCEPT DISTINCT chains and nested UNION ALL. No existing test weakened; 04204_crosstab_variant_merge (the #104277 test) still passes.
e	Both directions demonstrated?	Yes. Same source tree: pre-fix binary SIGSEGVs on the repro (EXIT 139); post-fix binary returns the correct value (EXIT 0). Verified on both the PR-branch build and a fresh master-based build.
f	Fix is general across code paths?	Yes. The sibling scalar path (createColumnToVariantWrapper) was already handled by #104277; this PR handles the remaining createVariantToVariantWrapper path, which is also reused by the Variant→Dynamic conversion (verified no regression). The fix targets the representation mismatch at its source (the Variant→Variant slot copy), not the crash site.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	Yes. Verified for all four CrossTab functions including theilsU (which has a distinct WindowData layout), across EXCEPT DISTINCT, INTERSECT DISTINCT, and nested UNION ALL. The guard is representation-generic (!old->equals(*new) + prepareUnpackDictionaries), not specific to one function or arg type. Non-AggregateFunction same-named slots satisfy equals() so they keep the verbatim fast path (no behavior change; confirmed with mixed Int/String/Array Variant + Dynamic tests).
h	Backward compatible?	Yes. No setting, no on-disk/wire/metadata format change, no new validation. Pure in-memory cast-wrapper correctness fix. SettingsChangesHistory.cpp not applicable.
i	Invariants and contracts preserved?	Yes. ColumnVariant invariant preserved: the converting wrapper is a per-row state conversion that preserves row count and order, and the shared local-discriminators/offsets columns are reused unchanged, so discriminator↔row alignment holds. The Variant-extension contract (new Variant is an extension of the old) is unchanged; only same-named-but-different-representation slots now get a proper conversion instead of a verbatim copy.

Session id: cron:clickhouse-worker-slot-2:20260702-151000

[groeneai]

cc @Avogar @nihalzp — could you review this? It extends the fix from #104277 to the Variant -> Variant cast path (createVariantToVariantWrapper): chained EXCEPT/INTERSECT and nested UNION ALL build a Variant->Variant cast that copied same-named AggregateFunction subcolumns verbatim, so a window-representation cramersV*/contingency/theilsU state landed under an aggregation-representation slot and crashed on read. The fix converts the subcolumn to the destination representation when the slots match by name but differ (equals() false).

[clickhouse-gh]

Workflow [PR], commit [6419d2b]

Summary: ✅

• Performance Comparison: Performance dashboard

---

AI Review

Summary

This PR closes the live Variant -> Variant, nested Variant, and new-write Dynamic conversion paths for hidden AggregateFunction state-representation mismatches. I found one remaining backward-compatibility gap: Dynamic rows that were already written by buggy builds are still interpreted by name on read, so upgrading does not actually fix affected persisted data.

Findings

❌ Blockers

• [src/Functions/FunctionsConversion.cpp:2110-2147] Canonicalizing only new Dynamic writes is not enough for a Bug Fix, because old malformed Dynamic rows remain unreadable after upgrade. Before this patch, CAST(<window state> AS Dynamic) could persist a window-representation state under the canonical AggregateFunction(...) name. SerializationDynamic stores only that name for regular variants in V1/V2 (src/DataTypes/Serializations/SerializationDynamic.cpp:219-224, 391-395), while dynamicElement / DataTypeDynamic::getDynamicSubcolumnData still resolve the name back to the canonical aggregation type on read (src/DataTypes/DataTypeDynamic.cpp:894-922). Suggested fix: add a read-side compatibility or explicit-rejection path for same-name Dynamic subcolumns whose stored type is not equals() to the canonical one; if old V1/V2 rows are no longer distinguishable, document that limitation explicitly instead of treating the change as fully backward compatible.

Final Verdict

• Status: ⚠️ Request changes
• Minimum required actions: handle or explicitly account for already-persisted malformed Dynamic rows; the current patch only protects new writes.

[groeneai]

Pre-PR validation gate for 6419d2b (Dynamic carrier + nested types)

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. SELECT cramersVBiasCorrectedMerge(dynamicElement(CAST(w AS Dynamic), 'AggregateFunction(cramersVBiasCorrected, UInt8, UInt8)')) FROM (SELECT cramersVBiasCorrectedState(...) OVER () AS w ...) SIGSEGVs every time. A single value is enough (no combine of two Dynamic values needed). Same stack as STID 2844-3b6e: CrossTabCountsState::merge (CrossTab.h:64) -> HashTable::begin -> HashMapCell::isZero.
b	Root cause explained?	A Dynamic identifies stored types by name and re-resolves the type from that name. AggregateFunction has a hidden state representation (Aggregation vs Window) not encoded in the name. CAST(<window state> AS Dynamic) stored the Window bytes under the name without converting to the canonical (name-resolved, Aggregation) representation; a later read (dynamicElement / -Merge) interpreted the Window bytes with the Aggregation layout -> reads a garbage HashMap -> SIGSEGV. Also present for a nested AggregateFunction inside a plain Variant slot, because the original #104277 fix only converted a top-level AggregateFunction.
c	Fix matches root cause?	Yes. Fix is at the cast source, not the symptom. createVariantToDynamicWrapper builds the Dynamic's storage Variant from the canonical (name-resolved) representation of each source type, so the existing Variant to Variant cast converts any mismatched subcolumn. createColumnToVariantWrapper now converts whenever the by-name-matched source differs from the destination slot.
d	Test intent preserved / new tests added?	Yes. 04498 extended (not weakened): Dynamic single-window for all 4 functions, Dynamic mixed window+aggregation (exercises the combine path), Dynamic nested Array(window), and pure-Variant nested Array(window). #104277's 04204 unchanged and still passes.
e	Both directions demonstrated?	Yes, on Build-ID-verified binaries. Without the source change (Build ID 13016e99), the extended 04498 prints the 5 pre-existing lines then SIGSEGVs at the first new case. With it (Build ID 492e19ed), all lines match the reference.
f	Fix is general across code paths?	Yes. Traced upstream to the cast rather than guarding the combine paths (insertFrom/insertManyFrom): once a Dynamic can only hold canonical representations, those paths are safe by construction. Covered both name-keyed containers (Dynamic and Variant) and the nested case the bot review did not mention.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	Yes. Verified cramersV, cramersVBiasCorrected, contingency, and theilsU (theilsU did not crash because its window layout keeps the CrossTabCountsState prefix, but it is still converted for correctness). Verified plain slot and nested Array slot; conversion recurses into Array/Map/Tuple via prepareUnpackDictionaries. Values match the pure-aggregation and window baselines (0.3506 / 0.4557 / 0.7137 / 0.3051; mixed = 0.4082).
h	Backward compatible?	Yes. No setting, format, or on-disk change. The change only makes a cast produce the canonical representation that every reader already assumed; it fixes a crash and does not alter results of previously-working queries (04204 unchanged).
i	Invariants and contracts preserved?	Yes. Restores the Dynamic/Variant invariant that a name-keyed slot stores the name-resolved representation. The added Variant to Variant conversion reuses the proven AggregateFunction representation-converting cast (canMergeStateFromDifferentVariant / mergeStateFromDifferentVariant), and the identity path is kept when a type already equals its canonical form.

Session id: cron:clickhouse-worker-slot-10:20260702-175800

[clickhouse-gh]

This only fixes newly-created Dynamic values. It does not make already-persisted bad Dynamic rows readable after upgrade.

Before this patch, CAST(<window state> AS Dynamic) stored the window representation under the canonical AggregateFunction(...) name. That can be written to disk: SerializationDynamic stores only the name for regular variants in V1/V2 (src/DataTypes/Serializations/SerializationDynamic.cpp:219-224, 391-395), and dynamicElement / DataTypeDynamic::getDynamicSubcolumnData still resolve reads by name back to the canonical aggregation type (src/DataTypes/DataTypeDynamic.cpp:894-922). So an upgraded server still ends up pairing those persisted bytes with the wrong state layout.

I think this needs a read-side compatibility or explicit-rejection path for same-name Dynamic slots whose stored type is not equals() to the canonical one. If V1/V2 rows are no longer distinguishable, that limitation needs to be called out explicitly, because as written the fix only protects new writes.

[groeneai]

The persisted bytes are representation-independent, so there are no bad on-disk rows to make readable. The mismatch is only in the live in-memory column layout, never in what reaches disk.

Verified on two Build-ID-verified debug binaries (the fix is one source file, so pre-fix 97643a87 = parent's FunctionsConversion.cpp, post-fix 193be927 = HEAD):

• Pre-fix writes CAST(<window state> AS Dynamic) into an on-disk MergeTree (Wide part): no crash.
• Pre-fix reads it back: cramersVBiasCorrectedMerge returns the correct value (0.3506), dynamicType/toString resolve fine.
• Upgrade sim: the post-fix binary reads the pre-fix-persisted data and returns the same 0.3506.
• The on-disk d.AggregateFunction(cramersVBiasCorrected, UInt8, UInt8).bin is byte-identical (same md5, 431 bytes) whether the state was produced by the window (OVER ()) or the plain aggregation representation. d.variant_discr.bin and d.dynamic_structure.bin match too.

Why: the on-disk format is the same for both representations by design. IAggregateFunction.h requires "The serialization format must be same for all variants of the same aggregate function", and CrossTabPhiSquaredWindowData::serialize explicitly keeps the same count/count_a/count_b/count_ab layout as CrossTabAggregateData. The write path (SerializationAggregateFunction::serializeBinaryBulk) serializes through the type-resolved (canonical) function, and deserialize re-validates invariants. A Dynamic slot on disk therefore only ever holds canonical count-map bytes, so a read after upgrade resolves the name to the canonical type and reads matching bytes.

So no read-side compatibility or rejection path is needed and no SettingsChangesHistory change applies. The name-keyed read paths cited (dynamicElement, getDynamicSubcolumnData) are safe because the bytes they resolve are already canonical. Test 04498 already covers dynamicElement and -Merge on window states for all four functions plus the nested Array case.

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.50%	85.60%	+0.10%
Functions	92.70%	92.70%	+0.00%
Branches	77.70%	77.70%	+0.00%

Changed lines: Changed C/C++ lines covered: 43/43 (100.00%) · Uncovered code

Full report · Diff report

[groeneai]

CI finish ledger — 6419d2b

CI is complete on this head. No failed tests on the current head; all aggregators green (Finish Workflow, Mergeable Check pass). Only the private CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
(no failed tests)	-	-
CH Inc sync	-	CH Inc sync (private, not actionable)

The AST fuzzer (targeted) that originally found the CrossTab Variant crash passes on this head. The bot's backward-compatibility concern (persisted Dynamic rows) was empirically disproven: the on-disk aggregate-state bytes are representation-independent (md5-identical for window vs plain aggregation states), pre-fix-persisted rows read back correctly on the fixed binary, so no read-side compat path is needed.

Session id: cron:our-pr-ci-monitor:20260703-000000