Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25339 +/- ##
========================================
Coverage 82.33% 82.33%
========================================
Files 1137 1137
Lines 431884 432004 +120
Branches 431884 432004 +120
========================================
+ Hits 355586 355693 +107
- Misses 54805 54813 +8
- Partials 21493 21498 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The approach looks good to me, especially keeping the residual join filter involved when determining the per-build-row UNKNOWN state for correlated NOT IN. I also like the added coverage for both anti and mark joins.
I left one non-blocking performance suggestion below. Nothing that needs to hold up the PR.
| None => { | ||
| let probe_rows = | ||
| UInt32Array::from_iter_values(0..state.batch.num_rows() as u32); | ||
| for_each_cross_product( |
There was a problem hiding this comment.
One potential performance concern here: when there are no scope keys, we evaluate the residual filter for every NULL build row × probe row pair. The symmetric path below does the same for NULL probe rows. For a nullable non-equality-correlated NOT IN, that could add quadratic work, including for build rows that have already been marked UNKNOWN.
Would it be worth adding a small bounded benchmark or targeted performance regression test for this path? As a follow-up optimization, we might also be able to skip build rows that are already marked UNKNOWN, although we'd need to be careful about volatile or erroring filter expressions.
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @adriangb , 2 non-blocking suggestions
| None => { | ||
| let build_rows = | ||
| UInt64Array::from_iter_values(0..left_data.batch().num_rows() as u64); | ||
| for_each_cross_product( |
There was a problem hiding this comment.
The case with no scope keys re-checks build rows that are already marked UNKNOWN
Without scope keys, case 2 evaluates the filter for every (build row × NULL probe row) pair in every probe batch, including build rows already set in null_indices_bitmap. Those bits never clear, so that work is wasted. 20K outer × 10K NULL inner with i.z < o.z spends 8.85s in join_time (debug build). Skip marked rows and stop once none are left (same for case 1 at :1466):
None => {
let num_build_rows = left_data.batch().num_rows();
for probe_rows in null_probe_rows.values().chunks(batch_size.max(1)) {
let build_rows = {
let bitmap = left_data.null_indices_bitmap().lock();
UInt64Array::from_iter_values(
(0..num_build_rows)
.filter(|i| !bitmap.get_bit(*i))
.map(|i| i as u64),
)
};
if build_rows.is_empty() {
break;
}
let probe_rows = UInt32Array::from(probe_rows.to_vec());
for_each_cross_product(&build_rows, &probe_rows, batch_size, &mut mark)?;
}
}Fine to handle in a follow-up
| num_keys: usize, | ||
| has_filter: bool, | ||
| ) -> Result<Self> { | ||
| let correlated = num_keys > 1 || has_filter; |
There was a problem hiding this comment.
correlated = num_keys > 1 || has_filter assumes on[0] is the NOT IN value key. When the value has no outer columns, 1 = i.id is pushed into the subquery, so on[0] becomes the correlation key o.g = i.g, and a NULL o.g is marked UNKNOWN:
CREATE TABLE o(id INT, g INT, z INT) AS VALUES (1,1,10),(2,NULL,10),(3,2,10);
CREATE TABLE i(id INT, g INT, z INT) AS VALUES (1,1,5),(5,2,5),(NULL,3,5);
SELECT id FROM o WHERE 1 NOT IN (SELECT i.id FROM i WHERE i.g = o.g AND i.z < o.z);
-- expected 2, 3; returns 3The form without AND i.z < o.z is also wrong and doesn't go through the new code, so this predates the PR. Fine as a follow-up: in build_join, only set null_aware when the in-predicate's outer side references a left column.
There was a problem hiding this comment.
Agreed, this is a bug in main:
main: R1 (with i.z < o.z) → 1 row R2 (equality only) → 1 row
PR: R1 → 1 row R2 → 1 row
|
@jayzhan211 @kosiew I opened #25386 w/ benchmarks for this change. Could we merge that first so we can look at before/afterS? |
HashJoinExec's Debug output did not include null_aware, so `expect_plan HashJoinExec` also passed for a plain anti join. Add the field to Debug and require `null_aware: true` on Q02-Q07. Q01 has non-nullable keys, so it is not null-aware. Q08 plans as a plain mark join on main until apache#25339 lands, so it keeps only the HashJoinExec check here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each assert compares the NOT IN count with a reference count that does not use NOT IN, so it holds at every NAJ_ROWS / NAJ_LARGE_ROWS value. Q05-Q08 give wrong results on main (apache#25336), so their asserts go in apache#25339 together with the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s UNKNOWN A null-aware LeftAnti join with a join filter ignored the filter for NULL keys: one NULL probe key removed every build row, even when the filter excluded that NULL row for every build row. A null-aware LeftAnti join with correlation scope keys failed to plan. Treat a null-aware LeftAnti or LeftMark join as correlated when it has scope keys or a join filter. Correlated joins record the UNKNOWN decision per build row in the null-indices bitmap: the candidate (build, probe) pairs come from the scope map, or from all pairs when there are no scope keys, and the join filter decides which pairs count. The LeftAnti final stage drops the rows marked UNKNOWN. JoinSelection only swaps an uncorrelated null-aware LeftAnti. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ware The hash join now applies the join filter when it marks UNKNOWN rows, so a NOT IN mark join no longer needs to fall back to a non null-aware join when a non-equality correlation stays behind as a join filter. The fallback gave FALSE instead of NULL, so NOT (x IN (...)) returned extra rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds sqllogictest regression tests for apache#25336 (expected results checked with DuckDB and PostgreSQL) and HashJoinExec unit tests for null-aware LeftAnti and LeftMark joins that have a join filter and no scope keys. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ll_aware These asserts fail on main (apache#25336) and pass with the fix. Q08 is a null-aware mark join once the fix lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
## Which issue does this PR close? - N/A. This PR adds benchmarks only. It is split out of apache#25339 so that the suite is on `main` first, and that PR can then be measured against it. ## Rationale for this change A `NOT IN` subquery becomes a null-aware join. An outer row that finds no match is TRUE only when neither side has a NULL in scope. If a NULL is in scope, the result is UNKNOWN. This decision is cheap for an uncorrelated `NOT IN`. For a correlated `NOT IN`, the correlation predicate stays behind as a join filter. The join must then evaluate that filter for each candidate (build row x probe row) pair, to find which rows the NULLs reach. A non-equality correlation gives no equality key, so there is no scope key to reduce the number of pairs. The cost then grows with the NULL count multiplied by the size of the opposite table. No benchmark measured this shape, so there was no way to see the cost, or to tell a change from noise. Review on apache#25339 asked for this benchmark. These are the measured results for apache#25339. Each number is the median of 60 iterations, taken as 6 interleaved rounds of 10 iterations on an Apple M4 Pro in release mode. The two sides are the base commit of apache#25339 and its head commit, each with this suite applied, so the comparison isolates the change in that PR. | Query | Shape | base | apache#25339 | | |---|---|---|---|---| | Q01 | uncorrelated, non-nullable keys | 17.9 ms | 17.7 ms | 0.99x | | Q02 | uncorrelated, 1% NULL subquery side | 14.9 ms | 14.9 ms | 1.00x | | Q03 | uncorrelated, 50% NULL outer side | 14.6 ms | 14.6 ms | 1.00x | | Q04 | correlated, nullable keys, no NULL present | 0.9 ms | 0.9 ms | 0.96x | | Q05 | correlated, 1% NULL outer side | 0.9 ms | 2.9 ms | 3.1x | | Q06 | correlated, 50% NULL outer side | 0.9 ms | 96.2 ms | 109x | | Q07 | correlated, 50% NULL subquery side | 0.8 ms | 94.4 ms | 112x | | Q08 | as Q06, with an equality correlation | 1.1 ms | 15.1 ms | 14x | Q01 to Q04 are the comparable rows, and they show no change. The base gives wrong results for Q05 to Q08, which is the bug that apache#25339 corrects. Thus the base numbers for those four rows are the time to calculate an incorrect result. They show the cost of correct results, not a regression. These are the results at the default sizes. DuckDB agrees with the "correct" column. | Query | correct (apache#25339) | base | |---|---|---| | Q05 | 7460 | 7450 | | Q06 | 5010 | 5000 | | Q07 | 10 | 0 | | Q08 | 5530 | 10000 | Q06 and Q07 are the rows that the review of apache#25339 asked about. They also give the baseline to measure any later optimization of that path against. Q08 has the same NULL fraction as Q06 and is 6 times cheaper, which is the value of the equality correlation. ## What changes are included in this PR? A `null_aware_join` SQL benchmark suite. There are no Rust changes. The runner finds suites in `benchmarks/sql_benchmarks/`, and the load SQL makes each table from `range()`, so there is no data generation step. - Q01 to Q03 are uncorrelated `NOT IN` at different NULL fractions. Their cost is linear with the table size. They are the regression guard for the plain null-aware path. - Q04 is the correlated shape with nullable keys that hold no NULL. It separates the baseline cost of the shape from the per-pair filter work. - Q05 to Q07 are the same correlation at 1% and 50% NULL on each side. This is where that work becomes visible. - Q08 has the same NULL fraction as Q06, but adds an equality correlation. The candidate pairs then come from a hash lookup. The difference between Q06 and Q08 shows the value of the scope key. Both table sizes are knobs. `NAJ_ROWS` (default 10000) sets the size for the correlated queries, whose cost grows with its square. `NAJ_LARGE_ROWS` (default 1000000) sets the size for the uncorrelated queries. ```bash ./bench.sh run null_aware_join # One query, with more rows for the correlated shape NAJ_ROWS=20000 ./bench.sh run null_aware_join 6 ``` This PR also adds the suite to `bench.sh` (including `all`) and documents it in `benchmarks/README.md` and `benchmarks/sql_benchmarks/README.md`. There is one Rust change: the `Debug` output of `HashJoinExec` now includes `null_aware`. The suite's `expect_plan` directive matches that output, so Q02 to Q07 can require `null_aware: true`. Before this change, `expect_plan HashJoinExec` also passed for a plain anti join. ## What is the testing strategy for this PR? This PR adds benchmarks, so it adds no new tests. The existing `checked_in_suites_cover_benchmark_directories` test in `benchmarks/src/sql_benchmark_suite.rs` covers suite discovery, and it passes with the new directory. Each query has these checks: - `expect_plan HashJoinExec`. Q02 to Q07 also require `expect_plan null_aware: true`. Q01 has non-nullable keys, so it is not null-aware. Q08 plans as a plain mark join on `main`, so apache#25339 adds its `null_aware: true` check. - Q01 to Q04 have an `assert` correctness canary. The assert compares the `NOT IN` count with a reference count that does not use `NOT IN`, so it is correct for all values of `NAJ_ROWS` and `NAJ_LARGE_ROWS`. I checked each reference against the `NOT IN` result in DuckDB at six pairs of sizes. The same asserts for Q05 to Q08 fail on `main`, so apache#25339 adds them together with the fix. I ran them on this branch merged with apache#25339, and all eight queries pass at the default sizes and at `-r 3000 -l 1001`. I ran all eight queries on this branch at the default sizes and at `-r 1500 -l 101`. As a counterfactual check on `main`, the Q05 to Q08 asserts fail, and `null_aware: true` fails on Q01 and Q08. Each query also runs on `main` as written. Q08 uses the mark join form on purpose. The plain `WHERE ... NOT IN` form with an equality correlation does not plan on `main`, and a query that runs on only one branch cannot compare two branches. ## Are there any user-facing changes? No. This PR changes benchmarks and documentation only. It does not change library code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com>
e9a4ea3 to
31f4dcd
Compare
Which issue does this PR close?
Rationale for this change
A correlated
NOT INsubquery gives wrong results when the correlation is not an equality and the subquery column contains NULL. There is no error and no warning.The same gap also makes an equality-correlated
NOT INin aWHEREclause fail to plan:The fix sketch in the issue (turn off
null_awarefor aLeftAntijoin that has a join filter) does not work. I tried it: the plain anti join ignores NULLs completely, so queries that are correct today start to return rows. For example,id NOT IN (SELECT t2.id FROM t2 WHERE t2.z > t1.z)must return no rows, and returns1, 2, 4, NULLwith that change.What changes are included in this PR?
The hash join already had the right mechanism for correlated
NOT INmark joins with equality correlation keys: a per-build-row bitmap that records "this row'sNOT INis UNKNOWN". This PR uses that mechanism for every correlated null-aware join and makes it apply the join filter. The commits are split for review:HashJoinExec: a null-awareLeftAntiorLeftMarkjoin is correlated when it has correlation scope keys or a join filter. For a NULL value on either side, the join finds the candidate (build, probe) row pairs through the scope key hash map, or takes all pairs when there are no scope keys. The join filter then decides which pairs make a build row UNKNOWN. TheLeftAntifinal stage drops those rows. The extra work is only for rows that have a NULL value key, so it is zero when the data has no NULLs.JoinSelectionswaps a null-awareLeftAntionly when it has a single key and no filter.DecorrelatePredicateSubquery: aNOT INmark join with a non-equality correlation is now planned as null-aware. FourEXPLAINresults insubquery.sltchange: the mark join now showsnull_aware, and in one of them the join is no longer swapped toRightMark, because null-aware mark joins are never swapped.null_aware_joinsuite (bench: SQL benchmark suite for null-aware (NOT IN) joins #25386), andexpect_plan null_aware: truefor Q08. These checks fail onmainand pass with this PR.What is the testing strategy for this PR?
null_aware_anti_join.sltandnull_aware_mark_join.slt: the queries from the issue, NULL outer values with empty and non-empty subquery results, equality plus non-equality correlation, a filter on the subquery value itself, the positiveINform, the mark column throughIS NULL/IS TRUE/IS FALSE/NOT ... ORand directly in aSELECTlist, and runs withbatch_size = 1. I checked all expected results with DuckDB 1.5.2 and PostgreSQL 17.11. 14 of these cases fail onmain.HashJoinExecunit tests for a null-awareLeftAntiandLeftMarkjoin that has a join filter and no scope keys, at all batch sizes.null_aware_joinbenchmark suite (bench: SQL benchmark suite for null-aware (NOT IN) joins #25386) now checks the result of all eight queries. Each assert compares theNOT INcount with a reference count that does not useNOT IN, so it holds for all values ofNAJ_ROWSandNAJ_LARGE_ROWS. The suite passes at the default sizes and at-r 3000 -l 1001. Onmain, the asserts for Q05 to Q08 fail. See bench: SQL benchmark suite for null-aware (NOT IN) joins #25386 for the performance numbers.Are there any user-facing changes?
Queries that returned wrong results now return correct results, and correlated
NOT INwith an equality correlation in aWHEREclause no longer fails to plan. There are no public API changes.🤖 Generated with Claude Code