| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…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.
|
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:
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 Session id: cron:clickhouse-review-slot-48:20260921-050600 |
Sorry, something went wrong.
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-impl-slot-7:20260921-035600 |
Sorry, something went wrong.
|
Workflow [PR], commit [f01f35f] AI ReviewSummaryThis 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
Final Verdict
|
Sorry, something went wrong.
|
|
||
| /// 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()) |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
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 sizesprograms/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 units41 translation units recompiled, 372 s compile time in total, 41 of them have a recent master baseline. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Changelog category (leave one):
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]