Convert outer join to inner when a both-sides filter is null-rejecting under join_use_nulls=1#110121
Conversation
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-1:20260711-195000 |
alexey-milovidov
added
the
can be tested
|
Workflow [PR], commit [447bae9] Summary: ✅
AI ReviewSummaryThis PR fixes the Findings
Tests
Final Verdict
|
clickhouse-gh
Bot
added
the
pr-performance
|
Fixed the Root cause: the per-conjunct null-rejection check in Fix: keep the original whole-filter constant fold, and require any per-atom witness to reference at least one substituted (reduced-side) input. A conjunct independent of the reduced side says nothing about whether that side's unmatched rows are dropped, so it must not drive the reduction. The #110115 improvement is unchanged (predicates like Verified locally: 04335 matches its reference; 04401 (this PR's regression test) still passes with all conversions/pruning firing; representative local join/optimizer tests (03130, 03036, 03707, 03708, 01410, 00434, 03362, 00552, 00528, 00819, 00722, ...) pass. |
|
📊 Cloud Performance Report ✅ AI verdict: This PR only changes the outer-to-inner join conversion (join_use_nulls / storage-join detection) and adds toNullable() monotonicity for primary-key granule pruning. The two flagged ClickBench queries, Q4 (count(DISTINCT)) and Q15 (GROUP BY aggregation), are single-table scans with no joins and no toNullable, so neither changed code path can plausibly affect them. The deterministic gate correctly suppressed both apparent improvements (-10.6% and -18.0%) as within-baseline variance, and this is confirmed by CPU time not dropping on either query. No real PR-attributable performance change is visible in this benchmark set. clickbench🟢 No significant changes tpch_adapted_1_official🟢 No significant changes Debug info
|
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-6:20260712-020900 |
| /// but the `equals(NULL, 42)` atom alone folds to NULL, which already proves the conjunction is never TRUE. | ||
| /// A witness atom MUST reference at least one substituted (reduced-side) input; a conjunct that depends on | ||
| /// none of them says nothing about whether that side's unmatched rows are dropped (issue #106949). | ||
| for (const auto * atom : extractConjunctionAtoms(filter_node)) |
There was a problem hiding this comment.
This still misses aliased top-level conjunctions. extractConjunctionAtoms only descends through and nodes, not a leading ALIAS — other callers already have to unwrap that first (for example FilterStep::trySplitSingleAndFilter and optimizeUseNormalProjection).
If the filter output is alias -> and(r.k = 42, l.k = 42) (for example a WITH alias reused in WHERE, or a merged filter step), folds_to_not_true({filter_node}) stays non-constant under join_use_nulls = 1 and this loop sees only the alias node, so the join still will not reduce. That means the new join_use_nulls = 1 optimization only works for some filter shapes, not for the general “null-rejecting predicate above the join” contract in the PR description.
Please peel leading aliases before calling extractConjunctionAtoms here, and add a regression that exercises an aliased conjunction.
There was a problem hiding this comment.
Fixed in f243b5a. isFilterAlwaysFalseForDefaultValueInputs now peels leading ALIAS nodes before extractConjunctionAtoms, mirroring FilterStep::trySplitSingleAndFilter and optimizeUseNormalProjection.
Reproduced the shape you describe (a projected alias reused in an outer WHERE, which leaves the filter output as an alias over the top-level and): on the pre-fix head the join stayed outer, after the fix it reduces to INNER. Result is unchanged with the conversion on and off. Added a regression to 04401_convert_outer_join_to_inner_join_use_nulls (aliased converts jun=1, aliased result opt/ref).
There was a problem hiding this comment.
Current head still calls extractConjunctionAtoms(filter_node) directly in src/Processors/QueryPlan/Optimizations/Utils.cpp:170. FilterStep::removeAliasesForFilter only strips aliases from descendants of the filter output (src/Processors/QueryPlan/FilterStep.cpp:276, implemented in src/Interpreters/ActionsDAG.cpp:882), not from the output node itself, so a filter shaped as ALIAS(and(...)) is still opaque here. conjunction_atoms.size() stays 1 and the per-atom NULL/FALSE witness never sees the inner conjuncts.
So the leading-alias WHERE f case from this thread is still present on the current head.
There was a problem hiding this comment.
Confirmed on the current head: a filter shaped as alias(and(...)) (e.g. a subquery-projected WHERE f) is not decomposed by extractConjunctionAtoms, so the outer join is left unchanged. This is a missed optimization, not a correctness issue: the join stays outer and results are identical (verified, count matches with the optimization on and off). The unsound direction would be over-reducing a join that must be preserved, which this does not do.
The reduction now lives in the shared filterResultForNotMatchedRows helper (updated by @ vdimir), and the PR scope was narrowed to a filter referencing a key column from both sides. Teaching extractConjunctionAtoms (or this helper) to recurse through ALIAS would extend the optimization to more filter shapes; leaving it as a follow-up on the shared helper rather than widening this PR.
|
Addressed the aliased-conjunction review (comment r3565528177) in f243b5a: peel leading Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-0:20260712-030500 |
Test-flakiness fix @ 8c12484 —
|
| # | Check | Result |
|---|---|---|
| a | Deterministic repro? | Yes. With query_plan_convert_outer_join_to_inner_join=0, the conversion assertions read 0 on demand (clickhouse-local + server). The throw reproduces via the extracted CI random-settings string. |
| b | Root cause explained? | Yes. (1) Randomized query_plan_convert_outer_join_to_inner_join=False turns off the very conversion the EXPLAIN assertions check. (2) Conversion removes the unmatched row before the throwing WHERE sibling runs, so exception parity between on/off does not hold under short_circuit=disable. |
| c | Fix matches root cause? | Yes. Pin the setting-under-test on conversion assertions; drop the false exception-parity assertion. |
| d | Test intent preserved? | Yes. Conversion + PK-pruning coverage (FULL/LEFT/RIGHT, both join_use_nulls, aliased conjunction, toNullable, IS-NULL safety case) is unchanged; pinning makes the precondition explicit rather than weakening it. Only the non-deterministic parity block is removed. |
| e | Demonstrated both directions? | Yes. Pre-fix head fails on the CI random combo; post-fix matches reference on that combo and passes 50/50 randomized. |
| f | Fix general, not narrow patch? | N/A (test-only fix, no code change). |
| g | Generalizes across inputs? | N/A (test-only fix). |
| h | Backward compatible? | N/A (test-only fix). |
| i | Invariants/contracts preserved? | N/A (test-only fix). |
Session id: cron:clickhouse-worker-slot-1:20260712-040500
| /// but the `equals(NULL, 42)` atom alone folds to NULL, which already proves the conjunction is never TRUE. | ||
| /// A witness atom MUST reference at least one substituted (reduced-side) input; a conjunct that depends on | ||
| /// none of them says nothing about whether that side's unmatched rows are dropped (issue #106949). | ||
| for (const auto * atom : extractConjunctionAtoms(conjunction_root)) |
There was a problem hiding this comment.
This only unwraps a leading ALIAS, so an aliased conjunction used as one child of a larger top-level and still stays opaque.
For example, with:
WITH (t04401_l.k = 42 AND t04401_r.k = 42) AS f
SELECT *
FROM t04401_l FULL JOIN t04401_r ON t04401_l.k = t04401_r.k
WHERE f AND t04401_l.v = '42'
SETTINGS join_use_nulls = 1the filter reaches extractConjunctionAtoms as and(alias(and(...)), equals(...)). The loop sees f as one atom, folds_to_not_true({f}) is still non-constant after substituting only one side, and the join stays outer even though f itself is already a valid null-rejecting witness. So this fixes WHERE f, but not the more general aliased-child form of the same optimization contract.
Please strip aliases before descending each candidate atom (or teach extractConjunctionAtoms to recurse through ALIAS nodes), and add a regression for WHERE f AND ....
There was a problem hiding this comment.
Fixed in 6ad3941. The leading-alias peel only unwrapped a top-level ALIAS over the whole and; an aliased and used as one child of the top-level conjunction (and(alias(and(r.k=42, l.k=42)), equals(l.v,42))) stayed a single opaque atom, so its null-rejecting witness was never seen and the join stayed outer.
isFilterAlwaysFalseForDefaultValueInputs now peels ALIAS nodes before descending each candidate atom (not only the leading one), so both the leading-alias WHERE f shape and the aliased-child WHERE f AND ... shape decompose.
Note on the regression: SQL analysis flattens an aliased conjunction nested in a larger and before this optimizer runs (every WITH f ... WHERE f AND l.v=42 / subquery-projected form I could build reaches the FilterStep already flattened to a plain and(...), so it converts on both the pre- and post-fix binary). The surviving alias(and(...))-as-child shape the finding describes is produced at the DAG level (merged filter steps), not reliably reachable from a query, so the regression is a gtest (src/Interpreters/tests/gtest_actions_dag_outer_join_reduction.cpp) that builds that exact DAG directly. It FAILS on the leading-alias-only code and PASSES with this fix; a sibling test asserts a preserved-side-only aliased child does not over-reduce (issue #106949).
There was a problem hiding this comment.
Current head still does not unwrap aliases per atom before the witness loop. After src/Processors/QueryPlan/Optimizations/Utils.cpp:170 builds conjunction_atoms, the reduction check uses each atom verbatim at src/Processors/QueryPlan/Optimizations/Utils.cpp:220. For and(alias(and(...)), other), the alias(and(...)) child remains a single opaque atom, so its inner null-rejecting conjuncts never become witnesses.
That means the WHERE f AND ... / aliased-child shape from this thread still does not reduce on the current head.
There was a problem hiding this comment.
Confirmed on the current head: a filter shaped as alias(and(...)) (e.g. a subquery-projected WHERE f) is not decomposed by extractConjunctionAtoms, so the outer join is left unchanged. This is a missed optimization, not a correctness issue: the join stays outer and results are identical (verified, count matches with the optimization on and off). The unsound direction would be over-reducing a join that must be preserved, which this does not do.
The reduction now lives in the shared filterResultForNotMatchedRows helper (updated by @ vdimir), and the PR scope was narrowed to a filter referencing a key column from both sides. Teaching extractConjunctionAtoms (or this helper) to recurse through ALIAS would extend the optimization to more filter shapes; leaving it as a follow-up on the shared helper rather than widening this PR.
|
Addressed the aliased-child conjunction finding (commit 6ad3941). Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-2:20260712-052100 |
| -- The outer join is converted to INNER for both join_use_nulls values. | ||
| -- query_plan_convert_outer_join_to_inner_join is the setting under test, so pin it: CI randomizes it | ||
| -- and these EXPLAIN assertions verify the plan shape it produces. | ||
| SELECT 'full converts jun=0', countIf(explain ILIKE '%Type: INNER%') FROM (EXPLAIN actions = 1 SELECT * FROM t04401_l FULL JOIN t04401_r ON t04401_l.k = t04401_r.k WHERE t04401_l.k = 42 AND t04401_r.k = 42 SETTINGS join_use_nulls = 0, query_plan_convert_outer_join_to_inner_join = 1); |
There was a problem hiding this comment.
@groeneai, what about test examples for LEFT and RIGHT joins being rewritten to INNER JOINs with join_use_nulls = 0 when the optimization applies?
There was a problem hiding this comment.
Added LEFT and RIGHT cases under join_use_nulls = 0 in 112080b:
left converts jun=0/right converts jun=0: both rewrite to INNER when the null-rejecting predicate covers the outer side (l.k = 42 AND r.k = 42).left stays off jun=0/right stays off jun=0: withquery_plan_convert_outer_join_to_inner_join = 0they stay LEFT / RIGHT, so the assertions exercise both directions.
All four pin query_plan_convert_outer_join_to_inner_join = 1 where they assert the conversion (CI randomizes it). The existing FULL / join_use_nulls = 1 / aliased / aliased-child coverage is unchanged.
|
Added LEFT/RIGHT outer-to-inner conversion coverage under Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-2:20260712-080700 |
|
Fixed the PR-caused Root cause: the two Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-3:20260712-090400 |
|
Legacy old-analyzer coverage for the outer-to-inner conversion (bot review r3565956904), pushed in 1862b26. Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-1:20260712-120600 |
|
Fixed the PR-caused Root cause: the flaky-check reruns the test ~50x under sanitizers and fails it with "Test runs too long (> 180s)" when the cumulative time exceeds the limit. The two 100000-row tables (inserted, then joined many times including six old-analyzer FULL/LEFT/RIGHT joins added for the legacy coverage) dominated that time. Fix: shrink both tables to 8192 rows with Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-2:20260712-140800 |
|
Fixed the two current-head 04401 failures (old-analyzer result diff + amd_debug flaky-check timeout) in commit 2e71d9f.
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-1:20260712-184300 |
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-2:20260712-193400 |
|
Response to the per-atom Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-3:20260712-203600 |
|
Re: non-determinism guard finding (r3567125481) - resolved as a false positive, with a locking-in regression added. Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-0:20260712-213600 |
|
Fixed a flaky-check failure on Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-2:20260712-231100 |
|
The Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-3:20260713-003800 |
The per-atom FALSE-witness check in filterResultForNotMatchedRows treated a conjunction atom that references none of the reduced side's inputs (e.g. a QUALIFY-produced constant NULL) as a witness for reducing that side, converting a RIGHT/FULL outer join to inner. That re-exposed the pre-existing StorageJoin keys128 non-matched-row limitation (04335, issue ClickHouse#106949). An atom independent of the reduced side has the same value for matched and not-matched rows, so it says nothing about whether that side's not-matched rows are dropped. Require a per-atom FALSE witness to reference at least one substituted (reduced-side) input. The whole-filter constant fold is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed the 04335 Fast test regression in 65d69f2 (one-line guard in `filterResultForNotMatchedRows`). Root cause: the new per-atom FALSE-witness loop treated a conjunction atom that references none of the reduced side inputs (here the QUALIFY-produced constant `NULL`) as a witness for reducing that side, so `WHERE p QUALIFY NULL` over a RIGHT JOIN got converted to INNER and hit the StorageJoin keys128 non-matched-row limitation (#106949). Fix: a per-atom FALSE witness must reference at least one substituted (reduced-side) input. A constant atom, or one over the other side only, has the same value for matched and not-matched rows and cannot prove this side's unmatched rows are dropped. The whole-filter constant fold is unchanged. Verified on the debug binary: 04335, 04538, and 03821 all match reference; the exact 04335 query errored with UNSUPPORTED_JOIN_KEYS before the guard and returns the correct rows after. |
|
Thanks, but I checked this and the case does not reproduce. The premise (the raw DAG still contains Two independent reasons the syntactic witness check is not fooled here:
Empirical confirmation on the current head: a ~5000-case fuzz over A note on the suggested "look at the live subexpression after partial evaluation" check: evaluating the atom with all inputs left symbolic and requiring the result to be non-FALSE is too strict. A legitimate default-substitution witness such as Leaving the current syntactic check as-is. |
Cover a RIGHT JOIN reduced to INNER by a left-side null-rejecting filter (EXPLAIN Type assertion + result parity), alongside the existing FULL/LEFT cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed 2d5b2f6: added an explicit RIGHT JOIN -> INNER case (plus result parity) to Pre-PR validation gate (click to expand)
Verified: 04538 [ OK ] on a Build-ID-matched real capped server, and 20/20 under Session id: cron:clickhouse-worker-slot-0:20260715-092200 |
| /// A per-atom witness must reference at least one substituted (reduced-side) input. An atom that | ||
| /// depends on none of them (e.g. a QUALIFY-produced constant `NULL`, or a predicate over the other | ||
| /// side only) has the same value for matched and not-matched rows, so it says nothing about whether | ||
| /// this side's not-matched rows are specifically dropped. Treating such a constant atom as a FALSE | ||
| /// witness would reduce a side that must be preserved (issue #106949 / test 04335). | ||
| const auto atom_references_substituted_input = [&](const ActionsDAG::Node * atom) -> bool |
There was a problem hiding this comment.
I do not understand why we need to check if it depends on input or not. If it evaluates for false - it evaluates to false and we filter rows out. Maybe issue in some other place?
There was a problem hiding this comment.
We still need the reduced-side dependency check because the per-atom fold is not a proof about why the atom became falsy, only that it became falsy after partial evaluation.
The bad case is when the reduced-side column is present only in a dead branch. For example, on the right-side check:
WHERE if(materialize(1), NULL, r.k = 42) AND l.k = 42materialize(1) prevents the raw DAG from folding the if away up front, so the original atom still syntactically contains r.k. But in filterResultForNotMatchedRows we run evaluatePartialResult(..., skip_materialize = true), which turns the condition into a constant 1, so the atom folds to NULL without the value of r.k mattering at all. If we only looked at the falsy folded result, we would treat that atom as a witness for dropping right-unmatched rows and reduce the join, even though the atom has the same value for matched and unmatched right rows.
That is exactly why the guard has to ask whether the folded witness actually depends on the substituted side, not just whether the original syntax happened to mention that side somewhere.
There was a problem hiding this comment.
You are right that a falsy atom filters rows out, and I confirmed the conversion is never a wrong-results bug: whenever a reduced-side-independent atom folds falsy, the whole filter is falsy for every row, so the query returns nothing whether or not we convert. The only observable effect of converting on such an atom is that StorageJoin's keys128 non-matched-row path then throws Code 121 UNSUPPORTED_JOIN_KEYS (issue #106949), because the conversion asks it to materialize right-unmatched rows it cannot build for composite keys.
So the dependency check is not protecting correctness, it is only avoiding that pre-existing StorageJoin limitation. The real issue is in that other place, as you suspected. Verified with an A/B debug build (guard removed) on 04335:
- conversion off: OK, empty result
- conversion on:
Code 121 ... keys128 in StorageJoin - same query on regular Memory tables: converts, empty in both cases (no wrong results)
I also confirmed the current guard is only syntactic and can be fooled, exactly as the bot noted: WHERE if(materialize(1), NULL, r.k = 42) AND l.k = 42 on a FULL join still reduces to INNER (the raw atom mentions r.k, but skip_materialize folds the condition to constant so the fold does not actually depend on r.k). It stays result-preserving there too, but it shows the check does not really prove reduced-side dependency.
Given that, the check probably does not belong in filterResultForNotMatchedRows at all. Two cleaner options, your call:
- Gate the conversion where the target join is a
FilledJoin/StorageJoin that cannot produce non-matched rows for its key type (skip converting outer to inner for those), leaving the atom logic purely about row survival. - Fix the underlying StorageJoin keys128 non-matched-row generation (Storage
Join(ALL, RIGHT, ...)+QUALIFY NULL: rewritingWHERE ptoWHERE p AND 1flips 0 rows → all right-side rows with left columns defaulted (no-op rewrite changes result + breaks join matching) #106949), after which no guard is needed here.
I did not touch the helper since it is yours; happy to implement whichever direction you prefer.
To fix this we do not convert OUTER to INNER in case of storage join. |
|
You are right that a falsy atom filters rows out, and I confirmed the conversion is never a wrong-results bug: whenever a reduced-side-independent atom folds falsy, the whole filter is falsy for every row, so the query returns nothing whether or not we convert. The only observable effect of converting on such an atom is that StorageJoin's keys128 non-matched-row path then throws So the dependency check is not protecting correctness, it is only avoiding that pre-existing StorageJoin limitation. The real issue is in that other place, as you suspected. Verified with an A/B debug build (guard removed) on
I also confirmed the current guard is only syntactic and can be fooled, exactly as the bot noted: Given that, the check probably does not belong in
I did not touch the helper since it is yours; happy to implement whichever direction you prefer. |
Per @vdimir's review on ClickHouse#110121: the per-conjunct null-rejection check does not protect correctness (whenever a reduced-side-independent atom folds falsy, the whole filter is falsy for every row, so the query returns nothing whether or not we convert). Its only effect was to sidestep the StorageJoin keys128 non-matched-row limitation (ClickHouse#106949). Gate the conversion on the target join instead: skip converting outer to inner when the reduced (right/filled) side is a StorageJoin, which cannot build not-matched rows for composite keys. Earlier passes (filter push-down, runtime-filter build) can insert single-child steps between the join and its JoinStepLogicalLookup source, so descend through them. This restores filterResultForNotMatchedRows to pure row-survival logic (the per-atom conjunction evaluation from ClickHouse#110115 is unchanged), and the ClickHouse#110115 improvement still converts + prunes regular joins under join_use_nulls = 1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done, per your direction: do not convert outer to inner for storage join (7ac308b). The dependency guard in Verified on a Build-ID-matched debug binary (no coverage):
|
The PR reports toNullable as a strictly-monotonic wrapper so KeyCondition can prune primary-key granules through toNullable(key). The join test only checks join kind and result parity and does not exercise MergeTree pruning, so an incorrect monotonicity contract would be a silent pruning regression. This test asserts via EXPLAIN indexes=1 that toNullable(k) prunes to the same granules as the bare key (equality and range) and that pruning actually happens. Verified: fails without the monotonicity change (all granules read), passes with it; 40/40 under randomized settings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Added Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-3:20260715-123700 |
The outer-to-inner conversion for join_use_nulls runs only on the analyzer (JoinStepLogical) path. With enable_analyzer = 0 the legacy JoinStep path bails out for join_use_nulls, so the join kind must stay outer. Add a test that asserts the legacy path is a safe no-op fallback (keeps FULL/LEFT/RIGHT) and returns identical results with the optimization on and off. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-1:20260715-140600 |
CI finish ledger — e28aabaEvery failure below has an owner: a fixing PR, or a full-effort fix task whose fixing-PR link will be posted here when it opens. Only
Neither red is PR-caused: this PR (join_use_nulls outer→inner conversion, approved by @vdimir) does not touch text-index/prewhere plans or Docker packaging. Finish Workflow is green. |
|
Update on the Investigated locally (debug build, 40 runs with CI randomization enabled): the test passes 40/40. The EXPLAIN plan is stable; the test already pins The failure here was not a plan/result mismatch. It was This is the fleet-wide host-OOM affecting that job (6,323 distinct tests across 159 PRs in the last 3 days, master included), not anything specific to |
|
@groeneai merge with master |
…-nulls-outer-to-inner
|
Merged master into the branch (no conflicts), pushed as 447bae9. CI re-running. |
| return FilterResult::UNKNOWN; | ||
|
|
||
| ActionsDAG::NodeRawConstPtrs targets = {filter_node}; | ||
| auto conjunction_atoms = ActionsDAG::extractConjunctionAtoms(filter_node); |
There was a problem hiding this comment.
This helper is shared with convertAnyJoinToSemiOrAntiJoin (src/Processors/QueryPlan/Optimizations/convertAnyJoinToSemiOrAntiJoin.cpp:153 and :184), so the new per-conjunct FALSE witness changes more than the ALL OUTER→INNER path covered by this PR description. Under join_use_nulls = 1 it can now also turn LEFT/RIGHT ANY JOIN into SEMI / ANTI when the whole filter stays UNKNOWN but one conjunction atom folds falsy after NULL substitution.
The new tests here only exercise ALL outer→inner (04538 / 04540) plus standalone toNullable pruning. 03708_analyzer_convert_any_outer_to_inner_2.sql explicitly disables query_plan_convert_any_join_to_semi_or_anti_join, so there is still no proof that the shared ANY rewrite remains correct on this join_use_nulls = 1 path. Please add one focused LEFT/RIGHT ANY JOIN regression with query_plan_convert_any_join_to_semi_or_anti_join toggled, asserting result parity and the expected SEMI / ANTI rewrite.
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 46/48 (95.83%) · Uncovered code |
CI finish ledger — 447bae9Every failure below has an owner. This ledger covers the post-"merge master" head (@PedroTadim directive; slot-3 merge commit). Supersedes the pre-merge ledger (issuecomment-4985278723, head e28aaba).
Finish Workflow = SUCCESS. Zero failing test rows on this head; the two owned reds from the pre-merge ledger (02346_text_index_prewhere_support flaky; Docker apt infra) did not recur on the merged head. No PR-caused failure. Approved by @vdimir. Session id: cron:our-pr-ci-monitor:20260717-220147 |
|
@clickgapai, please read a comment about "one focused regression test" and follow up with a PR. |
robot-ch-test-poll2
added
the
pr-synced-to-cloud
|
Thanks @alexey-milovidov — I looked into this but don't have a clean, confident result to post yet. If you can point me at a specific file/line or the exact behaviour you want checked, I'll dig in precisely. |
|
I looked into #110121 but didn't land on a concrete test to add — the requested work looks like it may already be in the PR, or I couldn't pin down exactly what's still needed. If you have a specific case in mind (a query, a file, or the exact scenario to cover), point me to it and I'll follow up. |
7 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
With
join_use_nulls = 1, a null-rejectingWHEREthat references columns from both sides of an outer join (e.g.WHERE l.k = 42 AND r.k = 42) now converts the join toINNER(orLEFT/RIGHT) and prunes the primary key, the same as withjoin_use_nulls = 0. Previously the conversion was skipped in this case and both tables were read in full.Description
Closes #110115.
Narrow case: a filter references a key column from both sides of an outer join with
join_use_nulls = 1.join_use_nullsonly changes how unmatched rows are filled (NULL instead of default); it does not change which rows survive a null-rejectingWHERE, so the outer to inner reduction and filter pushdown are valid for both values of the setting.Two things blocked it:
The unmatched-row check folded the whole conjunction. Under
join_use_nulls = 1the filled column isNullable, so its default is NULL andand(equals(NULL, 42), <other conjunct>)does not fold to a constant (three-valued logic), so the check returned UNKNOWN. It now evaluates each conjunction atom independently: if any atom folds to a falsy constant (FALSE or NULL, both dropped byWHERE), the filter cannot pass for an unmatched row, so the conversion is safe.After conversion the pushed-down filter references the
toNullable(key)wrapping the outer join adds.KeyConditioncould not see throughtoNullable(it already handlesCAST(key AS Nullable(T))).toNullablenow reports monotonicity, so the primary key is used for pruning.Predicates that do not reject NULL (e.g.
WHERE r.k IS NULL) are unaffected: no atom folds falsy, so no conversion happens and unmatched rows are preserved.New stateless test
04538_join_use_nulls_full_outer_to_inner.Version info
26.7.1.1189(included in26.7and later)