iframe-proxy

vdimir · 2026-03-02T18:37:11Z

Changelog category (leave one):

Improvement

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

Added the query_plan_merge_expression_into_join setting to allow merging Expression steps into JOIN steps during join reordering optimization. This enables join reordering across subqueries that wrap joins (e.g., when a JOIN is inside a subquery with computed columns), leading to better optimization of complex join trees.

clickhouse-gh · 2026-03-02T18:37:53Z

Workflow [PR], commit [0883b25]

Summary: ✅

Performance Comparison: Performance dashboard

AI Review

Summary

This PR adds query_plan_merge_expression_into_join so join reordering can flatten non-trivial ExpressionStep wrappers around child joins, plus follow-up fixes for outer-join predicate handling. The remaining issues are all in that new flattening path: it can still move row-sensitive expressions across join boundaries, it reclassifies final constants as pre-join left expressions, and the duplicate-column preflight no longer matches the actual flattening rule. I would not merge this as-is.

Missing context / blind spots

⚠️ I could not run a local clickhouse repro because the workspace only has the PR merge checkout and no built server/client binary. A quick local repro of the constant-only RIGHT JOIN and stateful-wrapper cases would close that gap, but the findings below follow directly from the current PR-head code.

Findings

❌ Blockers

[src/Processors/QueryPlan/Optimizations/optimizeJoin.cpp:707-745] addChildQueryGraph now flattens any non-ARRAY JOIN ExpressionStep when query_plan_merge_expression_into_join = 1. That lets stateful/cardinality-sensitive nodes such as rand or rowNumberInBlock be evaluated before a different join and at a different row multiplicity than in the original plan, which changes both join matching and returned values. Keep such wrappers unflattened unless the DAG is proven safe to move; at minimum reject wrappers with stateful or row-sensitive expressions.
[src/Processors/QueryPlan/Optimizations/optimizeJoin.cpp:1316-1320], [src/Processors/QueryPlan/JoinStepLogical.cpp:1404-1416] Final COLUMN outputs are appended to actions_after_join, so they become fromNone nodes in left_dag instead of staying in the post-join projection. For RIGHT and FULL joins that makes constants get default/null-extended with the left side instead of preserving their literal value, and for constant-only projections the empty-used_columns fallback can now pick __join_result_dummy or another constant and throw NOT_FOUND_COLUMN_IN_BLOCK in TableJoin::setUsedColumns. Keep final constants out of the pre-join DAG and use a guaranteed real input column only as the row-count keeper.
[src/Processors/QueryPlan/Optimizations/optimizeJoin.cpp:703-745], [src/Processors/QueryPlan/Optimizations/optimizeJoin.cpp:1427-1441] joinGraphHasOverlappingColumnNames is meant to reject duplicate-name join graphs before reconstruction, but its header collector no longer mirrors the new flattening rule. addChildQueryGraph flattens non-trivial expression-wrapped child joins when query_plan_merge_expression_into_join = 1, while collectJoinGraphRelationHeaders only peels trivial passthrough wrappers. A wrapper that aliases away duplicate child column names can therefore pass the preflight and still hit the duplicate-name LOGICAL_ERROR during join reconstruction. Make the collector use the same expression-wrapper handling as addChildQueryGraph.

⚠️ Majors

[src/Processors/QueryPlan/Optimizations/optimizeJoin.cpp:818-823] The get_exposed_columns helper still unwraps ExpressionStep with check = check->children[0] but without the children.size() == 1 guard that the main flattening path already uses. If the optimizer sees an intermediate or malformed unary node here, this is unchecked UB instead of a clean exception. Reuse the same arity guard as addChildQueryGraph.
[tests/queries/0_stateless/04001_join_reorder_through_expression.sql:24-63] The new setting is still only exercised with query_plan_merge_expression_into_join = 1; there is no matching plan-and-result case with the setting forced to 0 on the same query shape. Since this PR introduces a switchable behavior, the disabled branch should be locked down too.

Final Verdict

❌ Request changes. The new expression-flattening path still has correctness bugs and one reopened safety bug, and the disabled-setting branch is not covered by a companion regression test.

