FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Do not re-execute an IN subquery when the parallel replicas local plan is re-planned by groeneai · Pull Request #121273 · ClickHouse/ClickHouse · GitHub

Do not re-execute an IN subquery when the parallel replicas local plan is re-planned - #121273

Open
groeneai wants to merge 2 commits into
ClickHouse:masterfrom
groeneai:pr-local-plan-adopts-in-subquery-set
Open

groeneai wants to merge 2 commits into
ClickHouse:masterfrom
groeneai:pr-local-plan-adopts-in-subquery-set

Conversation

groeneai commented Sep 21, 2026
edited by clickhouse-gh Bot
Loading

Copy link
Copy Markdown
Collaborator

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

Fix an IN subquery being planned, index-analyzed and executed twice when parallel replicas run with parallel_replicas_min_number_of_rows_per_replica above 0, reading 1.5x more rows than necessary. Results were correct.

Description

Related: #118275. CI provenance: https://s3.amazonaws.com/clickhouse-test-reports/praktika.html?PR=71028&sha=f207a387f322d1875970230d16c1c333619d4f37&name_0=PR

What breaks. With enable_parallel_replicas = 1 and parallel_replicas_min_number_of_rows_per_replica > 0 (default 0), any WHERE col IN (SELECT ...) runs its subquery twice: read_rows 100,000 to 150,000, IndexAnalysisRounds 2 to 3. Under distributed_index_analysis each extra analysis is also a cross-replica round trip, which is what 04052_distributed_index_analysis_in_subquery_no_quadratic counts.

Root cause. Above 0 the setting makes the planner build a throwaway StorageDummy plan to harvest filter DAGs for the row-count estimate (collectFiltersForAnalysis). That estimate's index analysis fills the IN set behind the DAG, and the DAG reaches the executed read as SelectQueryInfo::filter_actions_dag, carrying the same filled set. createLocalPlanForParallelReplicas then re-plans the local replica's query from the query tree with a fresh PreparedSets, so its set is empty and its KeyCondition fills it again. The outer read is spared because its analysis is transplanted; a read nested under an IN is not.

The change. createLocalPlanForParallelReplicas offers the analyzed read's already-filled sets to the re-planned plan through the existing reuseBuiltSets, the remedy shape recorded at CreatingSetsStep.cpp:382-388 (share the set, do not withhold it). A set whose fill also feeds a GLOBAL IN external table is never offered. Nothing is adopted unless an earlier analysis already filled the set, so at the default 0 the re-planned plan is unchanged.

Scope. This closes the parallel_replicas_min_number_of_rows_per_replica trigger only, not #118275's parallel_replicas_plan_based = 1 trigger: measured, that path doubles both analyses and behaves identically before and after this change.

Validation. 04052 fails 3/3 on master with the setting and passes 50/50 here; the new test counts 3 before and 2 after; all query results unchanged.


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

…n is re-planned

With parallel_replicas_min_number_of_rows_per_replica above 0 an `IN` subquery was
planned, index-analyzed and executed twice. Measured on a 100,000-row MergeTree with
`SELECT sum(key) FROM t WHERE key IN (SELECT key FROM t WHERE key > 50000)`: read_rows
100,000 -> 150,000 (1.50x) and IndexAnalysisRounds 2 -> 3, with correct results. Under
distributed_index_analysis each extra analysis is also a cross-replica round trip,
which is what 04052_distributed_index_analysis_in_subquery_no_quadratic counts.

The chain. Above 0 the setting makes collectFiltersForAnalysis build a throwaway
StorageDummy plan purely to harvest filter DAGs for the row-count estimate. The
estimate's index analysis (selectRangesToRead -> KeyCondition::tryPrepareSetIndexForIn
-> buildOrderedSetInplace) fills the `IN` set behind that DAG, and the DAG reaches the
executed read as SelectQueryInfo::filter_actions_dag: ActionsDAG::clone copies
Node::column by pointer, so the executed read's filter surface holds the same,
already-filled set object. createLocalPlanForParallelReplicas then re-plans the local
replica's query from the query tree with a fresh PlannerContext and a fresh, empty set,
so the first KeyCondition built over the local plan's `key IN <empty set>` fills it a
second time. The outer read is spared because its analysis is transplanted; a read
nested under an `IN` is not.

Measurement decided which surface to collect from. Of the analyzed read's four
candidate filter surfaces only getQueryInfo().filter_actions_dag ever carries the
filled set, both with prewhere at its defaults and with optimize_move_to_prewhere and
query_plan_optimize_prewhere off; getFilterActionsDAG(), the prewhere actions and the
row-level filter are null in every configuration measured. The collector is therefore
called on that one surface instead of on all four.

