Derive the join order randomization seed from the initial query id by groeneai · Pull Request #112660 · ClickHouse/ClickHouse

groeneai · 2026-07-30T20:35:58Z

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

Fixes LOGICAL_ERROR: Coordination mode mismatch for stream when a multi-way join runs under parallel replicas with query_plan_optimize_join_order_randomize = 1. The seed is now derived from the initial query id, so every query plan of one query, including the plans built by remote replicas, picks the same join order.

Description

Reported on #107567 as STID 6117-68e9, tracked by #106039: CI report.

query_plan_optimize_join_order_randomize = 1 means "derive a random seed", but the substitution happened in the QueryPlanOptimizationSettings constructor while the session value stayed 1. That constructor runs once per plan construction, and one query builds several plans: one per scalar subquery and, under parallel replicas, one per replica. A replica re-plans the query text it receives, and also re-optimizes a serialized plan, since JoinStepLogical::serialize does not encode the optimized flag. Each plan rolled its own seed.

The seed feeds getRandomizedStats, which replaces the relation statistics, so a different seed picks a different leftmost relation. findReadingStep descends only children.front(), so a different table receives requestReadingInOrder and announces a different CoordinationMode for one stream; stream_to_coordinator is keyed by table name alone, so the second announcement throws. The abort dump contains both preconditions.

The fix derives the seed from initial_query_id, which is stable for one query and reaches remote replicas in ClientInfo, so every plan of one query agrees on the seed. 0 (the default) and explicit seeds above 1 are untouched; an empty initial_query_id, which an internal or background plan carries, keeps the old random seed. The trade is deliberate: a query building several plans now explores one ordering instead of one per plan. Coverage across queries is retained because each gets a fresh id and so a fresh seed.

On a three-replica cluster one initial query id yields 3 distinct seeds before the fix and 1 after. The new test carries 13 assertions, of which 8 fail on unmodified master.

Validation matrix and remaining carriers

This removes one carrier of the class, not all: the process-global hash-table statistics cache and per-replica real statistics can also diverge join order without this setting. Both were inactive for the reported abort. Followed up separately.

The pre-fix binary is a pristine master build whose copy of the modified file is byte-identical to origin/master.

measurement	pre-fix	post-fix
distinct seeds within one query id (2 plan constructions)	2 (5 of 5 runs)	1 (5 of 5 runs)
distinct seeds across 3 replica query ids of one initial query id	3 (3 of 3 runs)	1
`count()` result, 3-way join under parallel replicas	200	200
new test via `clickhouse-test`	FAIL on 8 of its 13 assertions	pass
new test, repeated runs	n/a	50 passed, 0 failed; 20 passed, 0 failed

Mutation tests: reverting the derivation to randomSeed reddens 8 assertions; replacing the hash with a constant reddens 6 of those 8, leaving green only the two that assert a single seed rather than the derived one, so no assertion is satisfiable by an arbitrary constant. Lowering the fixture's replica count reddens the fan-out assertion alone; removing the join-swap pin reddens the reorder control alone; dropping the plan-serialization pin reddens the serialized-plan assertion alone. Removing the clamp that forces the derived value above 1 reddens nothing, as expected: it fires only when the hash is 0 or 1 (probability 2/2^64).

Must-not-change controls stay green in both arms: an explicit seed is still used verbatim, two distinct explicit seeds still produce different join orders, and = 0 derives no seed.

The neighbouring parallel-replicas coordination tests, the carrier 04305_dpsub_outer_join, and the join-order-reordering tests all pass. Every other failure in a wider local sweep reproduces byte-identically on the pristine binary and is a sandbox gap.

