Skip to content

Commit 95bb0a0

Browse files
adriangbclaude
andauthored
fix: correlated NOT IN with a non-equality correlation returns wrong results (#25339)
## Which issue does this PR close? - Closes #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: #25558 (merged). They record the wrong results on `main`. > 2. Executor fix: this PR. It flips the expectations that it fixes. > 3. Optimizer fix: #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`](#25339 (comment)), [`InSubquery` value](#25339 (comment))), the [`on[0]` thread](#25339 (comment)) and the [constant-projection comment](#25339 (comment)) are fixed in #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 #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 #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>
1 parent ca0a6a4 commit 95bb0a0

7 files changed

Lines changed: 568 additions & 218 deletions

File tree

‎benchmarks/sql_benchmarks/null_aware_join/benchmarks/q05.benchmark‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,7 @@ SELECT count(*) = (
1515
FROM small_outer o
1616
WHERE o.id_n1 NOT IN (SELECT i.id_n0 FROM small_inner i WHERE i.z < o.z);
1717
----
18-
# Pinned to today's behaviour, which is incorrect. See
19-
# https://github.com/apache/datafusion/issues/25336 -- the fix flips this.
20-
false
18+
true
2119

2220
expect_plan HashJoinExec
2321
expect_plan null_aware

‎benchmarks/sql_benchmarks/null_aware_join/benchmarks/q06.benchmark‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,7 @@ SELECT count(*) = (
1515
FROM small_outer o
1616
WHERE o.id_n50 NOT IN (SELECT i.id_n0 FROM small_inner i WHERE i.z < o.z);
1717
----
18-
# Pinned to today's behaviour, which is incorrect. See
19-
# https://github.com/apache/datafusion/issues/25336 -- the fix flips this.
20-
false
18+
true
2119

2220
expect_plan HashJoinExec
2321
expect_plan null_aware

‎benchmarks/sql_benchmarks/null_aware_join/benchmarks/q07.benchmark‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,7 @@ SELECT count(*) = (
1717
FROM small_outer o
1818
WHERE o.id_n0 NOT IN (SELECT i.id_n50 FROM small_inner i WHERE i.z < o.z);
1919
----
20-
# Pinned to today's behaviour, which is incorrect. See
21-
# https://github.com/apache/datafusion/issues/25336 -- the fix flips this.
22-
false
20+
true
2321

2422
expect_plan HashJoinExec
2523
expect_plan null_aware

‎datafusion/physical-optimizer/src/join_selection.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,11 +172,13 @@ impl PhysicalOptimizerRule for JoinSelection {
172172
}
173173
}
174174

175-
/// Determines whether it is possible to swap inputs of a hash join - for null-aware joins, we can only swap `LeftAnti` with no filters
175+
/// Determines whether it is possible to swap inputs of a hash join - for null-aware joins, we can only swap an uncorrelated `LeftAnti`
176+
/// (a single join key and no filter), because the swapped `RightAnti` has no per-row NULL handling
176177
fn can_swap_hash_join(hash_join: &HashJoinExec) -> bool {
177178
hash_join.join_type().supports_swap()
178179
&& (!hash_join.null_aware
179180
|| (*hash_join.join_type() == JoinType::LeftAnti
181+
&& hash_join.on().len() == 1
180182
&& hash_join.filter().is_none()))
181183
}
182184

0 commit comments

Comments
 (0)