Fix crash in -Tuple combinator with RESPECT NULLS producing a state

[groeneai]

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 crash when an aggregate function with the -Tuple combinator was used with RESPECT NULLS and produced an intermediate state (-State, distributed aggregation, WITH ROLLUP/WITH CUBE). The combined function was named after the wrong state variant, so its serialized AggregateFunction type re-resolved to an incompatible in-memory layout on a round-trip.

Description

The -Tuple combinator resolves its nested functions with the requested NullsAction applied — RESPECT NULLS selects the window-variant any_respect_nulls, whose state layout differs from the aggregation-variant any — but the combined AggregateFunctionTuple was built with the pre-action base name (anyTuple instead of any_respect_nullsTuple). The built state used the window layout while its serialized -State type name claimed the aggregation layout, so on a type-name round-trip (distributed shard -> initiator, storage, plan (de)serialization) the state was reconstructed with the wrong implementation and read as a different SingleValueData, dereferencing garbage and crashing in SerializationTuple::serializeBinary.

The fix names the combined function after its resolved nested function (which reflects the action and state variant), matching what the single-nested combinator path and AggregateFunctionTuple::getNormalizedStateType() already do. Found by the AST fuzzer:
https://s3.amazonaws.com/clickhouse-test-reports/json.html (AST fuzzer amd_debug, commit c6b6f90, STID 3227-3f13).

Version info

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

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. 2-shard oracle crashes every run; single-node toTypeName(anyTupleState(tuple(1::UInt32)) RESPECT NULLS) deterministically shows the wrong name (anyTuple) on the unfixed build.
b	Root cause explained?	The -Tuple combinator resolves nested functions with the NullsAction applied (RESPECT NULLS -> window-variant any_respect_nulls, Field-based, different state layout than aggregation-variant any), but named the combined function with the pre-action base name. The built state used the window layout while the serialized -State type name claimed the aggregation layout; a type-name round-trip reconstructed the wrong implementation, read as a different SingleValueData, and dereferenced garbage -> SIGSEGV in SerializationTuple::serializeBinary.
c	Fix matches root cause?	Yes. Names the combined function after its resolved nested function, so the serialized name identifies the correct layout. Not a deref guard.
d	Test intent preserved / new tests added?	New stateless test 04607 asserts the corrected -State name and a distributed round-trip that survives. No existing test weakened.
e	Both directions demonstrated?	Yes. Unfixed build: server crashes (exact fuzzer stack). Fixed build: no crash, the fuzzer query returns a clean NOT_IMPLEMENTED (a pre-existing SerializationVariant limitation). 50x runs: 0 crashes.
f	Fix general, not narrow?	Yes. Corrects the wrapper-name derivation for every -Tuple + NullsAction case, not just any. Verified the name matrix (anyTuple/anyTupleStateDistinct/anyRespectNullsTuple consistent; sumTuple/corrTuple/quantilesTuple unchanged).
g	Generalizes across inputs?	Yes. Tested across combinator chains (State, StateDistinct, OrNull) and RESPECT/IGNORE NULLS; the single-nested combinator path was already correct and stays correct.
h	Backward compatible?	The serialized -State name for -Tuple + RESPECT NULLS changes from anyTuple to any_respect_nullsTuple. The old name was ambiguous and already produced crashing/corrupt states for the window layout, so correct names are forward-safe. No wire/format version change; no setting change.
i	Invariants preserved?	Yes. Restores the invariant that an AggregateFunction type name uniquely determines its state layout. No concurrency/format-structure changes.

Session id: cron:clickhouse-author-slot-0:20260718-070200

[groeneai]

Internal second-model review (adjudicated) — click to expand

An independent model reviewed this diff before it was opened. Findings and dispositions:

❌ Blockers

• Argument evaluation order (UB) — AGREE. The initial fix read nested_functions.front()->getName() and std::move(nested_functions) in the same call; argument evaluation order is unspecified, so the vector could be moved-from before front() was read. Fixed by reading the name into a local String before the move; rebuilt and re-verified (both directions + name matrix).

💡 Nits

• Verbose test comment — AGREE. The test's opening comment restated the full root-cause narrative; trimmed to two lines stating only the tested invariant (motivation lives in the commit/PR). Output unchanged.

(An earlier "empty diff" finding was a process artifact — the review ran before the commit; re-run on the committed diff.)

[groeneai]

cc @azat @alexey-milovidov — could you review this? It fixes a crash where the -Tuple combinator with RESPECT NULLS built a window-variant state (any_respect_nulls) but named the combined function with the pre-action base name (anyTuple), so the serialized -State type re-resolved to an incompatible layout on a round-trip. The fix names the combined function after its resolved nested function, matching the single-nested combinator path.

[clickhouse-gh]

Workflow [PR], commit [8600427]

Summary: ✅

• Performance Comparison: Performance dashboard