`query_plan_optimize_join_order_randomize = 1` means "derive a random seed", but the substitution happened in the `QueryPlanOptimizationSettings` constructor while the session value stayed `1`. That constructor runs once per query plan construction, and one query builds several plans: one per scalar subquery, one per `Merge` child, and, under parallel replicas, one per replica, because `serialize_query_plan` is off by default so every replica re-plans the query text it receives. Each of them rolled its own seed. The seed feeds `getRandomizedStats`, which replaces the relation statistics, so a different seed picks a different leftmost relation. `findReadingStep` descends only `children.front()`, so a different table then receives `requestReadingInOrder` and announces a different `CoordinationMode` for the same stream. Since `stream_to_coordinator` is keyed by the table name alone, the second announcement throws `Coordination mode mismatch for stream` in `ParallelReplicasReadingCoordinator::getOrCreateCoordinator`. This has been firing across many unrelated PRs; the abort dump on the run reported in ClickHouse#107567 contains both preconditions (`query_plan_optimize_join_order_randomize = 1` and `parallel_replicas_local_plan = false`). Derive the seed from `initial_query_id` instead. It is stable for one query and is propagated to remote replicas in `ClientInfo`, so every plan of one query derives the same seed while different queries still explore different orderings. The derived value is forced above 1 so it can never read back as the sentinel (1) or as disabled (0). An empty initial query id keeps `randomSeed()`: an internal or background plan has no query to be consistent with and no parallel-replica fan-out to diverge from. `0` (the default) and explicit seeds above 1 are untouched, so any query that does not opt into randomization is bit-identical. As a side effect a `= 1` run becomes reproducible, which is what the setting's own doc string asks for. Measured against a three-replica cluster, counting seeds across all replica query ids of one initial query id: 3 distinct seeds before, 1 after, deterministic over repeated runs. This removes one carrier of the failure class, not all of them. The process-global hash-table statistics cache and per-replica real statistics can still diverge join order for a query that does not set this setting; both were inactive for the reported abort, and they are followed up separately. Related: ClickHouse#106039

Review round 1 on the join-order-randomize seed fix. The regression test asserted that one query gets one seed, but not that the seed is the value derived from the initial query id. A compile-time constant seed satisfies uniqueness within a query and across replicas, so it would have passed every assertion while silently removing the cross-query randomization the setting exists for. Each of the two affected cells now also compares the logged seed against greatest(sipHash64(<initial query id>), 2), reusing that cell's existing query_id subquery so the two rows cannot drift apart. SQL sipHash64 over a String hashes the same bytes as the C++ overload, with no terminator, and greatest(..., 2) is UInt64, so the comparison reproduces the clamp exactly; this was measured before being asserted. A mutation replacing the hash with a constant now reddens exactly those two rows and nothing else. The setting description claimed the change "keeps a run reproducible". That is false as a user-facing statement: rerunning the same query text mints a new initial query id and therefore a new seed, so reproducibility requires an explicit query id. The description is published through system.settings, so it now states only what holds, and also covers the empty initial query id case that falls back to a random seed. The comment at the fix site kept the incident narrative that belongs in the commit message and the pull request description, so it is reduced to the invariant and the reason for the clamp. The code lines are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… guarantee The description claimed that rerunning the same query text "still explores a different ordering". The change only guarantees a new seed: the chosen order is a hash of (seed, relation index, table name) over a finite set of orderings, so two distinct seeds can legitimately select the same order. The regression test added by this branch records a measured instance of exactly that, naming seeds 12345 and 54321 as colliding for its table names, so the published text contradicted the diff's own test comment. The text now promises a new seed and says the ordering may differ, and points at an explicit query id as the way to keep the seed stable. The other four claims are unchanged: derivation from the initial query id, sharing with remote replicas, the empty-id fallback to a random seed, and the verbatim use of a value above 1. The description reaches users through system.settings.description and the generated settings page, which is why the wording is corrected rather than left to the PR body. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…sertion The guard read `uniqExact(query_id) > 1` under the label "cell C fanned out to several replicas". The predicate is correct for what it can observe, but the label promised more than it delivers: the initiator's own row is always one of the counted ones, so the check cannot distinguish "initiator plus one remote" from "initiator plus two remotes". Measured directly, it stays green with `max_parallel_replicas = 2`, so it does not notice a degraded fan-out. The predicate is kept verbatim and relabelled to the property it does pin, the same wording cell A already uses for the same fact: the query really did construct more than one plan. That is what protects the two substantive cell C rows from passing vacuously on a single matching row. The fan-out itself is now asserted separately from `ProfileEvents['ParallelReplicasAvailableCount']` on the initiator's own `query_log` row. That counter is incremented once per replica that actually joined the query, and it is accumulated in the initiator's own process, so it does not depend on when the replicas' `query_log` rows become visible. An earlier attempt to count the remote rows themselves was 35% flaky for exactly that reason: `SYSTEM FLUSH LOGS` flushes the local queues only and has no cross-replica barrier, so at the moment the initiator flushes some replica rows may not be enqueued yet. The similarly named `ParallelReplicasUsedCount` counts per replica that requested ranges and reads 1 here, so it is not the right event. The observing statement pins `enable_parallel_replicas = 0` because cell C sets it at session level, matching what `02950_parallel_replicas_used_count` does for its own observers.

