Check cancellation while searching a constant array

[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 max_execution_time and KILL QUERY being ignored while has, indexOf, indexOfAssumeSorted, countEqual, mapContainsKey or mapContainsValue searches a constant array. A set skip index evaluates its condition over every value it stored for a part in one such call, so a query using that index kept running to the end of the part after its time limit had expired or it had been killed.

Description

Reported in #120593 by @ 4ertus2 and investigated at @ PedroTadim's request. The issue describes two defects; this PR fixes the unkillable query. The slowness is a separate analyzer defect.

Root cause. x IN (<constant array>) resolves to has(<constant array>, x), and has over a constant array runs FunctionArrayIndex::executeConst, whose nested loop performs rows * arr.size() Field comparisons with no cancellation checkpoint. A set index evaluates its condition as one ExpressionActions over every value the index stored for a part, so that single call can receive as many values as the part has rows. The surrounding checks are too coarse: they run once per (part, index), before the work.

The change. executeConst uses the existing CancellationBudget machinery, as its other users do: checked once on entry, then charged per row with the comparisons that row performed. Charging i + 1 rather than arr.size() keeps checkTimeLimit(), which takes cancel_mutex, off the per-row path when a row matches early. One budget covers both index analysis routes and the ordinary data path. One row against an enormous constant array still runs one uninterrupted pass, bounded by the cost of materializing it. The non-constant-array paths scan only the row's own data and are left alone (#112203). hasAll/hasAny/hasSubstr reach the same shape through GatherUtils::sliceHas, separate machinery, so a separate PR unless you want it here.

Validation. Debug build, 200000 rows, one active part. Before: max_execution_time = 3 ran 72.5 s with no error, and KILL QUERY ... SYNC returned after 72.2 s. After: 3.0 s with TIMEOUT_EXCEEDED naming the function, and the kill returns in 0.18 s. The new test covers both index analysis routes and the kill; its three oracle lines diverge on unfixed master. The added per-row cost is bounded at +0.4 ns/row on a 3 element array.

Related: #120593
Related: #112203

---

Workflow [PR]
Sync PR [sync-upstream/pr/120870]

[groeneai]

Internal second-model review: adjudication log (click to expand)

Pre-publication review by an independent model (engine: codex): three gate rounds plus my own pre-gate pass
in each, two fix rounds. The final round re-verified every "AGREE, fixed" row below against the shipped tree.

#	Sev	Finding	Verdict	Evidence / action
1	⚠️	The KILL QUERY scenario bounded the kill wait at 60 s and accepted any eventual QUERY_WAS_CANCELLED, so it could pass on unfixed code	AGREE, fixed	Unfixed KILL ... SYNC returns after 72.2 s on debug and about 52 s on release, inside the old 60 s bound, where the pre-existing per-(part, index) checkpoint raises the same error the test greps for. Bound tightened to 15 s (fixed latency 182.7 ms, dominated by KILL ... SYNC's 100 ms poll), with the route, bulk filtering and the query condition cache pinned on that one query because its oracle is a bound
2	⚠️	The kill could land before execution starts, and then the expected QUERY_WAS_CANCELLED proves nothing: process-list visibility precedes planning. Separately, with bulk filtering randomized off the condition is evaluated per granule, so the deadline oracle can be satisfied by the entry check alone	AGREE, fixed	executeQuery.cpp:2880-2881 inserts the process-list entry before planning at :2886, and :3256-3269 raises QUERY_WAS_CANCELLED for a kill that lands first; the killed query's own lifetime was about 100 ms, so the margin was one debug-build planning phase wide. The kill now issues only after the query reports over one second elapsed, and a per-run guard asserts it accumulated time in secondary-index filtering. One deadline scenario pins bulk filtering on, so a single call per part is what must be interrupted; the other stays free to keep the per-granule arm covered. A build with only the periodic charge removed fails the test
3	⚠️	The description claimed five of the test's six asserted lines diverge on unfixed master	AGREE, fixed	Three is the maximum: one line is a fixture invariant holding in both directions, and two assert that the time went into skip-index filtering, which is more true on an unfixed build. Reworded to the three oracle lines
4	⚠️	hasAll, hasAny and hasSubstr carry the same constant-argument shape with no checkpoint at all, and the carrier enumeration never listed them	AGREE, disclosed	FunctionArrayHasAllAny::executeImpl keeps a constant argument as a GatherUtils const source (hasAllAny.h:78-85), so its work is rows * haystack * needle and is not bounded by the block's own data: the same defect class, and src/Functions/GatherUtils/ contains no cancellation machinery at all. Left out of this PR because it is a different ABI across five translation units and every earlier family under this machinery got its own PR; the description now says so instead of leaving it to be discovered. Say the word and I will do it here
5	💡	The budget overcharges a full scan by one unit and charges an empty array one unit per row; suggested i + (i < arr_size)	DISAGREE	That form charges 0 per row for an empty constant array, and CancellationBudget::chargeUnits(0) takes the early-return branch unconditionally (units_left > units is 65536 > 0), so the check would never fire. The overcharge is 1 unit in 50001 for the measured workload and only makes checks more frequent, the safe direction. i + 1 is deliberate
6	💡	The test still passes if only the entry cancellation call is deleted, so it does not isolate that statement	DISAGREE	Conceded narrowly, refuted in its consequence. The build with only the periodic charge deleted still printed planning route: stopped in function has, and on that build the entry call is the only code that can produce that message, so the test does observe it working. An arm deleting only the entry call has no deterministically constructable scenario: in break mode the pipeline executor cancels between work() calls (PipelineExecutor.cpp:199, :268-273), so a query burning the deadline outside the same expression evaluation ends with a partial result and never re-enters the function, and landing it inside would mean pinning intra-block evaluation order. The statement is also the verbatim idiom of this machinery's other users (CountSubstringsImpl.h:48-53, countMatches.cpp:70-74, FunctionStringReplace.h:73-77) and covers the constant-needle branch, which returns without charging
7	💡	Under timeout_overflow_mode = 'break' a deadline crossed inside the function now raises instead of returning a partial result, and this is not disclosed	DISAGREE	That is the documented contract of the machinery being reused, not something this change introduces: CancellationBudget.cpp:28-31 states it, on the ground that a partial result from a function is a wrong value rather than a smaller one, and @ Algunenano adopted the same machinery in 561c49160f505 and refined it in 2b95d1c84518e
8	💡	The changelog entry quoted an unqualified "72 seconds", measured on a debug build	AGREE, fixed	The entry states the consequence instead of a build-specific number; qualified figures stay in the description
9	💡	"as 13 other files already do" is wrong under either counting rule	AGREE, fixed	Count dropped from the description and the commit message
10	💡	"one stored value per row of the part" states the reported fixture's property as a general one	AGREE, fixed	Reworded to the actual bound: the call can receive as many values as the part has rows
11	💡	The remaining single uninterrupted pass (one row against an enormous constant array) was not disclosed	AGREE, fixed	Now one clause in the description, with the bound it sits under

Severity: ❌ blocker / ⚠️ major / 💡 nit. DISAGREE verdicts carry recorded evidence and are terminal per finding.

Session id: cron:clickhouse-review-slot-8:20260918-145700

[clickhouse-gh]

Workflow [PR], commit [113a176]

Summary: ✅

• Performance Comparison: Performance dashboard

---

AI Review

Summary

This PR adds CancellationBudget checks to FunctionArrayIndex::executeConst() and a focused stateless test, which does fix the constant-array skip-index stall described in the PR. The current head still leaves the documented has(map, key) / notHas(map, key) constant-map carrier outside that new checkpoint, so max_execution_time and KILL QUERY can still be ignored on that supported entrypoint. Verdict: request changes until that remaining carrier is covered too.

Findings

❌ Blocker: [src/Functions/array/arrayIndex.h:943] executeMap() still calls convertToFullColumnIfConst() before extracting the keys array, so has(const_map, key_column) is downgraded to a non-const ColumnArray and never reaches the new cancellation budget in [src/Functions/array/arrayIndex.h:1230]. That leaves the same rows * map_size scan uninterruptible on a documented has(map, key) surface, and notHas inherits it because it delegates to has. mapContainsKey / mapContainsValue are not evidence that this carrier is covered: their adapters preserve constness in [src/Functions/array/FunctionsMapMiscellaneous.cpp:334], so they go through a different path. Suggested fix: preserve constness for the map-to-keys conversion here, or add an equivalent in-loop checkpoint on this carrier, and add a focused regression test on has(const_map, key_column) / notHas(const_map, key_column).

Final Verdict

Status: ⚠️ Request changes
Minimum required actions: cover the constant-map has / notHas carrier with the same cancellation behavior as the constant-array path, and add a focused regression test for it.

LLVM Coverage Report

Measured on commit 113a176.

Metric	Baseline	Current	Δ
Lines	88.20%	88.20%	+0.00%
Functions	91.80%	91.80%	+0.00%
Branches	79.50%	79.50%	+0.00%

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

Full report · Diff report

[groeneai]

The Stateless tests (amd_msan, flaky check) red at 4bea950 was my own new test rather than the fix: 1 run in 50 printed KILL QUERY reached the index scan: 0, meaning the kill landed before the skip index scan and that run asserted nothing about the checkpoint. The other four flavours were 50/50 each.

Cause: the scenario waited for system.processes.elapsed > 1 before killing, and the process list entry is inserted before planning starts, so on a contended runner a second can still be spent ahead of the scan. 846d178 (test only, no source change) verifies the attempt instead of timing it: one that the liveness guard rejects is retried with a longer wait (1 s, 4 s, 16 s), each attempt under its own query id. The oracle is unchanged, and a kill that never stops the query still reports still waiting after 15s on every attempt.

Reproduced deterministically by adding a 3 s scalar subquery to the kill query, which lengthens exactly the phase the old predicate could not see past: the published test then fails with the CI diff byte for byte, this one passes (system.query_log: attempt 1 rejected at 0 microseconds of index filtering, attempt 2 asserted at 1.14 s). 100 of 100 runs pass at 8-way concurrency, and the test still fails on the unfixed base binary (151.7 s, still waiting after 15s).

[clickhouse-gh]

This closes the ColumnConst(Array) carrier, but has still has a second constant-container path through Map. executeMap() does convertToFullColumnIfConst() and then hands a plain ColumnArray to executeArrayImpl() (src/Functions/array/arrayIndex.h:943-968), so has(const_map, key_column) never reaches this CancellationBudget. That leaves the same rows * map_size() unchecked loop the PR is fixing for arrays, just on the documented has(map, key) surface; notHas inherits it because it delegates to has.

I think executeMap() needs to preserve const-ness here the same way MapToSubcolumnAdapter already does in src/Functions/array/FunctionsMapMiscellaneous.cpp:255-256, so a const map becomes a const keys array and reuses executeConst(). A focused regression test on has(const_map, column) would pin the remaining carrier.

[groeneai]

Confirmed, measured on the pushed head. With this PR's own fixture (200k rows, one part) and
max_execution_time = 1, timeout_overflow_mode = 'break', a 1000-key constant map runs 2009 ms with no
error, while the array carrier stops at 1004 ms with Code 159 ... elapsed time limit reached in function has. KILL QUERY sent after 1s returns 1.27s later against 0.166s for the array, and the map error carries no
while executing 'FUNCTION has(...)', so the cancellation is seen by the next pipeline check, not inside the
function. notHas inherits it. mapContains(map, key) and has(mapKeys(map), key) already preserve constness
and already stop, so the gap is specific to the has(map, key) spelling.

The materialized copy makes work track memory, about 1.66 GiB per second of unchecked scan (1000 keys: 1910 ms
at 3.17 GiB; 10000 keys dies on a single 13.04 GiB allocation, so the huge-map example errors rather than
stalls). max_memory_usage defaults to 0, so the window is bounded by server memory rather than by a constant.

I prototyped your remedy: it stops at the deadline, uses 6.05 MiB instead of 3.17 GiB, is 1.4x faster, and a new
scenario in the test reddens on this head for exactly that line. It is not free, though. 31 of 32 differential
cases are byte-identical; the 32nd is has(map(NULL::Dynamic, 1), NULL), which goes 0 to 1. That is what
mapContains, mapKeys and both array paths already return, but it is pinned at 0 by
04338_has_map_dynamic_key_lowcardinality_arg, and the non-const map path keeps returning 0, so const-
preservation trades an array-vs-map inconsistency for a const-vs-non-const one. A constant map above 1e6 keys
also starts raising Code 128 TOO_LARGE_ARRAY_SIZE, as mapContains already does there.

A Dynamic/NULL semantics change does not belong in a cancellation fix for the constant-array carrier this issue
reports, so I am not folding it in here. I will send it separately, in the shape with no semantic delta (chunk
the row range inside executeMap and check cancellation between chunks, which also bounds the peak memory
above), unless a maintainer prefers the const-preserving version with the NULL answer changed deliberately.

[groeneai]

Sent as #121034, and not in the shape I said I would use. The Dynamic-NULL delta that made me pick chunking does not exist in the const-preserving version as implemented: 04338_has_map_dynamic_key_lowcardinality_arg stays green with its reference untouched, and 92 differential cases across every needle wrapper and key type are byte identical to master. Chunking measured worse, since it bounds the copy instead of removing it, so #121034 removes the copy and puts the cancellation checkpoint in the loop that performs the work.

[clickhouse-gh]

Build profile diff (arm_release)

Commit 113a17636e1f1845ae8150d0b11fd2887844456e was not compared: the CI logs cluster did not answer (every POST attempt failed with an exception).

See the job log for details.

[groeneai]

Stateless tests (amd_msan, flaky check) on 846d178: fixed test-only in 881eb13

2 runs in 50 reddened, both on the same line, the anti-vacuity guard of the scenario evaluated during planning (planning route spent over 1s filtering marks: 1 becomes 0). Cancellation itself held: both runs reported stopped in function has for both routes, and the four other flavours passed 50/50 each.

The query is stopped by its own deadline, so the scan only gets whatever is left of that deadline once the phases ahead of it have taken their share. The guard, 1 s of FilteringMarksWithSecondaryKeysMicroseconds out of a 3 s deadline, is therefore really the predicate "those phases fitted in 2 s". They cost 32 to 76 ms locally, a 40x margin, which is why every other run passes; MSan running nproc-1 parallel copies of a 1e10-comparison scan is where they do not. Both deadline scenarios were exposed, not only the one that reddened.

So an attempt its guard rejects is now discarded and retried with a deadline large enough to dwarf that cost, 3 s and then 12 s, each attempt under its own query id so the probe cannot read an earlier attempt's row. The oracle and the reference file are unchanged.

Measured on the published head, Build-ID 6fe1dedb1a9541a007e1e0ab56c5b81893dd6075:

• Deterministic repro: charging the deadline for a slow pre-scan phase with a scalar sleep(2.5), which is evaluated during analysis, makes the published logic fail with the CI diff byte for byte and this logic pass. system.query_log records attempt _0_3 at 462,569 us of index filtering, rejected, then _0_12 at 9,464,236 us, asserted.
• 100 of 100 runs pass at 8-way concurrency, 50 with both randomizers and 50 with --no-random-settings, at 8.89 to 9.85 s per run against the previous head's 8.8 to 9.4 s, because the first attempt suffices whenever nothing is slow.
• The test still fails on the pristine base binary in 146 s, and on a binary built with only budget.chargeUnits(i + 1) deleted in 132 s, so neither a missing fix nor a missing periodic charge can hide behind the retry.

Making the guard relative instead, region * 2 > query_duration, would have been weaker rather than stronger: since the duration is the deadline, that is the predicate "those phases fitted in 1.5 s", and it reads 0 on the same reproduction.

[groeneai]

Build (amd_fuzzers) on 881eb13: PR-caused, fixed in c631758

One line in src/Functions/CMakeLists.txt; no source or test change. The link error was
undefined symbol: DB::makeCancellationCheck(char const*) in every standalone fuzzer executable.

makeCancellationCheck is defined in Functions/CancellationBudget.cpp, which belongs to
clickhouse_functions, while has.cpp is deliberately extracted into dbms_sources by
src/Functions/array/CMakeLists.txt so that ArrayExistsToHasPass.cpp can reach
createInternalFunctionHasOverloadResolver. The cancellation check this PR adds to arrayIndex.h
executeConst is therefore instantiated inside libdbms.a, which left an unresolved reference to
a clickhouse_functions symbol there:

$ nm -C --undefined-only -A build/src/libdbms.a | grep makeCancellationCheck
build/src/libdbms.a:has.cpp.o:  U DB::makeCancellationCheck(char const*)
$ nm -C --defined-only build/src/libdbms.a | grep -c makeCancellationCheck
0

That breaks the self-containment the DBMS_FUNCTIONS list exists to keep, and the targets in
src/{Core,Storages,Compression}/fuzzers link dbms alone, so nothing there can resolve it. No
other build flavour links such an executable, which is why only this one reddened.

CancellationBudget.cpp now joins DBMS_FUNCTIONS, the treatment checkHyperscanRegexp.cpp
already gets for being called from a header. It uses only Context, ProcessList,
CurrentThread and Exception, all already in dbms, so nothing is pulled the other way, and the
callers left in clickhouse_functions still resolve it through their public dbms link. has.cpp
is the only arrayIndex.h includer on the dbms side, and the other header-side callers,
CountSubstringsImpl.h and FunctionStringReplace.h, are instantiated only from
clickhouse_functions translation units, so no other call site leaked one and master was never
affected.

Verified both ways by linking src/Core/fuzzers/names_and_types_fuzzer.cpp against dbms alone
with the fuzzer targets' library set: before the change it fails with exactly this one undefined
symbol, at the same arrayIndex.h line and from the same has.cpp.o member as CI; after it links
with none. libdbms.a now defines the symbol, the full clickhouse binary still links with no
duplicate definition, and 05227_has_const_array_cancellation passes 20 of 20 runs at 8-way
concurrency.

[groeneai]

CI finish ledger - c631758

Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task
whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Performance Comparison (arm_release, master_head, 5/6) / iceberg_suite_tpch	perf run error Code: 159, Timeout exceeded: elapsed 15525.884 ms, maximum: 15000.000 ms on iceberg_suite_tpch.query4.run0, which aborts the test and reds the shard (10 errors, 1 too long, 1 faster, 1 unstable). Identical run error on true master (REFs/master) and on 60+ unrelated PRs today, so it is not caused by this PR. Root cause: the join-reordering FK to PK heuristic from #120176 picks a 1-row estimate for the unestimated Iceberg relations and pushes that query over the harness 15 s cap (issue #120921)	#120923 (external, open): Revert "Join reordering: FK->PK heuristic for equi joins without NDV statistics"
Mergeable Check, PR	praktika aggregators, red only because they reflect the single shard failure above	#120923 (external, open)

Session id: cron:our-pr-ci-monitor:20260919-133220

[groeneai]

CI finish ledger - c687601

Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task
whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Integration tests (amd_tsan, 2/6) / test_storage_nats/test_nats_jetstream_credentials_rotation.py::test_jetstream_credentials_rejected_after_rotation	the sole red of 1222 tests in that shard: create_pipeline() at :175 gets Code: 665. DB::Exception: Cannot connect to Nats last error: (unix/sock.c:56): poll error: 4. Error 4 is EINTR, which the vendored nats-io client treats as a socket error, and the test allows a single connect attempt. Not reachable from this diff, which touches no NATS or contrib code. Measured over the last 30 days: 27 rows across 25 distinct pull requests and 3 true-master rows	#120202 (external, open)
Build profile diff	job-level RuntimeError: CI logs cluster query failed, raised in query at ci/jobs/build_profile_diff_job.py:234 and reached from compare_opt_functions via run_comparison (:1556), so a non-ok CI logs cluster response aborts the job with a traceback instead of degrading to "no baseline". Not reachable from this diff: the job's only work is reading build_time_trace on the logs cluster, and it neither builds nor runs the server. Measured over the last 24 hours: 203 error rows across 203 distinct heads and 202 distinct pull requests, latest at 05:13:45Z, and 0 rows on master, since the job runs on pull requests only	#118708 (mine, open)
PR	praktika rollup of the two rows above (Failed: Build profile diff, Integration tests (amd_tsan, 2/6))	#120202 (external, open) and #118708 (mine, open)

Session id: cron:our-pr-ci-monitor:20260920-053046