Session id: cron:clickhouse-worker-slot-4:20260625-121700

cc @rschu1ze @KochetovNicolai — could you review? The query condition cache key encodes only the WHERE/PREWHERE predicate, not the SAMPLE clause, so a sampled read writes a sampling-narrowed mark mask that a later non-sampled query with the same predicate reuses, silently returning too few rows. This disables the cache (both write paths) for sampled reads, mirroring the existing FINAL / unique-key / bucket_id guards.

Workflow [PR], commit [cfd4be6]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR closes the query-condition-cache poisoning hole for sampled MergeTree reads by blocking both the index-analysis write path and the runtime PREWHERE/WHERE write path whenever result.sampling.use_sampling is true. I did not find a remaining correctness issue in the current code, and there are no prior review threads or replies that still require follow-up.

Final Verdict

• Status: ✅ Approve

CI finish ledger — f9c5177

Every failure below has an owner: a fixing PR (ours or external). Only CH Inc sync (private sync) is exempt. None of these are caused by this PR's ReadFromMergeTree sampling-guard change (this PR touches only QCC sampling + a new stateless test); all are pre-existing chronic-trunk / flaky signatures (CIDB 30d cross-PR counts shown).

Session id: cron:our-pr-ci-monitor:20260625-180000

LLVM Coverage Report

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

Full report · Diff report

Found via ClickGap automated review. Please close or comment if this is incorrect or needs adjustment.

TL;DR

Do not cache query condition results for sampled reads

ClickGap verified: reproduced it with the script below on 26.5; tested 6 version(s) — 5 affected, 1 clean (see Affects); bisected to a candidate range — a single introducing PR could not be pinned (see Bisect).

Reproduction

-- Requires a running server (clickhouse local never populates the query
-- condition cache). Default use_query_condition_cache = 1.
CREATE TABLE tab (sample_key UInt64, id UInt64, val String)
ENGINE = MergeTree ORDER BY (sample_key, id) SAMPLE BY sample_key
SETTINGS index_granularity = 8192, min_bytes_for_wide_part = 0;
SYSTEM STOP MERGES tab;
INSERT INTO tab SELECT sipHash64(number*100+1), number, 'hit' FROM numbers(100000);
INSERT INTO tab SELECT sipHash64(number*100+2), number, 'hit' FROM numbers(100000);
INSERT INTO tab SELECT sipHash64(number*100+3), number, 'hit' FROM numbers(100000);
INSERT INTO tab SELECT sipHash64(number*100+4), number, 'hit' FROM numbers(100000);
INSERT INTO tab SELECT sipHash64(number*100+5), number, 'hit' FROM numbers(100000);
INSERT INTO tab SELECT sipHash64(number*100+6), number, 'hit' FROM numbers(100000);

SYSTEM DROP QUERY CONDITION CACHE;
-- A SAMPLE query must NOT write to the query condition cache:
SELECT count() FROM tab SAMPLE 0.1 WHERE val = 'hit' FORMAT Null;
SELECT count() FROM system.query_condition_cache;              -- expected 0, BUG: 6
-- A later non-sampled query must see the whole table:
SELECT count() FROM tab WHERE val = 'hit';                     -- expected 600000, BUG: 98304

Bisect

• The bisect boundary was rejected — PR Fix If, Distinct, DistinctIf, IfState aggregate function combinators with Tuple return type not being able to read older serialized states after introduction of Nullable(Tuple) #100826 fixes aggregate function combinators with Tuple return types and backward compatibility of serialized states, which is unrelated to query condition caching for sampled reads (a query execution/optimization issue). — a coincidental boundary, not the cause; no introducing PR pinned.

Affects

• ✓ Fix is on master — this PR's change fixes the bug on master. Branches below still have the bug and need the backport.
• Reproduces on: 26.5, 26.4, 26.3, 26.2, 26.1
• Does NOT reproduce on: master
• Backports already tracked by maintainers (pr-must-backport): 26.5, 26.4, 26.3, 26.2, 26.1 still show the bug pending the backport merge — no action needed.

Suggested next step

The bug reproduces as far back as the oldest tested release (26.1) and the only converged boundary is topically unrelated — this is a long-standing / pre-existing bug in comp-mergetree, not a recent regression; no introducing PR exists within the tested range.

Component: comp-mergetree
Severity: P2

cc fix PR author: @groeneai
cc fix PR reviewer: @rschu1ze

ClickGap · Finding: phase_d_pr108488

Triage metadata

• Primary component: comp-mergetree
• Secondary components: comp-testing
• Component owners (comp-mergetree): @CurtizJ @alexey-milovidov