…re-planning comment Round 4 of review. Two test-only changes, no behaviour change. The cell C fan-out row read `ProfileEvents['ParallelReplicasAvailableCount'] = 3`. That counter is incremented in the connection-establishment loop of RemoteQueryExecutor, before `sendQuery`, so it counts compatible connections rather than plan constructions that agreed on the seed. Replace it with a row that counts distinct plan constructions carrying this query's derived seed. The scope is the seed value computed from the initiator's own `query_log` row, which is always locally visible; joining the replicas' rows instead races their `query_log` flush, because `SYSTEM FLUSH LOGS` has no cross-replica barrier. The header comment claimed a replica re-plans because `serialize_query_plan` is off by default. That reason does not hold on the `distributed plan` functional jobs, which enable the setting in the default profile. The mechanism that holds on both paths is that `JoinStepLogical::serialize` does not encode the `optimized` flag, so the join-order rewrite runs again on the replica whether it receives the query text or a serialized plan. State that instead; the setting is deliberately left unpinned so the serialized-plan path keeps its coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…e query_log lookups Three test-only changes to the 04653 regression test, all requested by review. Split the cell C non-vacuity assertion in two. It previously loaded two independent properties onto one inequality: that several plan constructions agreed on the derived seed, and that the fixture fanned out to the configured replica count. The population that carries the derived seed is one initiator plan construction plus one per replica, so over a population of max_parallel_replicas + 1 no single bound can express both, and "secondary replica participation" cannot be expressed at all. Each property now has its own row with a bound that means what its label says: a >= 2 seed-agreement bound, and an exact = 4 fan-out bound deliberately coupled to max_parallel_replicas = 3. Reducing the replica count to 2 reddens only the fan-out row, which is what the split is for. Pin the join swap setting on both EXPLAIN statements of the cell B reorder oracle. That oracle compares the read orderings produced by two different explicit seeds, and a forced swap changes the read order independently of the seed, so the row failed for every forced value of the setting. Both the functional runner and the stress runner randomize it, and the existing no-random-settings tag does not protect against the stress runner, which injects through a client option that wins over the randomized value. Statement-level scope is used so cells C and B2 keep exercising the default. Order the four expected-seed lookups over system.query_log by event time, descending. That table is server-global and append-only, and a stress thread runs with a fixed database, so an unordered single-row pick could hash a previous run's query id. The ordered form is deterministic and selects the newest run's initiator.

The runner re-runs a failed test in the supplied database without cleaning it (tests/clickhouse-test wraps every execution in a retry loop, and TestCase._cleanup is gated on `not args.database`), and a stress thread supplies a fixed --database. One database can therefore hold several attempts of this test. The observed side of six assertions joined `system.text_log` through every initiator of a `log_comment` in the database, so it saw one derived seed per attempt: the `uniqExact(...) = 1` rows measured the attempt count and the `groupUniqArray` comparisons compared a multi-element array against a single-element one. Both reddened permanently on the second attempt, reporting a correct fix as broken. Reuse the ordered newest-initiator lookup that the expected side already used, as the `initial_query_id` filter rather than as a comparison operand. This keeps every assertion non-circular and changes no row's value, only its scope. The two cell B sites are left unscoped deliberately: an explicit constant seed cannot gain a distinct element from accumulated history, and a `count() = 0` population over `randomize = 0` stays empty however many attempts run.

Cell C sets `parallel_replicas_local_plan = 0`, and on that branch the replicas receive query text rather than a serialized plan: `createRemotePlanForParallelReplicas` is reached only inside the `canUseLocalPlanForParallelReplicas` branch and only under `serialize_query_plan`, and the `else` branch passes no plan at all, so `ReadFromRemote` selects the query-text stage. The test therefore covered only the re-plan-from-text path, on every job. Add a cell that ships a serialized plan. The receiver re-optimizes it, so the join-order rewrite runs there too and must derive the same seed. `serialize_query_plan` is pinned explicitly so the cell exercises the path on every job rather than only on one whose profile enables it, and the settings are statement-level so the existing cell C rows keep measuring the query-text path.

A population filtered by the seed value cannot observe a plan construction that derived a different seed, because such rows are filtered out by definition. Cell C2 was the only cell whose sole assertion was scoped that way, so it could not fail on the seed disagreement this change is about. Scope it by initial_query_id and compare the whole observed set against the single derived value, mirroring the shape cell C already uses. One row then asserts both halves: the constructions agreed, and they agreed on the derived value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Cell C2 asserted that every plan construction of a serialized-plan query derives the same seed, but that held on the query-text fallback too: a replica that re-plans the query text derives the SAME seed from the same propagated initial_query_id, so removing the serialize_query_plan pin left the cell green and the path it exists to cover was unwitnessed. The discriminator is the number of plan constructions. A receiver that deserializes a plan constructs QueryPlanOptimizationSettings twice (executeQuery.cpp:1980 and :1987 - the second passes do_optimize = false, but the object is built at the call site, so both log the seed), while a replica re-planning the query text constructs it once (InterpreterSelectQueryAnalyzer.cpp:403). The initiator is excluded from the population because it constructs twice on both paths whenever parallel_replicas_local_plan is set, which cell C2 must pin; the guard precedes the construction (InterpreterSelectQueryAnalyzer.cpp:184 returns early for a secondary query, the construction is at :192), so the initiator is exactly the one participant that can double without deserializing. Both subqueries read the initiator's own always-visible query_log row, the same row the seed subquery already reads, so the new row needs no join against the replicas' query_log, whose flush this test cannot force. Removing the cell C2 serialize_query_plan pin now reddens this row and only this row. Measured 20 OK / 0 FAIL over 20 randomized runs and 3 OK / 0 FAIL in a reused database; the population is 4 constructions across 2 replica ids on the serialized-plan path and 2 across 2 on the query-text path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-07-30T20:36:00Z

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

Pre-publication review by an independent model (engine: codex; 16 findings across 8 review
passes; 10 fix rounds, each pass re-reviewing the full scope). The final pass returned 0
findings. Cumulative verdicts over the whole PR: 30 AGREE, 4 DISAGREE, 1 DEFERRED.

The findings below are the substantive ones. Every AGREE was fixed and re-verified; every
DISAGREE carries a recorded citation or measurement.

#	Sev	Finding	Verdict	Evidence / action
1	⚠️	The test asserted only that one seed was used, which a constant seed would also satisfy	AGREE, fixed	Added an oracle comparing the logged seed against the SQL reconstruction `greatest(sipHash64(<initial query id>), 2)`, verified empirically before being written into the test
2	⚠️	The setting doc string promised a stable join ordering, which the fix does not guarantee	AGREE, fixed	Weakened to a statement about the seed, and documented that rerunning the same text gets a new id
3	⚠️	The parallel-replicas non-vacuity guard bounded two different properties with one assertion	AGREE, fixed	Split into a plan-count assertion and a fan-out assertion, each labelled with what it measures
4	⚠️	The reorder control could be perturbed by the runner randomizing `query_plan_join_swap_table`	AGREE, fixed	Pinned the setting on that oracle only, so a forced swap cannot masquerade as a seed effect
5	⚠️	An unordered `query_log` lookup could hash a previous attempt's query id after a retry	AGREE, fixed	Both sides of every seed assertion are now scoped to the newest initiator
6	⚠️	The serialized-plan replica path was never exercised, only the query-text path	AGREE, fixed	Added a cell pinning `serialize_query_plan = 1`, plus a discriminator that fails on the query-text fallback
7	⚠️	The seed-scoped population could not observe a construction that derived a different seed	AGREE, fixed	Rescoped that assertion to the query and compared the whole observed set against the single derived value
8	⚠️	Three mutation counts stated in this description did not match the measurements	AGREE, fixed	Re-measured from the logs and corrected: the revert reddens 8 assertions, the constant hash 6
9	⚠️	An assertion on `ParallelReplicasUsedCount` would be a per-replica counter, not a fan-out count	DISAGREE	Measured 1, not 3, on a three-replica run, so the proposed oracle would have been wrong; used distinct plan-construction query ids instead
10	💡	Removing the clamp that forces the derived seed above 1 reddens no assertion	DISAGREE	Correct but not fixable: it fires only when the hash is 0 or 1, probability 2/2^64. Disclosed in the description rather than hidden

Severity: ❌ blocker / ⚠️ major / 💡 nit. DISAGREE verdicts carry recorded evidence and are
terminal per finding. Findings on hunks unchanged by a fix round are auto-dropped.

Session id: cron:clickhouse-review-slot-51:20260730-200100

groeneai · 2026-07-30T20:41:18Z

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes, three independent deterministic observables, no probabilistic claim anywhere. (1) One `clickhouse local` command, no server: one query with three scalar subqueries over the same 3-way join at `query_plan_optimize_join_order_randomize = 1` logs 2 distinct seeds. (2) Against a server, counting seeds for a single query id: 2 distinct, 5 of 5 runs. (3) Against a three-replica cluster, seeds counted across all replica query ids of one initial query id: 3 distinct, 3 of 3 runs.
b	Root cause explained?	Yes. The `= 1` sentinel was resolved with `randomSeed()` in the `QueryPlanOptimizationSettings` constructor while the session value stayed `1`. That constructor runs once per query plan construction, and one query builds several plans (scalar subqueries, `Merge` children, and one per replica since `serialize_query_plan` is off by default so every replica re-plans the query text). Each rolled its own seed; the seed feeds `getRandomizedStats`, which replaces the relation statistics, so `swap_on_sizes` picks a different leftmost relation; `findReadingStep` descends only `children.front()`, so a different table receives `requestReadingInOrder` and announces a different `CoordinationMode`; `stream_to_coordinator` is keyed by the table name alone, so the second announcement throws in `getOrCreateCoordinator`. The abort dump on the reported run contains both preconditions (`query_plan_optimize_join_order_randomize = 1`, `parallel_replicas_local_plan = false`).
c	Fix matches root cause?	Yes. The defect is that the sentinel is resolved once per plan construction instead of once per query, so the fix changes what it resolves to at that exact point rather than moving it. No band-aid: no widened bound, no `no-random-*` tag on the carrier test, no downgraded assertion, no defensive check at the throw site. The alternative of pinning eight plan-shape settings off for parallel replicas was rejected because it would delete the randomization's coverage for exactly the configuration that wants it.
d	Test intent preserved / new tests added?	Yes. No existing test weakened, retagged or removed. New stateless test `04653_join_order_randomize_seed_stable` with 13 assertion rows: 6 target assertions (per cell: the seed is unique for the query, and it EQUALS the derived value `greatest(sipHash64(<initial query id>), 2)`; plus two serialized-plan cell C2 rows: one asserting the same derived seed, and one asserting that the replicas really received a serialized plan rather than query text, by counting plan constructions per replica id), 4 non-vacuity guards (per cell, that the query really constructed more than one plan; plus, for the parallel-replicas cell, that the configured fan-out really happened, `uniqExact(query_id) = 4` over the population carrying the derived seed, which is scoped on the seed value computed from the initiator's own `query_log` row so it does not depend on when the replicas' own rows become visible) and 3 must-not-change controls (explicit seed used verbatim; two distinct explicit seeds still reorder; `= 0` derives no seed at all). The two derivation assertions exist because uniqueness alone is also satisfied by a compile-time constant seed, which would silently delete the cross-query randomization the setting exists for. The SQL oracle was verified to reproduce the C++ derivation exactly before being asserted: `sipHash64` on a `String` hashes the same bytes with no terminator (`sipHash64('ab') = sipHash64(toFixedString('ab',2))` is 1, against `toFixedString('ab',3)` it is 0) and `greatest(..., 2)` is `UInt64`, matching the C++ clamp. The observed side of six assertions is scoped to the NEWEST initiator, because the runner re-runs a failed test in a supplied `--database` without cleaning it (`tests/clickhouse-test:4619-4650`; `_cleanup` is gated `not args.database` at `:3818-3821`; `stress.py:161` supplies one), so an unscoped observed side would see one correctly-derived seed per attempt and redden permanently: measured 3 attempts -> `uniqExact(seed)` 3 unscoped vs 1 scoped, and a 3-run fixed-database arm goes from runs 2+3 FAIL to 3 OK / 0 FAIL.
e	Both directions demonstrated?	Yes, via `clickhouse-test` on Build-ID-verified binaries (pre-fix `2c3b80cd`, an in-tree revert of the fix to the merge-base; fix `64d5c772`). Pre-fix the test FAILS on all four seed assertions (both uniqueness rows and both derivation rows, `1 -> 0`), with both non-vacuity rows and all three controls still green; with the fix it passes, and randomized runs pass at 50/50 and 20/20. Four discriminating mutations. Two on `src/`, each rebuilt with a moved Build ID and then restored bit-identically: reverting the derivation to `randomSeed()` (`6936499e`) re-reddens all four seed rows; replacing the hash with a constant seed (`bf01949a`) reddens exactly the two derivation rows and nothing else, the measurement showing those rows close a hole the uniqueness rows structurally cannot see. Two on the test: reverting the newest-initiator scoping reddens exactly the four run-sensitive rows on attempts 2 and 3 of a shared database; and removing the cell C2 `serialize_query_plan` pin reddens exactly one row and nothing else, the row that counts plan constructions per replica id (a receiver that deserializes a plan constructs `QueryPlanOptimizationSettings` twice, `executeQuery.cpp:1980` and `:1987`, where a replica re-planning the query text constructs it once, `InterpreterSelectQueryAnalyzer.cpp:403`; the initiator is excluded because `parallel_replicas_local_plan`, which this cell must pin, doubles it on both paths). Measured 4 constructions across 2 replica ids on the serialized-plan path against 2 across 2 on the query-text path. On the final tree the two `src/` mutations redden 8 rows (derivation -> `randomSeed()`) and 6 rows (hash -> a constant) respectively.
f	Fix is general across code paths?	Yes, fixed at the single source of the nondeterminism, so all five passes that can call `requestReadingInOrder` (read-in-order, aggregation-in-order, distinct-in-order, limit-by-in-order, window storage-ordering reuse) are covered uniformly with no per-pass change. Verified this run rather than assumed: `tryReuseStorageOrderingForWindowFunctions` only consumes the order and seeds nothing; `ReadFromObjectStorageStep` has zero `InitialAllRangesAnnouncement` references so it announces no stream; `ReadFromMerge` is unreachable under parallel replicas (`StorageMerge` is not `isMergeTree`). Sibling sentinels of the same shape: `grep -rn "randomSeed()" src/Processors/QueryPlan/ src/Core/Settings*.cpp src/Interpreters/Context.cpp` finds only this site plus a load-balancing priority function, which is not a plan-shaping setting, so there is nothing to fold in.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	The input is one `UInt64` setting, so the meaningful matrix is its value domain, and all four regions are covered by the test: `0` (disabled, control), `1` (the sentinel, now with both a uniqueness and a derivation assertion per cell), `> 1` (explicit seed honoured verbatim, control), and the derived-value edge (the clamp forcing the value above 1). The second input axis is the plan multiplicity the seed must span, and both of its forms are covered: several plans within one process (three scalar subqueries, cell A) and several plans across processes (three replicas each re-planning, cell C, where the replicas carry distinct `query_id`s and must all hash the initial one). Type wrappers, NULL and container matrices do not apply to a scalar setting. Disclosed: the clamp fires only when the hash of the query id is 0 or 1 (probability 2/2^64), so no test can provoke it, and the mutation that removes it correctly reddens nothing.
h	Backward compatible? (maintainer-approved exception only)	Yes. No default changes: the default is `0` and the enclosing `if` only runs when the value is exactly `1`, so every query that does not opt into randomization is bit-identical. No serialization change, no protocol change, therefore no new `SettingsChangesHistory.cpp` entry is required. Verified rather than assumed: the only existing entry for this setting is the `0, 0` line from when it was added, which needs no edit. The setting is `EXPERIMENTAL` and test-only by its own doc string; the behaviour change for `= 1` users is that the seed becomes consistent across the plans of one query instead of being redrawn per plan construction. It does not make a rerun reproducible, because a rerun mints a new initial query id and therefore a new seed; the doc string says so explicitly rather than claiming reproducibility.
i	Invariants and contracts preserved?	Yes. The constructor's postcondition (the field is either `0` or a usable seed above `1`) is strengthened, not weakened: the clamp makes the derived value provably outside `{0, 1}`. `initial_query_id_` is a by-value `String` parameter and is hashed before the member assignment further down the constructor, so there is no lifetime or ordering question; the empty-id case is an explicit branch rather than an implicit hash of the empty string. No allocation, no lock, no new error path or early return is introduced, and nothing about abnormal-termination or restart re-entry applies because the value is RAM-only per-plan state. Downstream readers were enumerated: `optimizeJoin.cpp` (`effective_randomize_seed`) and `DistributedPlanExecutor.cpp`, which force-sets the field to `0` for its own reason and is unaffected.