---

AI Review

Summary

This PR fixes the -Tuple combinator so aggregate states are named from the action-adjusted base aggregate instead of an arbitrary resolved element. That closes the original RESPECT NULLS layout mismatch and the follow-up multi-element placeholder case (countTuple((NULL, x))), and the current code and tests are consistent with that contract. I did not find any remaining correctness or compatibility issues in the current diff.

Final Verdict

• Status: ✅ Approve

[groeneai]

The AST fuzzer surfaced this crash again from a second query shape (STID 4514-3f61, AST fuzzer (amd_debug, targeted) on the PR 110886 run): first_valueOrNullDistinctOrDefaultTupleOrNull(tuple(toString(NULL))) over a distributed table with GROUP BY ALL WITH TOTALS. Same root cause as this PR: OrNull(Tuple(OrDefault(Distinct(OrNull(first_value))))) where the -Tuple state is built with the any_respect_nulls layout but named after any, so the two-level memory-efficient merge reconstructs it with the wrong implementation and segfaults in AggregateFunctionDistinctSingleGenericData<false>::merge.

Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110886&sha=96d9482957eed1f36011908527ae1b34cd7566cf&name_0=PR&name_1=AST%20fuzzer%20%28amd_debug%2C%20targeted%29

Reproduced deterministically on debug and confirmed fixed by this PR (both directions):

SET distributed_aggregation_memory_efficient=1, group_by_two_level_threshold=1, prefer_localhost_replica=0;
-- t_ns(k UInt8, ns Nullable(String))
SELECT first_valueDistinctTupleOrNull(tuple(ns)) RESPECT NULLS
FROM remote('127.0.0.{1,2}', currentDatabase(), t_ns);  -- SIGSEGV without this PR, OK with it

So this PR also closes STID 4514-3f61. The existing 04607_... test already covers the -Distinct-nested variant.

[groeneai]

This PR also fixes a second, distinct crash site: Segmentation fault (STID: 4211-3a99) (AST fuzzer amd_debug, on PR #110380's run, report). Same root cause as this PR (the -Tuple combinator dropping the NullsAction from its serialized state name), but it surfaces through the -ArgMin combinator instead of -Tuple/SerializationTuple.

Original fuzzed query: anyLastDistinctOrDefaultTupleOrNullDistinctOrNullDistinctArgMin(tuple(dummy), '<string>') RESPECT NULLS over remote(...).

Minimal deterministic repro (no remote() needed):

SELECT anyTupleArgMinMerge(s) FROM (
  SELECT CAST(anyTupleArgMinState(tuple(dummy), 'a') RESPECT NULLS AS AggregateFunction(anyTupleArgMin, Tuple(UInt8), String)) AS s FROM system.one
  UNION ALL
  SELECT CAST(anyTupleArgMinState(tuple(dummy), 'b') RESPECT NULLS AS AggregateFunction(anyTupleArgMin, Tuple(UInt8), String)) AS s FROM system.one
);

Why it is the same bug (state type name, RESPECT NULLS vs plain):

function	RESPECT NULLS state name	plain state name
anyArgMin (no Tuple)	any_respect_nullsArgMin (action preserved, fine)	anyArgMin
anyTuple (no ArgMin)	anyTuple (action dropped)	anyTuple
anyTupleArgMin	anyTupleArgMin (action dropped, inherited from the nested -Tuple)	anyTupleArgMin

The -Tuple state name loses the action; the wrong name propagates up through the outer -ArgMin, so on a -State round-trip the state re-resolves to the wrong variant and setIfSmaller reads a bad SingleValueDataString (SingleValueData.cpp:1425 <- AggregateFunctionCombinatorsArgMinArgMax.cpp:188, address 0xfffffffffffffff8).

Verified both directions on debug:

• baseline (Build ID fd9d69ce): the repro SEGVs with the exact CI stack, server dies.
• with this PR (Build ID 95c85a65): anyTupleArgMinState(...) RESPECT NULLS now names its state any_respect_nullsTupleArgMin; the repro no longer crashes (clean exception) and a natural two-shard memory-efficient merge returns a result with the server alive.

Suggest adding an -ArgMin-over--Tuple RESPECT NULLS state round-trip to 04607_tuple_combinator_respect_nulls_state_name.sql so this second crash site is covered by a regression test.

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	86.10%	86.10%	+0.00%
Functions	91.80%	91.80%	+0.00%
Branches	78.20%	78.20%	+0.00%

Changed lines: Changed C/C++ lines covered: 11/18 (61.11%) · Uncovered code

Full report · Diff report

[groeneai]

CI finish ledger — 8600427

CI fully finished, all green. 159 checks passed, 18 skipped, 0 failed. No red checks to own.

Session id: cron:our-pr-ci-monitor:20260718-200000

[alexey-milovidov]

Not bad.