clickhouse-gh · 2026-03-11T13:55:34Z

+
+SET enable_parallel_replicas = 0;
+SET query_plan_join_swap_table = 'auto';
+SET query_plan_optimize_join_order_limit = 64;


Nice coverage for the enabled path. To reduce regression risk for the new switch, please add one companion case with query_plan_merge_expression_into_join = 0 on the same query shape (plan + result), so both enabled and disabled semantics are locked by tests.

I re-checked the current 04001 test and still don't see a companion case with query_plan_merge_expression_into_join = 0 for the same query shape.

Since this PR introduces a switchable behavior, we should lock both branches (1 and 0) with plan+result checks to prevent regressions where the disabled path drifts silently.

antaljanosbenjamin · 2026-04-01T15:08:08Z

+            {
+                bool merge_expression_into_join = query_graph.context->optimization_settings.merge_expression_into_join;
+                auto * expr = typeid_cast<ExpressionStep *>(check->step.get());
+                if (expr && check->children.size() == 1 && !expr->getExpression().hasArrayJoin()


I think checking that it has a single child is unnecessary and confusing, as ExpressionStep can only have one child, if I am not mistaken. Or do I miss something here?

Suggested change

if (expr && check->children.size() == 1 && !expr->getExpression().hasArrayJoin()

if (expr && !expr->getExpression().hasArrayJoin()

ExpressionStep is logically unary, but in this optimizer path we dereference node->children[0] and may run before all structural assumptions are guaranteed. Keeping node->children.size() == 1 here is a defensive guard against malformed/intermediate plan nodes and avoids an avoidable exception in this hot path.

clickhouse-gh · 2026-04-02T08:18:56Z

+            /// have the correct row count. When all post-join outputs are constants
+            /// (e.g. `__join_result_dummy`), residual_dag has no required inputs,
+            /// and the join would produce blocks with 0 columns
+            if (!left_dag.getOutputs().empty())


If actions_after_join contains only COLUMN constants (e.g. SELECT 1 FROM t1 CROSS JOIN t2 after reorder), residual_dag.getRequiredColumnsNames() is empty and this fallback takes left_dag.getOutputs().front()->result_name. For that shape, the first output can be __join_result_dummy (a fromNone constant), which is not present in left/right input columns. Then TableJoin::setUsedColumns throws NOT_FOUND_COLUMN_IN_BLOCK instead of executing the query.

Please pick a guaranteed real input column (from left_dag.getInputs()/right_dag.getInputs() or child headers) rather than an arbitrary output node.

This still looks live in the current code.

chooseJoinOrder now explicitly appends COLUMN constants (for example __join_result_dummy) to actions_after_join at optimizeJoin.cpp:1263-1267. In buildPhysicalJoinImpl, when residual_dag.getRequiredColumnsNames() is empty, the fallback still picks left_dag.getOutputs().front()->result_name (JoinStepLogical.cpp:1295-1297). Because used_expressions is seeded from actions_after_join first (JoinStepLogical.cpp:1207-1221), that front output can be the fromNone constant instead of a real input column.

Then TableJoin::setUsedColumns receives a name that is not present in either side and throws NOT_FOUND_COLUMN_IN_BLOCK.

Could we change this fallback to take a guaranteed real input from left_dag.getInputs() / right_dag.getInputs() (or child headers), and skip COLUMN nodes here?

alexey-milovidov · 2026-04-07T00:22:36Z

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

alexey-milovidov · 2026-04-09T03:28:16Z

The MSan stress test failure (MemorySanitizer: use-of-uninitialized-value, STID 4179-5154 or 4148-3044) is a known pre-existing issue unrelated to this PR. Fix: #102158

clickhouse-gh · 2026-05-27T13:56:24Z

+                if (expr && !expr->getExpression().hasArrayJoin()
+                    && (isPassthroughActions(expr->getExpression()) || merge_expression_into_join))
+                {
+                    check = check->children[0];


This branch unwraps ExpressionStep with check = check->children[0], but unlike addChildQueryGraph above it does not guard check->children.size() == 1 first.

In this optimizer path we can still see intermediate/malformed nodes, and operator[] here is unchecked. A zero-child ExpressionStep would trigger undefined behavior before we fail with a proper exception.

Please add the same defensive arity check used in addChildQueryGraph before dereferencing children[0].

clickhouse-gh · 2026-06-24T11:55:32Z

📊 Cloud Performance Report

✅ AI verdict: no_change — no significant changes across 38 queries analysed

This PR adds a new join-reordering optimization (query_plan_merge_expression_into_join) that merges Expression steps into JOIN steps and reworks how outer-join ON-clause predicates are pinned during reorder. Both flagged queries are ClickBench Q4 and Q15, which run against a single table with no joins, so the changed code path is never exercised by them. The 18% and 17% improvements are large and consistent in the measurements but cannot plausibly be caused by a join-only optimizer change, so they are downgraded as run-to-run variance rather than real PR effects.

clickbench

⚠️ 2 inconclusive

Flagged queries (2 of 43)

	Query	Verdict	Baseline median (ms)	PR median (ms)	Change	q-value	Hint
⚠️	4	not_sure	263	214	-18.4%	<0.0001	aggregation: ClickBench Q4 is single-table, no joins; PR only changes join reordering/expression-merge. Off-path, treat as variance.
⚠️	15	not_sure	245	204	-16.7%	<0.0001	aggregation: ClickBench Q15 is single-table, no joins; PR only touches the join optimizer. Off-path, treat as variance.

_{q-value = BH-FDR adjusted p; smaller is stronger evidence. MIRAI flags a query when q < fdr_q (default 0.10) — the value the verdict is based on.}

tpch_adapted_1_official

🟢 No significant changes

Debug info

StressHouse run: 8b257768-7c2d-4e10-a0d0-42674affb3b3
MIRAI run: bc0cf70c-6cd1-465a-a489-2a5ebb864a58
PR check IDs:
- clickbench_411604_1782763968
- clickbench_411621_1782763968
- tpch_adapted_1_official_411628_1782763968
- tpch_adapted_1_official_411651_1782763968

clickhouse-gh · 2026-06-24T13:29:45Z

+                    /// Include COLUMN Const nodes (like `__join_result_dummy`) in actions_after_join.
+                    /// These are `fromNone` and will be placed in left_dag, ensuring the left pre-join
+                    /// always produces at least one output column (so blocks have correct row count).
+                    if (mapped_it->second->type == ActionsDAG::ActionType::COLUMN)


This makes final COLUMN constants part of actions_after_join, so buildPhysicalJoinImpl treats them as left-side inputs (fromNone goes into left_dag). For RIGHT/FULL joins, unmatched rows are produced by default/null-extending the left side, so a query like SELECT 1 FROM l RIGHT JOIN r ... can return the default/null-extended value for the constant instead of evaluating 1 after the join.

Please keep final constants in the post-join projection/residual DAG and use a guaranteed real input column only for the row-count keeper column.

If we do not put predicate into query_graph.outer_join_conditions it automatically goes into residual filters then

vdimir · 2026-07-02T10:08:13Z

@groeneai check out fuzzer and stress test issues and create repro if it's new ones

AST fuzzer (amd_debug, targeted, old_compatibility) FAIL
Logical error: 'Inconsistent AST formatting: the original AST: (STID: 1941-1bfa) FAIL cidb
Stress test (arm_debug) FAIL
Logical error: '!part.empty()' (STID: 2508-2ef2) FAIL cidb
AST fuzzer (amd_msan) FAIL
Logical error: Block structure mismatch in A stream: different columns: (STID: 0993-27f0) FAIL cidb, issue

groeneai · 2026-07-02T10:53:47Z

@vdimir Triaged all three. All are pre-existing trunk failures, none caused by the expression-merge / join-reordering change, so no new repro is needed. Evidence is 30d CIDB (cross-PR + master) + the exact crash sites.

1. STID 1941-1bfa Inconsistent AST formatting (AST fuzzer amd_debug, targeted, old_compatibility)
Known chronic formatter/parser round-trip family, unrelated to join reordering. 30d: 89 hits across 74 distinct PRs + 8 on master, still firing today (last seen 2026-07-02). Prior exact-STID reports: #106850, #106358 (closed).
Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=98533&sha=d44ac9b1a5a8142aeee52375ffbc6863960233ba&name_0=PR&name_1=AST%20fuzzer%20%28amd_debug%2C%20targeted%2C%20old_compatibility%29

2. STID 2508-2ef2 !part.empty() (Stress test arm_debug)
Known latent engine bug, not join-reordering related. ASTIdentifier ctor asserts on an empty name-part when a backquoted dot-terminated identifier (a lambda arg like `__table1.`) is converted QueryTree -> AST while building an error message. Crash site: ASTIdentifier.cpp:48 <- IdentifierNode::toASTImpl <- LambdaNode::toASTImpl. 30d: 2 unrelated branches, 0 on master; the identical stack reproduces on an unrelated branch too. Fix in flight: #108735 (LambdaNode.cpp, which you approved). It is still open, so this run predates it, which is why the assert still fires here.
Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=98533&sha=d44ac9b1a5a8142aeee52375ffbc6863960233ba&name_0=PR&name_1=Stress%20test%20%28arm_debug%29

3. STID 0993-27f0 Block structure mismatch in A stream (AST fuzzer amd_msan)
The issue you already linked, #108142: a chronic UnionStep/IntersectOrExceptStep bug where one branch constant-folds and the common header diverges. 30d: 174 hits across 135 distinct PRs + 13 on master, last seen 2026-07-02. Fix in flight: #107719.
Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=98533&sha=d44ac9b1a5a8142aeee52375ffbc6863960233ba&name_0=PR&name_1=AST%20fuzzer%20%28amd_msan%29

clickhouse-gh · 2026-07-03T13:41:44Z

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 269/279 (96.42%) · Uncovered code

Full report · Diff report

clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Mar 2, 2026

vdimir force-pushed the vdimir/join_reorder_through_expression branch from 1ccd131 to 7cba228 Compare March 11, 2026 10:12

clickhouse-gh Bot reviewed Mar 11, 2026

View reviewed changes

Comment thread src/Processors/QueryPlan/Optimizations/optimizeJoin.cpp Outdated

vdimir force-pushed the vdimir/join_reorder_through_expression branch from 7cba228 to 19e7334 Compare March 11, 2026 13:40

clickhouse-gh Bot reviewed Mar 11, 2026

View reviewed changes

Comment thread src/Processors/QueryPlan/Optimizations/optimizeJoin.cpp

vdimir force-pushed the vdimir/join_reorder_through_expression branch from 19e7334 to 16d7279 Compare March 11, 2026 13:48

clickhouse-gh Bot reviewed Mar 11, 2026

View reviewed changes

vdimir force-pushed the vdimir/join_reorder_through_expression branch from c3d87b4 to 6e16799 Compare March 11, 2026 14:48

clickhouse-gh Bot reviewed Mar 11, 2026

View reviewed changes

Comment thread src/Processors/QueryPlan/JoinStepLogical.cpp Outdated

vdimir force-pushed the vdimir/join_reorder_through_expression branch from f6bfae3 to 73d2ea8 Compare March 16, 2026 11:53

clickhouse-gh Bot reviewed Mar 23, 2026

View reviewed changes

Comment thread src/Interpreters/InterpreterExplainQuery.cpp Outdated

vdimir commented Mar 24, 2026

View reviewed changes

Comment thread tests/queries/0_stateless/04001_join_reorder_through_expression.sql

vdimir changed the title ~~wip merge expression into join~~ Merge expressions into join during join reordering Mar 24, 2026

vdimir marked this pull request as ready for review March 24, 2026 16:08

antaljanosbenjamin self-assigned this Mar 27, 2026

vdimir force-pushed the vdimir/join_reorder_through_expression branch from 286d053 to 430261c Compare April 1, 2026 11:43

vdimir requested a review from antaljanosbenjamin April 1, 2026 11:44

antaljanosbenjamin approved these changes Apr 1, 2026

View reviewed changes

clickhouse-gh Bot reviewed Apr 2, 2026

View reviewed changes

vdimir force-pushed the vdimir/join_reorder_through_expression branch 2 times, most recently from 3658658 to 78fbe40 Compare May 26, 2026 14:49

clickhouse-gh Bot reviewed May 27, 2026

View reviewed changes

clickhouse-gh Bot assigned antaljanosbenjamin and unassigned antaljanosbenjamin Jun 1, 2026

ClickHouse deleted a comment from clickhouse-gh Bot Jun 1, 2026

clickhouse-gh Bot reviewed Jun 1, 2026

View reviewed changes

Comment thread src/Core/SettingsChangesHistory.cpp Outdated

clickhouse-gh Bot reviewed Jun 1, 2026

View reviewed changes

Comment thread tests/queries/0_stateless/04001_join_reorder_through_expression.sql

Merge branch 'master' into vdimir/join_reorder_through_expression

304dacb

vdimir commented Jun 22, 2026

View reviewed changes

Comment thread tests/queries/0_stateless/04001_join_reorder_through_expression.sql

vdimir added 2 commits June 22, 2026 16:38

add order by to 04001_join_reorder_through_expression

b90be24

long 04277_join_reorder_constraints

cd93a68

clickhouse-gh Bot reviewed Jun 22, 2026

View reviewed changes

Comment thread src/Processors/QueryPlan/Optimizations/joinOrder.cpp

fix cross join with left and transitive predicates

495abf1

vdimir added 2 commits June 24, 2026 13:20

Merge branch 'master' into vdimir/join_reorder_through_expression

d801a53

move query_plan_merge_expression_into_join to 26.7 section

49aaa6b

clickhouse-gh Bot reviewed Jun 24, 2026

View reviewed changes

vdimir added 2 commits June 29, 2026 13:06

Merge branch 'master' into vdimir/join_reorder_through_expression

46e8630

Fix build after merge

0c5bc5e

vdimir commented Jun 30, 2026

View reviewed changes

Comment thread tests/queries/0_stateless/04001_join_reorder_through_expression.sql

vdimir added 2 commits June 30, 2026 10:38

Fix 04001_join_reorder_through_expression

740bf44

Merge branch 'master' into vdimir/join_reorder_through_expression

ee89bd7

vdimir enabled auto-merge June 30, 2026 16:49

vdimir added this pull request to the merge queue Jun 30, 2026

github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Jun 30, 2026

vdimir enabled auto-merge July 1, 2026 08:08

Merge branch 'master' into vdimir/join_reorder_through_expression

b92f4dc

vdimir disabled auto-merge July 1, 2026 12:46

vdimir added 2 commits July 1, 2026 12:47

Merge branch 'master' into vdimir/join_reorder_through_expression

d4936d3

fix after dpsub merge

d44ac9b

alexey-milovidov mentioned this pull request Jul 1, 2026

Support max_insert_threads for plain INSERTs #109000

Open

1 task

Merge branch 'master' into vdimir/join_reorder_through_expression

3507cbb

vdimir enabled auto-merge July 3, 2026 10:15

Merge branch 'master' into vdimir/join_reorder_through_expression

0883b25

	if (expr && check->children.size() == 1 && !expr->getExpression().hasArrayJoin()
	if (expr && !expr->getExpression().hasArrayJoin()

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

vdimir commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Uh oh!

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

Uh oh!

AI Review

Summary

Missing context / blind spots

Findings

Final Verdict

Uh oh!

Uh oh!

Uh oh!

clickhouse-gh Bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot May 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antaljanosbenjamin Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot May 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

clickhouse-gh Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot May 15, 2026

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Apr 7, 2026

Uh oh!

alexey-milovidov commented Apr 9, 2026

Uh oh!

clickhouse-gh Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

clickbench

tpch_adapted_1_official

Uh oh!

clickhouse-gh Bot Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

vdimir Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vdimir commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

groeneai commented Jul 2, 2026

Uh oh!

clickhouse-gh Bot commented Jul 3, 2026

LLVM Coverage Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

vdimir commented Mar 2, 2026 •

edited

Loading

clickhouse-gh Bot commented Mar 2, 2026 •

edited

Loading

clickhouse-gh Bot commented Jun 24, 2026 •

edited

Loading

vdimir commented Jul 2, 2026 •

edited

Loading