test: characterize correlated NOT IN null-aware behaviour - #25558
Merged
Merged
Conversation
Adds sqllogictest and benchmark coverage for correlated `NOT IN`, pinned to what DataFusion does today. Several of these expectations are wrong, and a few shapes do not plan at all; each such block carries a note and a link to apache#25336. The fixes flip them, so the flip is visible in those diffs rather than buried in a large change. sqllogictest, in `null_aware_anti_join.slt` and `null_aware_mark_join.slt`: - correlated `NOT IN` with a non-equality correlation, which stays a residual join filter; - a correlation that names only outer columns, so it cannot become an equi-join key; - a constant value expression with and without a correlation; - a subquery inside the `IN` value, both spellings of the outer `IN`; - `IS NOT NULL` over a subquery predicate and a comparison between two marks, the contexts that can tell a NULL mark from a FALSE mark; - plan pins for the joins the planner chooses in each case. Benchmarks: - Q09, a correlated non-negated `IN`. It must not use a null-aware join, a direction none of Q01-Q08 covers, and it passes today; - correctness canaries on Q05-Q08, each comparing the `NOT IN` result with a reference that does not use `NOT IN`. All four currently disagree, so they are pinned to `false`. Expected results verified with DuckDB and PostgreSQL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xudong963
approved these changes
Sep 21, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25558 +/- ##
==========================================
- Coverage 82.42% 82.41% -0.01%
==========================================
Files 1138 1138
Lines 435429 435429
Branches 435429 435429
==========================================
- Hits 358889 358875 -14
- Misses 54839 54848 +9
- Partials 21701 21706 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Thanks @xudong963 |
adriangb
added a commit
to pydantic/datafusion
that referenced
this pull request
Sep 23, 2026
…results (apache#25339) ## Which issue does this PR close? - Closes apache#25336. > [!NOTE] > **This PR now holds only the executor fix.** It used to hold the executor fix, the optimizer fix, and their tests together. To make review easier, I split it: > > 1. Tests and benchmarks: apache#25558 (merged). They record the wrong results on `main`. > 2. Executor fix: this PR. It flips the expectations that it fixes. > 3. Optimizer fix: apache#25560, stacked on this PR. > > The executor code is the same as at 78b49ba (the last head that was reviewed), less one dead line. The review threads on `decorrelate_predicate_subquery.rs` ([plain `IN`](apache#25339 (comment)), [`InSubquery` value](apache#25339 (comment))), the [`on[0]` thread](apache#25339 (comment)) and the [constant-projection comment](apache#25339 (comment)) are fixed in apache#25560. ## Rationale for this change A correlated `NOT IN` whose correlation cannot become an equi-join key leaves a residual join filter. The null-aware hash join ignored that filter when deciding whether a NULL on the subquery side makes `NOT IN` UNKNOWN, so a NULL the filter excludes still poisoned every outer row: ```sql CREATE TABLE oc(id INT, g INT) AS VALUES (1,5),(2,5),(3,0),(4,NULL),(NULL,5),(NULL,0); CREATE TABLE ic(id INT) AS VALUES (1),(NULL); SELECT id, g FROM oc WHERE oc.id NOT IN (SELECT ic.id FROM ic WHERE oc.g > 0); ``` returns no rows; DuckDB and PostgreSQL return `3|0`, `4|NULL`, `NULL|0`. `oc.g > 0` holds only for `id` 1, 2 and the NULL-id row, so only those three see the subquery `{1, NULL}`; the rest see an empty subquery, and `NOT IN` over an empty set is TRUE. The plan was already right — `LeftAnti ... Filter: oc.g > Int32(0) null_aware` — so this is purely an execution fix. No optimizer change is involved. ## What changes are included in this PR? A NULL now makes `NOT IN` UNKNOWN only for the build rows whose correlation scope and residual filter keep that NULL, recorded per build row in a null-indices bitmap. Candidates come from a scope-map lookup when there are correlation keys and from a cross product otherwise, then pass the filter. Cost is proportional to the number of NULLs and is zero when the data has none; build rows already marked UNKNOWN are skipped. Null-aware `LeftAnti` also accepts more than one join key, which the equality-correlated shape needs. `RightAnti` still requires exactly one. ## What is the testing strategy for this PR? The coverage landed in apache#25558. This PR flips the expectations it fixes — the diff in `null_aware_anti_join.slt` and the Q05–Q07 canaries is the behaviour change. A "pinned to today's behaviour" note is removed only where the expectation below it changes; the notes on shapes that apache#25560 fixes stay. `datafusion/physical-plan/src/joins/hash_join/exec.rs` also gains unit tests for the filter-only anti and mark paths at several batch sizes. ## Are there any user-facing changes? Correlated `NOT IN` with a residual filter returns correct results. Some shapes that failed to plan now run. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Adrian Garcia Badaracco <adriangb@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
mohitgurav20
pushed a commit
to mohitgurav20/datafusion
that referenced
this pull request
Sep 24, 2026
apache#25560) ## Which issue does this PR close? - Completes apache#25336 for the mark-join shapes. > [!NOTE] > The executor fix landed in apache#25339 and the tests in apache#25558. This PR is the last part of that split. > > This PR fixes the review threads from apache#25339 on `decorrelate_predicate_subquery.rs` ([plain `IN`](apache#25339 (comment)), [`InSubquery` value](apache#25339 (comment))), the [`on[0]` thread](apache#25339 (comment)) and the [constant-projection comment](apache#25339 (comment)). ## Rationale for this change Decorrelation asked for null-aware semantics only for some `NOT IN` mark joins. A non-equality correlation leaves a residual join filter rather than an equi-join key, and that shape was planned without it, so a NULL on the subquery side read as FALSE instead of UNKNOWN: ```sql CREATE TABLE oc(id INT, g INT, h INT) AS VALUES (2,2,0),(1,2,0),(9,2,5),(3,1,0); CREATE TABLE ic(id INT, k INT) AS VALUES (1,1),(NULL,1),(2,2); SELECT id FROM oc WHERE oc.h > 1 OR oc.id NOT IN (SELECT i.id FROM ic i WHERE i.k < oc.g) ORDER BY id; ``` returns `2, 3, 9`; DuckDB returns `3, 9`. Row `id = 2` is UNKNOWN, not TRUE. apache#25339 gives the executor the machinery. This asks for it in the remaining place. ## What changes are included in this PR? A mark join needs null-aware semantics only where a NULL mark can behave differently from a FALSE mark. `AND`/`OR` give TRUE only from TRUE, and a `Filter` keeps a row only when the predicate is TRUE, so a non-negated `IN` reached through nothing but `AND`/`OR` is identical with a FALSE mark and must stay on the plain join. Requesting it there is expensive: **1–2 ms → 7.4 s at 100k × 100k**. That test is made **per subquery occurrence**, in a recursion that only knows `AND`/`OR`. The permissive outcome lives in one arm whose pattern is its own proof — a non-negated `IN` whose value holds no subquery, reached through nothing but `AND`/`OR` frames — and everything else, including future `Expr` variants, takes the null-aware branch. Two further changes in `build_join`: - a constant `IN` value is projected as an outer column so the equality becomes `on[0]`, the key position the executor reads as the `NOT IN` value key. Otherwise a correlation takes that slot and the value-key NULL rules are applied to the wrong key; - `null_aware` is computed once instead of being re-derived for the constant projection, the mark branch and the anti branch. Net effect on the optimizer source is **−14 lines**. ## What is the testing strategy for this PR? The coverage landed in apache#25558; this PR flips the remaining expectations, including the Q08 canary, so the diff shows the behaviour change. It also removes one note that apache#25558 put on the wrong query in `subquery.slt` (`#simple_uncorrelated_scalar_subquery2`, which no fix changes). The plan pins added in apache#25558 guard the *negative* direction — that a positive `IN` stays on the plain join — which no result assertion can catch, since a needless null-aware join is correct, only slower. Mutation testing over the decision confirms it: mutants that widen null-awareness are killed only by those pins. ### Benchmarks Results against the apache#25339 merge: [`null_aware_join`](apache#25560 (comment)), [`projection_subquery`](apache#25560 (comment)), [`tpcds`](apache#25560 (comment)). - `tpcds`, `projection_subquery`, and `null_aware_join` Q01–Q07 and Q09 do not change. Q09 is the positive `IN` that must stay on the plain join. - `null_aware_join` Q08 goes from 0.99 ms to 20.6 ms. This is not a regression: on `main`, Q08 returns an incorrect result (its correctness canary is `false`), so 0.99 ms is the time to calculate a wrong answer. 20.6 ms is the cost of the correct result, and it is the same as the combined apache#25339 measured (21.2 ms). - The remaining Q08 cost comes from the scope-key path in the executor, not from this PR. The [DuckDB comparison on apache#25339](apache#25339 (comment)) explains it, and apache#25438 tracks the fix. ## Are there any user-facing changes? Correlated `NOT IN` inside a larger predicate returns correct results. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Adrian Garcia Badaracco <adriangb@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
diegoQuinas
pushed a commit
to diegoQuinas/datafusion
that referenced
this pull request
Sep 24, 2026
apache#25560) ## Which issue does this PR close? - Completes apache#25336 for the mark-join shapes. > [!NOTE] > The executor fix landed in apache#25339 and the tests in apache#25558. This PR is the last part of that split. > > This PR fixes the review threads from apache#25339 on `decorrelate_predicate_subquery.rs` ([plain `IN`](apache#25339 (comment)), [`InSubquery` value](apache#25339 (comment))), the [`on[0]` thread](apache#25339 (comment)) and the [constant-projection comment](apache#25339 (comment)). ## Rationale for this change Decorrelation asked for null-aware semantics only for some `NOT IN` mark joins. A non-equality correlation leaves a residual join filter rather than an equi-join key, and that shape was planned without it, so a NULL on the subquery side read as FALSE instead of UNKNOWN: ```sql CREATE TABLE oc(id INT, g INT, h INT) AS VALUES (2,2,0),(1,2,0),(9,2,5),(3,1,0); CREATE TABLE ic(id INT, k INT) AS VALUES (1,1),(NULL,1),(2,2); SELECT id FROM oc WHERE oc.h > 1 OR oc.id NOT IN (SELECT i.id FROM ic i WHERE i.k < oc.g) ORDER BY id; ``` returns `2, 3, 9`; DuckDB returns `3, 9`. Row `id = 2` is UNKNOWN, not TRUE. apache#25339 gives the executor the machinery. This asks for it in the remaining place. ## What changes are included in this PR? A mark join needs null-aware semantics only where a NULL mark can behave differently from a FALSE mark. `AND`/`OR` give TRUE only from TRUE, and a `Filter` keeps a row only when the predicate is TRUE, so a non-negated `IN` reached through nothing but `AND`/`OR` is identical with a FALSE mark and must stay on the plain join. Requesting it there is expensive: **1–2 ms → 7.4 s at 100k × 100k**. That test is made **per subquery occurrence**, in a recursion that only knows `AND`/`OR`. The permissive outcome lives in one arm whose pattern is its own proof — a non-negated `IN` whose value holds no subquery, reached through nothing but `AND`/`OR` frames — and everything else, including future `Expr` variants, takes the null-aware branch. Two further changes in `build_join`: - a constant `IN` value is projected as an outer column so the equality becomes `on[0]`, the key position the executor reads as the `NOT IN` value key. Otherwise a correlation takes that slot and the value-key NULL rules are applied to the wrong key; - `null_aware` is computed once instead of being re-derived for the constant projection, the mark branch and the anti branch. Net effect on the optimizer source is **−14 lines**. ## What is the testing strategy for this PR? The coverage landed in apache#25558; this PR flips the remaining expectations, including the Q08 canary, so the diff shows the behaviour change. It also removes one note that apache#25558 put on the wrong query in `subquery.slt` (`#simple_uncorrelated_scalar_subquery2`, which no fix changes). The plan pins added in apache#25558 guard the *negative* direction — that a positive `IN` stays on the plain join — which no result assertion can catch, since a needless null-aware join is correct, only slower. Mutation testing over the decision confirms it: mutants that widen null-awareness are killed only by those pins. ### Benchmarks Results against the apache#25339 merge: [`null_aware_join`](apache#25560 (comment)), [`projection_subquery`](apache#25560 (comment)), [`tpcds`](apache#25560 (comment)). - `tpcds`, `projection_subquery`, and `null_aware_join` Q01–Q07 and Q09 do not change. Q09 is the positive `IN` that must stay on the plain join. - `null_aware_join` Q08 goes from 0.99 ms to 20.6 ms. This is not a regression: on `main`, Q08 returns an incorrect result (its correctness canary is `false`), so 0.99 ms is the time to calculate a wrong answer. 20.6 ms is the cost of the correct result, and it is the same as the combined apache#25339 measured (21.2 ms). - The remaining Q08 cost comes from the scope-key path in the executor, not from this PR. The [DuckDB comparison on apache#25339](apache#25339 (comment)) explains it, and apache#25438 tracks the fix. ## Are there any user-facing changes? Correlated `NOT IN` inside a larger predicate returns correct results. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Adrian Garcia Badaracco <adriangb@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Correlated
NOT INreturns wrong results in several shapes, and a few do not plan at all. The fixes span two layers — the hash join executor and decorrelation — and touch different shapes. Landing the coverage first, pinned to what DataFusion does today, makes each fix's diff show exactly which behaviour it changes instead of burying the flips in a large change.Nothing here changes behaviour. Every expectation is what
mainproduces right now, and the wrong ones carry a note and a link to the issue.Concretely, this is wrong today (DuckDB 1.5.2 and PostgreSQL 17.11 both return three rows):
What changes are included in this PR?
sqllogictest, in
null_aware_anti_join.sltandnull_aware_mark_join.slt:NOT INwith a non-equality correlation, which stays a residual join filter;INvalue, both spellings of the outerIN;IS NOT NULLover a subquery predicate, and a comparison between two marks — the contexts that can tell a NULL mark from a FALSE mark;Benchmarks:
IN. It must not use a null-aware join. Q01–Q08 all cover the direction where null-awareness is required, so a regression that adds it where it is not needed is invisible to them. Q09 passes today.NOT INresult against a reference that does not useNOT IN. All four disagree today, so they are pinned tofalse.What is the testing strategy for this PR?
This PR is tests. Expected results were verified against DuckDB 1.5.2 and PostgreSQL 17.11. The whole suite is green on
main, including all nine benchmarks.Are there any user-facing changes?
No.
🤖 Generated with Claude Code