Skip to content

test: characterize correlated NOT IN null-aware behaviour - #25558

Merged
adriangb merged 1 commit into
apache:mainfrom
pydantic:na-tests
Sep 21, 2026
Merged

adriangb merged 1 commit into
apache:mainfrom
pydantic:na-tests

Conversation

@adriangb

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Correlated NOT IN returns 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 main produces 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):

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

What changes are included in this PR?

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 naming 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 join the planner picks in each case.

Benchmarks:

  • Q09, a correlated non-negated 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.
  • correctness canaries on Q05–Q08, each comparing the NOT IN result against a reference that does not use NOT IN. All four disagree today, so they are pinned to false.

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

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>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.41%. Comparing base (1e09a2a) to head (6f70c9d).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@adriangb

Copy link
Copy Markdown
Contributor Author

Thanks @xudong963

@adriangb
adriangb added this pull request to the merge queue Sep 21, 2026
Merged via the queue into apache:main with commit 31a4ca0 Sep 21, 2026
41 checks passed
@adriangb
adriangb deleted the na-tests branch September 21, 2026 15:17
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants