Commit d4fefea
fix: SortMergeJoin full outer join incorrectly matches rows when filter evaluates to NULL (apache#21660)
## Which issue does this PR close?
- Closes #.
## Rationale for this change
While enabling `SortMergeJoinExec` with filters in [DataFusion
Comet](https://github.com/apache/datafusion-comet), I hit a correctness
bug in DataFusion's `SortMergeJoinExec` for full outer joins with
nullable filter columns. The bug was originally surfaced via the
[SPARK-43113](https://issues.apache.org/jira/browse/SPARK-43113)
reproducer.
When a join filter expression evaluates to `NULL` (e.g., `l.b < (r.b +
1)` where `r.b` is `NULL`), the full outer join treats the row pair as
**matched** instead of **unmatched**. Per SQL semantics, `NULL` in a
boolean filter context is `false` (not satisfied), so the rows should be
emitted as separate unmatched rows.
The bug has been present since filtered full outer join support was
added to SMJ in apache#12764 / apache#13369. It was never caught because:
1. The join fuzz tests generate filter column data with
`Int32Array::from_iter_values()`, which never produces `NULL` values.
2. No existing unit test or sqllogictest exercised a full outer join
filter that evaluates to `NULL`.
## What changes are included in this PR?
**Root cause:** The full outer join code path had a special case that
preserved raw `NULL` values from the filter expression result
(`pre_mask`) instead of converting them to `false` via
`prep_null_mask_filter` like left/right outer joins do. This caused two
problems:
1. **Streamed (left) side:** In `get_corrected_filter_mask()`, `NULL`
entries in `filter_mask` are treated as "pass through" (for
pre-null-joined rows from `append_nulls()`). But when the filter
expression itself returns `NULL`, those entries also appear as `NULL` in
the mask — and get incorrectly treated as matched. This produced wrong
join output (matched rows instead of unmatched).
2. **Buffered (right) side:** `BooleanArray::value()` was called on
`NULL` positions in `pre_mask` to update `FilterState`. At NULL
positions, the values buffer contains a deterministic but semantically
meaningless result (computed from the default zero-storage of NULL
inputs). For some rows this value happens to be `true`, which
incorrectly marks unmatched buffered rows as `SomePassed` and silently
drops them from the output.
**Fix:** Remove the full outer join special case in
`materializing_stream.rs`. All outer join types now uniformly use the
null-corrected `mask` (where `NULL` → `false` via
`prep_null_mask_filter`) for both deferred filtering metadata and
`FilterState` tracking. Semi/anti/mark joins are unaffected — they use
`BitwiseSortMergeJoinStream` which already converts NULLs to `false`.
**Tests:**
- New unit test `join_full_null_filter_result` reproducing the
SPARK-43113 scenario with a nullable right-side column.
- Modified `make_staggered_batches_i32` in `join_fuzz.rs` to inject ~10%
`NULL` values into the filter column (`x`), so the fuzz tests exercise
`NULL` filter handling across all join types.
## Are these changes tested?
Yes.
- New unit test (`join_full_null_filter_result`) directly reproduces the
bug.
- Existing 57 SMJ unit tests all pass.
- All 41 join fuzz tests pass with the new nullable filter column data,
including `test_full_join_1k_filtered` which compares `HashJoinExec` vs
`SortMergeJoinExec` and would have caught this bug if the fuzz data had
included `NULL`s.
- Will run 100 iterations of the fuzz tests overnight to shake out any
remaining nondeterministic issues.
- Testing in Comet CI (all Spark SQL tests)
apache/datafusion-comet#3916
## Are there any user-facing changes?
Full outer sort-merge joins with filters involving nullable columns now
produce correct results. Previously, rows where the filter evaluated to
`NULL` were incorrectly included as matched; they are now correctly
emitted as unmatched (null-joined) rows.1 parent ff7d243 commit d4fefea
File tree
3 files changed
+122
-19
lines changed- datafusion
- core/tests/fuzz_cases
- physical-plan/src/joins/sort_merge_join
3 files changed
+122
-19
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1276 | 1276 | | |
1277 | 1277 | | |
1278 | 1278 | | |
1279 | | - | |
| 1279 | + | |
| 1280 | + | |
| 1281 | + | |
| 1282 | + | |
| 1283 | + | |
| 1284 | + | |
| 1285 | + | |
| 1286 | + | |
1280 | 1287 | | |
1281 | 1288 | | |
1282 | 1289 | | |
| |||
Lines changed: 9 additions & 14 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1423 | 1423 | | |
1424 | 1424 | | |
1425 | 1425 | | |
1426 | | - | |
| 1426 | + | |
| 1427 | + | |
1427 | 1428 | | |
1428 | | - | |
1429 | | - | |
| 1429 | + | |
| 1430 | + | |
| 1431 | + | |
| 1432 | + | |
1430 | 1433 | | |
1431 | | - | |
| 1434 | + | |
1432 | 1435 | | |
1433 | 1436 | | |
1434 | 1437 | | |
1435 | | - | |
1436 | | - | |
1437 | | - | |
1438 | | - | |
1439 | | - | |
1440 | | - | |
1441 | | - | |
1442 | | - | |
1443 | 1438 | | |
1444 | 1439 | | |
1445 | 1440 | | |
1446 | | - | |
| 1441 | + | |
1447 | 1442 | | |
1448 | 1443 | | |
1449 | 1444 | | |
| |||
1468 | 1463 | | |
1469 | 1464 | | |
1470 | 1465 | | |
1471 | | - | |
| 1466 | + | |
1472 | 1467 | | |
1473 | 1468 | | |
1474 | 1469 | | |
| |||
Lines changed: 105 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
55 | 55 | | |
56 | 56 | | |
57 | 57 | | |
58 | | - | |
| 58 | + | |
59 | 59 | | |
60 | 60 | | |
61 | 61 | | |
| |||
65 | 65 | | |
66 | 66 | | |
67 | 67 | | |
| 68 | + | |
68 | 69 | | |
69 | 70 | | |
70 | 71 | | |
| |||
2049 | 2050 | | |
2050 | 2051 | | |
2051 | 2052 | | |
| 2053 | + | |
| 2054 | + | |
| 2055 | + | |
| 2056 | + | |
| 2057 | + | |
| 2058 | + | |
| 2059 | + | |
| 2060 | + | |
| 2061 | + | |
| 2062 | + | |
| 2063 | + | |
| 2064 | + | |
| 2065 | + | |
| 2066 | + | |
| 2067 | + | |
| 2068 | + | |
| 2069 | + | |
| 2070 | + | |
| 2071 | + | |
| 2072 | + | |
| 2073 | + | |
| 2074 | + | |
| 2075 | + | |
| 2076 | + | |
| 2077 | + | |
| 2078 | + | |
| 2079 | + | |
| 2080 | + | |
| 2081 | + | |
| 2082 | + | |
| 2083 | + | |
| 2084 | + | |
| 2085 | + | |
| 2086 | + | |
| 2087 | + | |
| 2088 | + | |
| 2089 | + | |
| 2090 | + | |
| 2091 | + | |
| 2092 | + | |
| 2093 | + | |
| 2094 | + | |
| 2095 | + | |
| 2096 | + | |
| 2097 | + | |
| 2098 | + | |
| 2099 | + | |
| 2100 | + | |
| 2101 | + | |
| 2102 | + | |
| 2103 | + | |
| 2104 | + | |
| 2105 | + | |
| 2106 | + | |
| 2107 | + | |
| 2108 | + | |
| 2109 | + | |
| 2110 | + | |
| 2111 | + | |
| 2112 | + | |
| 2113 | + | |
| 2114 | + | |
| 2115 | + | |
| 2116 | + | |
| 2117 | + | |
| 2118 | + | |
| 2119 | + | |
| 2120 | + | |
| 2121 | + | |
| 2122 | + | |
| 2123 | + | |
| 2124 | + | |
| 2125 | + | |
| 2126 | + | |
| 2127 | + | |
| 2128 | + | |
| 2129 | + | |
| 2130 | + | |
| 2131 | + | |
| 2132 | + | |
| 2133 | + | |
| 2134 | + | |
| 2135 | + | |
| 2136 | + | |
| 2137 | + | |
| 2138 | + | |
| 2139 | + | |
| 2140 | + | |
| 2141 | + | |
| 2142 | + | |
| 2143 | + | |
| 2144 | + | |
| 2145 | + | |
| 2146 | + | |
| 2147 | + | |
| 2148 | + | |
| 2149 | + | |
| 2150 | + | |
| 2151 | + | |
| 2152 | + | |
| 2153 | + | |
| 2154 | + | |
2052 | 2155 | | |
2053 | 2156 | | |
2054 | 2157 | | |
| |||
3589 | 3692 | | |
3590 | 3693 | | |
3591 | 3694 | | |
3592 | | - | |
3593 | | - | |
3594 | | - | |
| 3695 | + | |
3595 | 3696 | | |
3596 | 3697 | | |
3597 | 3698 | | |
| |||
0 commit comments