Session id: cron:clickhouse-impl-slot-7:20260729-180650

groeneai · 2026-07-30T20:43:07Z

cc @antaljanosbenjamin @devcrafter, could you review this? query_plan_optimize_join_order_randomize = 1 resolved its "derive a seed" sentinel inside the per-plan-construction QueryPlanOptimizationSettings constructor while the session value stayed 1, so every plan of one query rolled its own seed; since the seed replaces the relation statistics via getRandomizedStats, replicas picked different join orders and announced conflicting coordination modes for one stream. The seed is now derived from initial_query_id, which already reaches replicas in ClientInfo.

clickhouse-gh · 2026-07-30T23:12:48Z

Workflow [PR], commit [ca9bfd2]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info
Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel)		FAIL
	Server died	FAIL	cidb
	Logical error: Got read request from replica A for unknown stream B. (STID: 5217-5b3d)	FAIL	cidb, issue

AI Review

Summary

This PR fixes query_plan_optimize_join_order_randomize = 1 by deriving the seed from initial_query_id, so all plan constructions for one query, including parallel-replica replans and serialized-plan receivers, agree on the same randomized join-order seed. I traced the constructor change, the initial_query_id propagation through ClientInfo, the query-text and serialized-plan replica paths, and the new regression test plus clean CI results; I did not find any remaining correctness, compatibility, or evidence gaps.