createRemotePlanForParallelReplicas and createRemotePlanFragmentForParallelReplicas are
deliberately left alone: those plans are serialized and shipped, an in-memory Set
cannot travel, and each replica must build its own.

The external-table guard sits in the new collector rather than in reuseBuiltSets or
collectBuiltSets because the difference is in the consumer, not in the mechanism. The
existing collectBuiltSets deliberately shares GLOBAL IN sets: its only consumer is the
automatic-parallel-replicas probe plan, which is never executed, and 03800's query_6
measures that behaviour. This consumer is executed, so it declines any set whose fill is
entangled with an external table. That is a conservative boundary rather than a defect
being fixed: on this path buildQueryTreeForShard materializes a GLOBAL IN subquery into
the temporary table before the local plan exists and replaces the subquery with a read of
that table, so the re-planned set is keyed on the table and cannot match an offered set
anyway. The hash collision the predicate rules out is real - the collected GLOBAL IN set's
hash is byte-identical to a plain IN's over the same subquery text, because the set is
registered before that substitution - and refusing it keeps reuseBuiltSets,
collectBuiltSets and the probe path byte-for-byte unchanged.

Pruning is preserved rather than degraded. Both in-place build paths fill the set's
elements, so an adopted set reports hasExplicitSetElements() and the local plan's own
KeyCondition still uses the index instead of falling back to a set-less condition.

At the default 0 there is no pre-pass, the filter surface is null, nothing is collected
and reuseBuiltSets returns at its first statement, so those plans are unchanged. No
setting is added, changed or re-defaulted.

This closes the parallel_replicas_min_number_of_rows_per_replica trigger. It does not
close the parallel_replicas_plan_based = 1 trigger of ClickHouse#118275: that path splits one
plan into fragments rather than re-planning it, it doubles both analyses rather than
one, and 04052 under it behaves identically before and after this change.

Related: ClickHouse#118275
Drop the fix narration at the local-plan reuse site and the header's caller cross-reference; keep the GLOBAL IN temp-table property and the external-table contract.
groeneai added can be tested Allows running workflows for external contributors groeneai-origin-request PR origin: a maintainer pinged or directed groeneai labels Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author
Internal second-model review: adjudication log (click to expand)

Pre-publication review by an independent model (engine: codex), over 3 review rounds and 2 fix rounds:
2 gate findings plus 4 from my own cold reads. Every row was fixed before this PR was published, and
the final round returned 0 findings from both the gate and my own read.

