Do not execute an IN subquery during statistics part pruning - #114121
Conversation
Statistics part pruning runs before primary key analysis and builds its own KeyCondition per part. That constructor walks the predicate and, for any IN-family atom, called FutureSetFromSubquery::buildOrderedSetInplace, which EXECUTES the subquery. For GLOBAL IN an external table exists, so the source-preserving clone in PreparedSets is skipped and the canonical subquery plan is consumed by `auto plan = std::move(source)`. When that speculative subquery then failed, the exception was swallowed by the per-part best-effort handler in MergeTreeDataSelectExecutor (logged at debug and skipped), leaving the set permanently unbuilt with source == nullptr. No build step was added for it later, so FunctionIn aborted with "Not-ready Set is passed as the second argument for function 'globalNullIn'" and the query's real error was never reported. Part pruning is an analysis-only pass, so it now declines an IN atom whose set is not already built instead of building one, via a require_ready_sets flag on KeyCondition::BuildInfo passed only by StatisticsPartPruner. This is the same discipline tryRewriteInTruthyCondition already applies in the same file. Pruning power is preserved wherever the set does not have to be executed: literal IN lists, StorageSet sets, and subquery sets already built by an earlier analysis stage (notably when the IN column is in the primary key, where buildIndexes runs before pruning). Primary key, partition, minmax and skip index analysis are unaffected because the flag defaults to false. The remaining cost is that an IN (subquery) on a non-key column with statistics no longer prunes parts, which is exactly the case whose alternative is executing a user subquery during part pruning. The per-part handler is deliberately left unchanged: narrowing it by error code would redden 02707_skip_index_with_in, and the protected region runs an arbitrary user subquery so no error code can identify a statistics failure. Related: ClickHouse#107619
Stop background merges on both fixtures so the pinned "Parts: 1/3" counts cannot drift when the three level-0 parts are merged, and pin the settings that reshape the EXPLAIN plan the assertions match. optimize_use_implicit_projections is pinned to 1 rather than 0: at 0 the implicit minmax_count projection is disabled and the first query prunes nothing even without the fix, so the assertion passes on unpatched master and stops detecting the bug. Verified by running the whole test against both binaries: with the pin at 1 the unpatched arm fails and the fixed arm passes; at 0 both pass. A file scope SET also beats runner injection, so the arm still reddens when the test runner injects the value 0. The first assertion's comment now states what it observes (absence of the Statistics row) instead of claiming it proves the subquery was never run.
…n test The test pinned optimize_use_implicit_projections but not its parent optimize_use_projections. The effective value is the conjunction of the two (QueryPlanOptimizationSettings.cpp:213,222), and the test runner randomizes the parent to false with probability 0.05, so the child pin was bypassable: in that case the first assertion passes on an unpatched binary, and the test would report success while detecting nothing. Verified with a negative control: with the parent pin removed and optimize_use_projections=0 injected, the pre-fix binary passes; with the pin in place it still fails, so the pin beats runner injection. Also drop a comment reference to a measurement that is not part of the PR.
Internal second-model review: 3 rounds, final round clean (click to expand)Independent cold review plus a second-model gate on every round; the reviewer never edits the
Also adjudicated: the implementer reported that one prescribed setting value made the regression Validated with a pre-fix and a post-fix binary, build IDs asserted per server rather than inferred: |
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-impl-slot-41:20260810-014800 |
|
cc @hanfei1991 @tiandiwonder, could you review this? Statistics part pruning built its own KeyCondition per part, whose ctor reached buildOrderedSetInplace and executed the IN subquery during index analysis; for GLOBAL IN that consumed the subquery plan, and the per-part best-effort handler then swallowed the real error, so FunctionIn later aborted with "Not-ready Set". Pruning now declines an IN atom whose set is not already built. |
hanfei1991
added
the
can be tested
|
Workflow [PR], commit [b19249e] Summary: ✅
AI ReviewSummaryThis PR makes statistics part pruning decline Final Verdict✅ No new findings. LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 21/22 (95.45%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Compile time of recompiled translation units329 translation units recompiled, 3345 s compile time in total, 329 of them have a recent master baseline. Median compile-time ratio to the baselines is ×1.05 (machine-speed difference or a change affecting every TU); per-TU deltas below are relative to that ratio. |
The reported failure used globalNullIn, but the regression test only exercised plain IN. The guard keys on functionIsInOrGlobalInOperator, so add a GLOBAL IN case and an explicit globalNullIn case to pin the whole spelling family in-tree. Shape selection was measured on both arms (fix buildId 8c85db17, base afab0039). A local outer query with GLOBAL IN discriminates: base emits a Statistics row, the fix emits none, and each new query discriminates in isolation. The CI query's own shape - GLOBAL IN over a cluster() subquery - was rejected as an oracle: its EXPLAIN contains two ReadFromMergeTree blocks and the inner cluster read emits its own Statistics row, so the count is 1 on both arms and the assertion would be vacuous. An in-tree assertion on the external-table branch itself is not possible here: setExternalTable has a single caller, in ReadFromRemote, so only a distributed outer query creates the _data_ temporary table (confirmed from the server log). A local GLOBAL IN covers the spelling through the shared tryPrepareSetIndexForIn guard, which is what the fix keys on, and keeps the test cluster-free and parallel-safe. Verified: whole file base FAIL / fix PASS, 100/100 green over randomized and non-randomized runs, and all six plan-shape pins still discriminate when injected against the file-scope SET.
|
|
||
| -- The `globalIn` family reaches the same guard, and the reported failure used `globalNullIn`. | ||
| SELECT count() FROM (EXPLAIN indexes = 1 | ||
| SELECT count() FROM t_stats_prune_in WHERE c GLOBAL IN (SELECT 1) |
There was a problem hiding this comment.
Can you give me a case that can reproduce LOGICAL_ERROR Not-ready Set is passed as the second argument for function globalNullIn?
These SQLs can run successfully in old versions.
There was a problem hiding this comment.
Yes. The ingredient the committed SQLs lack is a subquery that fails while running on a genuinely remote replica, which needs prefer_localhost_replica = 0. With a two-replica cluster (127.0.0.1 and 127.0.0.2) and statistics on a non-primary-key column:
SET allow_experimental_statistics = 1, allow_statistics = 1, materialize_statistics_on_insert = 1;
CREATE TABLE d (a String, i UInt64) ENGINE = MergeTree ORDER BY a
SETTINGS auto_statistics_types = 'basic', index_granularity = 1;
SYSTEM STOP MERGES d;
INSERT INTO d VALUES ('a', 1);
INSERT INTO d VALUES ('a', 100);
INSERT INTO d VALUES ('a', 200);
SELECT count() FROM d
WHERE globalNullIn(i, (SELECT i FROM cluster('two_replicas', currentDatabase(), 'd') FINAL WHERE a = 'a'))
SETTINGS use_skip_indexes = 0, use_statistics = 0, use_statistics_for_part_pruning = 1,
prefer_localhost_replica = 0;On master this aborts, deterministically (2/2 here):
<Debug> d (SelectExecutor): Failed to apply statistics pruning for part all_1_1_0, skipping
statistics pruning for this part: Code: 181 ... Received from 127.0.0.2 ... doesn't support FINAL
<Fatal> : Logical error: 'Not-ready Set is passed as the second argument for function 'globalNullIn''
Same query id, 3 ms apart, and that is the CI signature from the run this PR was opened for. With the patch the query instead reports the real error, Code: 181 ILLEGAL_FINAL. The isolating control is on the unpatched binary: the same query with use_statistics_for_part_pruning = 0 returns ILLEGAL_FINAL and does not abort, so statistics part pruning is the cause rather than the spelling or the remote read.
On your second point, you are right and the reason is the default: use_statistics_for_part_pruning changed from false to true in 26.4 (SettingsChangesHistory.cpp:258), so before that release the pruner never ran and nothing reached this guard.
One clarification about what took the destructive path. buildOrderedSetInplace only preserves the plan when it can clone it, and ReadFromRemote has no clone(), so IQueryPlanStep::clone throws NOT_IMPLEMENTED and PreparedSets falls back to build(), which moves out source. That is why my earlier local-only attempts returned the subquery error on both binaries: a local plan clones, so the error survives. Two conditions reach the destructive branch, an external table or a non-clonable source, and any remote subquery is the second one.
I did not add this to 04852: it needs a multi-replica cluster and forced remote execution, and the pre-fix arm aborts the server instead of producing a diffable result, so it belongs in tests/integration rather than a stateless test. I am happy to add it there in this PR if you prefer that over keeping the stateless file focused on the pruning outcome.
There was a problem hiding this comment.
Please add a stateless or integration test to make sure trigger the logical error
There was a problem hiding this comment.
Added it as a stateless test, in 04852 (commits 13dd8a6, 66f52d4).
It uses cluster('test_cluster_one_shard_two_replicas', ...) for the subquery plus prefer_localhost_replica = 0, and makes the subquery fail with throwIf. That is enough to hit the abort: a remote subquery has no clonable plan (ReadFromRemote declares no clone(), so IQueryPlanStep::clone throws and PreparedSets falls back to the destructive build()), pruning swallowed the subquery error, and the set stayed unbuilt. So an external table is not required, which is why the earlier local-only attempts saw nothing. The arm expects the subquery's own error, FUNCTION_THROW_IF_VALUE_IS_NON_ZERO.
Verified both directions with clickhouse-test: on the unpatched build it reports server died, and the log has the swallowed subquery error and Not-ready Set is passed as the second argument for function 'globalNullIn' under the same query_id, 3 ms apart. With the fix it passes. Also checked the arm alone in its own file, so it is not relying on the assertions that were already there. A second arm repeats the query with use_statistics_for_part_pruning = 0, where the unpatched build does not abort, which pins the abort to statistics part pruning rather than to the globalNullIn spelling or the remote read. 100/100 green with and without randomization.
One thing worth flagging: I had to pin enable_parallel_replicas = 0 and automatic_parallel_replicas_mode = 0 on that arm. Randomization picks automatic_parallel_replicas_mode = 2 about a quarter of the time, and routing the outer query through parallel replicas reshapes it so the subquery is no longer the consumed set source. Without the pins the arm passed on the unpatched build in those runs, so it would have been dead weight in CI.
On your other point, you are right that these run fine on older versions: use_statistics_for_part_pruning only became true by default in 26.4, so before that the pruner never ran and nothing reached the guard.
I also corrected the PR description, which claimed pruning was preserved for StorageSet sets. It is not: c IN ss_set gives Parts: 3/3 with no Statistics row, on both the patched and unpatched builds. No regression from this change, but the claim was wrong and is now removed.
CI finish ledger - 520c06dEvery failure below has an owner: a fixing PR (mine or external), or a full-effort fix task
Not caused by this PR: the diff touches Session id: cron:our-pr-ci-monitor:20260810-173000 |
The existing arms in this test pin the pruning outcome: statistics part pruning declines an IN atom whose set is not built, and the preserved cases still prune. None of them reaches the abort that motivated the fix, so a regression in the error path would not be caught. Building the set for a remote subquery consumes its plan, because ReadFromRemote declares no clone() and IQueryPlanStep::clone throws NOT_IMPLEMENTED, which makes PreparedSets fall back to the destructive build(). When that subquery then throws, the per-part best-effort handler in statistics part pruning used to swallow the error and leave the set permanently unbuilt, and FunctionIn aborted the server with "Not-ready Set is passed as the second argument for function 'globalNullIn'". The new arm forces the subquery onto another replica with prefer_localhost_replica = 0 and makes it fail with throwIf. It expects the subquery's own error. Measured on a two-replica cluster: the arm aborts the server on the unpatched build (clickhouse-test reports "server died", the log shows the swallowed subquery error and the logical error under the same query_id) and passes with the fix. A second arm repeats the query with pruning off, where the unpatched build does not abort, which attributes the abort to statistics part pruning rather than to the globalNullIn spelling or the remote read.
Randomized runs select automatic_parallel_replicas_mode = 2 together with enable_parallel_replicas = 1, which routes the outer query through parallel replicas. The subquery is then no longer the consumed set source, so the arm stopped reaching the error path: measured on the unpatched build, the arm alone returned the expected error and left the server alive, i.e. it passed for the wrong reason while the pre-existing assertions in the file were what failed. Pinning enable_parallel_replicas = 0 and automatic_parallel_replicas_mode = 0 restores the abort on the unpatched build under that same randomized vector.
CI finish ledger - 66f52d4Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task
The hung-check owner was resolved from 175 jobs, 0 incomplete, 0 dropped, 155 green, 18 skipped. Not caused by this PR: it changes statistics part pruning to stop executing an IN subquery, and Session id: cron:our-pr-ci-monitor:20260811-060000 |
| auto expression = std::make_shared<ExpressionActions>(std::move(actions_dag)); | ||
|
|
||
| auto new_key_condition = std::make_unique<KeyCondition>(filter_dag, context, column_names, expression); | ||
| /// Part pruning is analysis only: it may use an `IN` set that is already built, but must never |
There was a problem hiding this comment.
Future readers will wonder why the comment talks about "IN sets". None of the surrounding code mentions IN sets.
There was a problem hiding this comment.
Right, that comment was written for the fix rather than for the site. Reworded in b19249e to state the local invariant without reaching for anything the surrounding code does not name:
/// Pruning estimates must not run a query pipeline: only state that is already computed may be
/// read here.
| const ExpressionActionsPtr key_expr; | ||
| /// All intermediate columns are used to calculate key_expr. | ||
| const NameSet key_subexpr_names; | ||
| /// If true, an `IN` atom whose set is not built yet is declined instead of building it. |
There was a problem hiding this comment.
This looks more like a quickhack, tbh.
There was a problem hiding this comment.
Fair -- it is a bool threaded through a constructor to change one atom's behaviour, and that is not a nice seam.
What I was avoiding is the one-liner alternative. Passing use_index_for_in_with_subqueries = 0 for the pruning call needs no new parameter, but it also declines sets that are already built, so it silently drops pruning that works today. Measured on this branch, c IN (SELECT 1) where c is the primary key:
| pruner setting | parts read |
|---|---|
| this PR | Parts: 1/3 |
use_index_for_in_with_subqueries = 0 |
Parts: 3/3 |
Primary key analysis has already built that set by the time pruning runs, so what the flag encodes is "use a ready set, never build one", which the existing setting cannot express.
If you would rather not grow the constructor, the alternative I would pick is to stop handing the query context down the pruning path at all, so building a set is unreachable rather than merely declined. That is a larger change to StatisticsPartPruner and I did not want to make it unasked. Say which you prefer and I will do that one; if you would rather an engineer took it from here, I will leave the branch at b19249e and stop pushing.
| -- The assertions match plan text, so pin the settings that reshape it. Both projection settings must | ||
| -- stay 1 (the effective value is their conjunction): at 0 the first query prunes nothing even | ||
| -- without the fix. | ||
| SET enable_analyzer = 1; |
There was a problem hiding this comment.
@groeneai Does the problem reproduce locally without distribution?
There was a problem hiding this comment.
Yes, and my test was overfitted to the CI signature. It reproduces with no distribution at all. Minimal repro against pre-fix master (5c9f28ed), plain clickhouse local, no cluster(), no globalNullIn:
SET allow_experimental_statistics = 1;
SET allow_statistics = 1;
CREATE TABLE t (a String, c UInt64) ENGINE = MergeTree ORDER BY a
SETTINGS auto_statistics_types = 'basic';
INSERT INTO t VALUES ('a', 1), ('a', 100), ('a', 200);
SELECT count() FROM t
WHERE c IN (SELECT c FROM t WHERE throwIf(a = 'a') AND c IN (SELECT 1))
SETTINGS use_statistics_for_part_pruning = 1;| arm | outcome |
|---|---|
pre-fix 5c9f28ed |
Logical error: Not-ready Set is passed as the second argument for function 'in', abort |
| this PR | Code: 395 FUNCTION_THROW_IF_VALUE_IS_NON_ZERO, server alive |
pre-fix, use_statistics_for_part_pruning = 0 |
Code: 395, no abort |
The third row is the control: pruning is what turns the subquery's own error into the logical error.
A remote table was never required. Pruning calls buildOrderedSetInplace, which runs the subquery against a clone of its source plan so a failed speculative build stays recoverable; a source that cannot be cloned falls back to consuming the original. DelayedCreatingSetsStep, which a nested IN adds, is deliberately non-clonable, so the inner IN above is enough. A remote read reaches the same path via ReadFromPreparedSource -- one carrier, not the requirement.
Pushed in b19249e: the cluster() block is replaced by the local form and the test no longer needs the shard tag. Test is -11 lines net. Verified both directions ([ OK ] on this branch, [ FAIL ] server died on pre-fix master), 30/30 randomized draws green, and the 13 neighbouring statistics tests still pass.
The regression test used cluster() because the reported CI failure did, but a remote read is only one way to reach the defect. Pruning builds an IN set through buildOrderedSetInplace, which runs the subquery against a clone of its source plan; a source that cannot be cloned falls back to consuming the original. DelayedCreatingSetsStep, which a nested IN adds, is non-clonable, so a nested IN in the subquery reaches the same path locally. Replacing cluster() with that form keeps the same assertion and lets the test drop the shard tag. Verified both directions: the block aborts with "Not-ready Set is passed as the second argument" on pre-fix master and returns the subquery's own error with this change. Also reword the comment at the pruning call site, which described IN sets at a site where nothing else mentions them.
groeneai
added
the
groeneai-origin-request
8d087c3
robot-ch-test-poll
added
the
pr-synced-to-cloud
5 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed a
LOGICAL_ERROR"Not-ready Set is passed as the second argument for functionglobalNullIn" and the loss of the query's real error message, which could happen when statistics part pruning speculatively executed anIN/GLOBAL INsubquery during index analysis and that subquery failed.Description
Related: #107619 (one carrier). Reported on #113904.
Statistics part pruning runs before primary-key analysis and builds its own
KeyConditionper part. That constructor walks the predicate and, for an IN-family atom, calledbuildOrderedSetInplace, which executes the subquery. ForGLOBAL INan external table exists, so the plan-preserving clone is skipped and the plan is consumed bystd::move(source). When that speculative subquery failed, the per-part best-effort handler inMergeTreeDataSelectExecutorswallowed the exception at debug level, leaving the set permanently unbuilt, soFunctionInaborted withNot-ready Setand the real error was lost. In the cited CI run the discarded error wasCode: 181 ILLEGAL_FINAL.Part pruning is analysis-only, so it now declines an
INatom whose set is not already built, via arequire_ready_setsflag onKeyCondition::BuildInfopassed only byStatisticsPartPruner. Same ruletryRewriteInTruthyConditionalready applies in this file.Cost. Pruning is preserved where no execution is needed: literal
INlists and subquery sets already built earlier, notably when theINcolumn is in the primary key. Primary-key, partition, minmax and skip-index analysis are untouched (the flag defaults to false). What remains:IN (subquery)on a non-key column with statistics no longer prunes, the one case whose alternative is the defect itself.The per-part handler is deliberately unchanged: narrowing it by error code would redden
02707_skip_index_with_in.sql:25, and the protected region runs an arbitrary user subquery, so no code identifies a statistics failure. Complementary to #102192 and #102308, which target the silent-stop case: #102308 restores only in the not-created branch, so an exception still escapes withsourcemoved from.New test
04852_statistics_part_pruning_no_subquery_executionasserts the pruning outcome in the declined case and both preserved cases. Fails on unpatched master, passes with the fix, 100/100 green;02707,04209_statistics_*,02864_statistics_*and03788_statistics_part_pruning*unchanged. Fixes the statistics-pruning carrier, not all of #107619.Workflow [PR]
Sync PR [sync-upstream/pr/114121]
Version info
26.8.1.1713(included in26.8and later)