Final Verdict

Status: ✅ Approve

clickhouse-gh · 2026-07-31T07:11:30Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	86.50%	86.40%	-0.10%
Functions	91.90%	91.80%	-0.10%
Branches	78.70%	78.50%	-0.20%

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

Full report · Diff report

groeneai · 2026-07-31T16:29:50Z

CI finish ledger - `ca9bfd2`

Every failure below has an owner: a fixing PR (ours 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
Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) / `Logical error: Got read request from replica 2 for unknown stream ....inner_id....` (STID 5217-5b3d) + `Server died` (one abort, two rows)	parallel-replicas coordinator defect on the projection short-circuit path. Not caused by this pull request: its diff is `src/Core/Settings.cpp`, `QueryPlanOptimizationSettings.cpp` and one new test, none of which touches the coordinator	#111689 (merged). The tested build predates it: `compare/f4f2c3388956...ca9bfd2950c2` reports `behind_by: 1239`, so this head was built before the fix landed and a master merge will clear it

Session id: cron:our-pr-ci-monitor:20260731-160000

groeneai and others added 10 commits July 29, 2026 18:53

groeneai mentioned this pull request Jul 30, 2026

Parallelize listing of globbed s3 table function paths #107567

Open

alexey-milovidov added the can be tested Allows running workflows for external contributors label Jul 30, 2026

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jul 30, 2026

Navigation Menu

Uh oh!

Derive the join order randomization seed from the initial query id - #112660

Derive the join order randomization seed from the initial query id#112660
groeneai wants to merge 10 commits into
ClickHouse:masterfrom
groeneai:fix-6117-68e9-coordination-mode-mismatch

groeneai commented Jul 30, 2026

Uh oh!

groeneai commented Jul 30, 2026

Uh oh!

groeneai commented Jul 30, 2026

Uh oh!

groeneai commented Jul 30, 2026

Uh oh!

clickhouse-gh Bot commented Jul 30, 2026 •

edited

Loading

Uh oh!

clickhouse-gh Bot commented Jul 31, 2026

Uh oh!

groeneai commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

groeneai commented Jul 30, 2026

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Description

Uh oh!

groeneai commented Jul 30, 2026

Uh oh!

groeneai commented Jul 30, 2026

Uh oh!

groeneai commented Jul 30, 2026

Uh oh!

clickhouse-gh Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Final Verdict

Uh oh!

clickhouse-gh Bot commented Jul 31, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Jul 31, 2026

CI finish ledger - ca9bfd2

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clickhouse-gh Bot commented Jul 30, 2026 •

edited

Loading

CI finish ledger - `ca9bfd2`