# Sev Finding Verdict Evidence / action
1 ⚠️ The GLOBAL IN arm of the new test does not exercise the external-table exclusion its comment claimed to cover (05234_pr_local_plan_adopts_in_subquery_set.sql) AGREE, fixed buildQueryTreeForShard substitutes a temporary TableNode before the local plan is built and the tree hash mixes in node type, so the re-planned set can never match the offered one; the arm's stated purpose is corrected to what it does check, and the only discriminating shape costs one extra index-analysis round, never a wrong result.
2 💡 The commit message and the new collector's comment presented that exclusion as preventing replicas from reading an empty temporary table AGREE, fixed On this path the temporary table is filled eagerly before the local plan exists and the one setExternalTable caller runs after adoption, so both now describe a conservative boundary instead.
3 💡 The report oracle (IndexAnalysisRounds = 2) is also the value a run with parallel replicas disengaged produces AGREE, fixed The report SELECT now also asserts ProfileEvents['ParallelReplicasUsedCount'] > 0, as 02950_parallel_replicas_used_count does, verified under a call-site mutation to be an engagement assertion and not a second detector.
4 ⚠️ The regression sweep was keyed on filenames and default settings, so it missed the only pre-existing test in this change's trigger shape AGREE, fixed 03232_pr_not_ready_set is that test; it runs 5/5 on both sides and its A/B shows this change also closes it there (IndexAnalysisRounds 3 to 2, read_rows about halved), recorded as no regression on the destructive in-place path rather than as coverage of the counting property, with no line added to the diff.
5 💡 The no-prewhere arm's comment claimed the filter reaches the read through another surface AGREE, fixed The probe shows only SelectQueryInfo::filter_actions_dag ever carries the filled set, identically with prewhere on and off; the arm stays (its before-value is 3 and the mutation reddens it independently) and its comment now states what it checks.
6 💡 A second unrun test, 04327_pr_view_union_empty_branch, also pins the setting above 0 DROPPED on my own measurement Its six IN ( are all tuple INs, which build a FutureSetFromTuple the collector skips, so it is not in the trigger shape; run anyway because it was free, 2/2 on both sides.

Severity: ❌ blocker / ⚠️ major / 💡 nit. Findings on hunks unchanged by a fix round are auto-dropped.

Four properties in the wrong-results class were re-derived independently in the final round rather than
inherited: adoption matches on the content-derived FutureSet::getHash() and only ever replaces a set
that is not yet created; Set's hash table is built unconditionally by insertFromBlock and the
collected set carries explicit elements, so runtime FunctionIn and index pruning are both preserved;
the new block sits before the right_branch_selected guard because a set is keyed by subquery content
while an AnalysisResult is bound to one table expression; and the adoption runs in the only window
where the re-planned plan's sets are still reachable, confirmed by the probe rather than by reading.

Session id: cron:clickhouse-review-slot-48:20260921-050600

Copy link
Copy Markdown
Collaborator Author
Pre-PR validation gate (click to expand)
# Question Answer
a Deterministic repro? Yes: 04052_distributed_index_analysis_in_subquery_no_quadratic --no-random-settings --client-option parallel_replicas_min_number_of_rows_per_replica=1 is 3/3 FAIL on master and 50/50 PASS here, and the new test's counter is 3 before and 2 after on every single run, so this is not a rate.
b Root cause explained? Above 0 the setting makes the planner build a throwaway StorageDummy plan for the row-count estimate, whose index analysis fills the IN set behind the filter DAG that then reaches the executed read as SelectQueryInfo::filter_actions_dag, while the local replica's plan is re-planned with a fresh PreparedSets, so its KeyCondition fills the set a second time.
c Fix matches root cause? Yes: the already-filled set is handed to the re-planned build through the existing reuseBuiltSets, the remedy shape recorded at CreatingSetsStep.cpp:382-388, with no widened bound, no no-random-* tag, no settings clamp, no defensive check and 04052's reference untouched.
d Test intent preserved / new tests added? 04052 is unmodified and its BETWEEN 3 AND 4 bound still holds; the new 05234_pr_local_plan_adopts_in_subquery_set adds three IndexAnalysisRounds arms plus a GLOBAL IN result arm, each also asserting ParallelReplicasUsedCount > 0 so a future disengagement of this path reddens the test instead of passing on a count of 2 that plain local execution also produces.
e Both directions demonstrated? Yes, on Build-ID-verified binaries: reverting only src/ reddens the new test at estimate 3 and estimate_no_prewhere 3 while the control stays at 2 and every sum stays 3749925000, and reverting only the call site reproduces exactly that with the engagement column still 1, so the counters and not the engagement assertion are what detect the defect.
f General across code paths? createLocalPlanForParallelReplicas is the single re-planning site and buildQueryPlanForParallelReplicas reaches it, so one edit fixes both variants; the two remote-plan builders are deliberately excluded because those plans are serialized and shipped and an in-memory Set cannot travel, and applyParallelReplicas does not call the changed function at all, which I measured.
g Generalizes across inputs? The unit adopted is a whole Set keyed by the subquery's tree hash, so column type wrappers are not a dimension, and the dimensions that are real were measured: prewhere on and off (both arms reddening), IN vs GLOBAL IN, the setting at 0 vs 1, parallel_replicas_plan_based 0 vs 1, plus an indexHint-wrapped IN, which is covered because appendSetsFromActionsDAG recurses into FunctionIndexHint::getActions().
h Backward compatible? Yes, and nothing to record: no setting is added, changed or re-defaulted, so there is no SettingsChangesHistory.cpp entry, and at the default 0 the filter surface is null, nothing is collected and reuseBuiltSets returns at its first statement.
i Invariants and contracts preserved? The invariant is that a re-planned build may adopt a set an earlier build filled, except where that fill also feeds an external table; that predicate is a conservative boundary rather than a defect being fixed, since on this path buildQueryTreeForShard materializes the subquery into the temporary table before the local plan exists and keys the re-planned set on that table, so it could not match an offered set anyway; pruning is preserved because the collected set reports hasExplicitSetElements() == true, and storage, tuple and correlated sets are excluded by the typeid_cast and the isCreated() check.

Session id: cron:clickhouse-impl-slot-7:20260921-035600

clickhouse-gh Bot closed this Sep 21, 2026
clickhouse-gh Bot reopened this Sep 21, 2026

clickhouse-gh Bot commented Sep 21, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [f01f35f]


AI Review

Summary

This PR reuses a set built during the parallel-replicas row-count estimate so the re-planned local plan does not rerun a plain IN subquery. The approach is directionally right, but the new reuse filter is still too coarse for mixed plain IN / GLOBAL IN shapes, so the double-execution bug remains there.

Findings
⚠️ Majors
  • [src/Planner/Utils.cpp:835] appendBuiltSetsFromActionsDAG drops any ready set once external_table_expected was ever set on the original analyzed FutureSetFromSubquery. CollectSets sets that bit on an already-registered set when the same RHS also appears under GLOBAL IN, but buildQueryTreeForShard rewrites the GLOBAL IN arm to a temporary table before the local plan is re-planned. The local plain IN arm still hashes to the original subquery and could safely adopt the ready set, yet this guard filters it out, so a query like x IN (subq) AND y GLOBAL IN (subq) still executes the subquery once in the estimate and again in the local plan. Suggested fix: do not suppress reuse solely on external_table_expected; key the exclusion on an actually attached external table or another truly non-replayable case, and add a regression arm that mixes plain IN and GLOBAL IN over the same RHS.
Final Verdict
  • Status: ⚠️ Request changes
    Minimum required actions:
  • Narrow the GLOBAL IN exclusion so a sibling plain IN can still reuse the analyzed set after buildQueryTreeForShard splits the GLOBAL arm onto a temporary-table key.
  • Add a focused regression test for mixed plain IN / GLOBAL IN over the same RHS.

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Sep 21, 2026
Comment thread src/Planner/Utils.cpp

/// Filling a `GLOBAL IN` set also populates the temporary table the remote replicas read, so
/// adopting such a set in place of a fill would skip that write.
if (set_and_key->external_table_expected || from_subquery->hasExternalTable())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

external_table_expected is sticky on the original FutureSetFromSubquery for any sibling GLOBAL IN over the same RHS (src/Planner/CollectSets.cpp:138-152). That means a query like x IN (subq) AND y GLOBAL IN (subq) still misses this reuse path: buildQueryTreeForShard rewrites the GLOBAL IN arm to a temporary table before the local plan is re-planned (src/Storages/buildQueryTreeForShard.cpp:1006-1028), so the plain IN arm still matches the original subquery hash, but this guard drops the ready set before reuseBuiltSets sees it. The subquery will still run once in the estimate and again in the local plan.

I think the unsafe case here is an actually attached external table (hasExternalTable()), not the original external_table_expected bit. Narrowing this guard and adding a mixed plain IN / GLOBAL IN regression arm would preserve the optimization for the plain IN side without changing the GLOBAL temp-table path.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Confirmed the mechanism, and measured what it costs. CollectSets does mark an already-registered set (src/Planner/CollectSets.cpp:136-142), so with key IN (subq) AND intDiv(value, 100) GLOBAL IN (subq) the plain arm's set carries the bit and the reuse is refused. On this head: 4 IndexAnalysisRounds and 249,999 rows read, against 3 and 199,999 for the same query with parallel_replicas_min_number_of_rows_per_replica = 0. Every arm returns the same sum, so what is left in that shape is the extra work, not a wrong result.

The predicate you propose does not survive the same check. hasExternalTable() is false for every set at this point: the table is attached at pipeline build time, from ReadFromParallelRemoteReplicasStep::initializePipeline -> addFilters -> tryBuildAdditionalFilterAST (ReadFromRemote.cpp:1140, :486), and external_table_set is only populated on the pre-analyzer ActionsVisitor path. I built your version to be sure: the mixed query drops to 3 rounds / 199,999 rows, which is exactly the control, so the narrowed guard refuses nothing at all. It is a removal, not a narrowing. The bit exists because the attachment is late (PreparedSets.h:49-52), and CreatingSetsStep::usesExternalTable() already keys this class of plan decision on the same disjunction.

So I am keeping the guard. This is the first consumer of reuseBuiltSets whose plan is really executed, and the only thing that would make adoption safe without the bit is that buildQueryTreeForShard rewrites every GLOBAL IN before the local plan is built. I would rather leave the mixed shape unoptimized than rest on that. The scope of this change stays as stated in the description. If a reviewer prefers the wider behaviour, dropping the condition is a one-line change and I will add the mixed arm with a mutation control next to it.

clickhouse-gh Bot added the comp-parallel-replicas Parallel reading from replicas for query speed (not data replication consistency). label Sep 21, 2026

clickhouse-gh Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Build profile diff (arm_release)

Comparing f01f35f52 with master d14841253 (stripped binary size, per-symbol sizes and ThinLTO time; object sizes against the warmup build of de0734db6; compile times per translation unit against the most recent warmup build that recompiled it).

✅ No significant changes.

Binary sizes

programs/clickhouse-stripped: smaller than the master baseline by the known offset between the two builds, so the difference is not shown. A delta that differs from the offset by more than 50% of it is shown, in either direction.

The official master build is compiled with -g and a pull request build is not, and XRay counts debug instructions towards its instrumentation threshold, so master instruments thousands of functions more and its binary is ~0.4% larger no matter what the pull request does.

Compile time of recompiled translation units

41 translation units recompiled, 372 s compile time in total, 41 of them have a recent master baseline.

Job report

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors comp-parallel-replicas Parallel reading from replicas for query speed (not data replication consistency). groeneai-origin-request PR origin: a maintainer pinged or directed groeneai pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant


Back | FazBrowse Home | New Git URL