Repository navigation
fix: reject null-aware LeftAnti hash join in Partitioned mode - #26586
Conversation
A null-aware `LeftAnti` join emits build rows in the final stage, and the
"a probe partition saw a NULL `NOT IN` key" fact is recorded on the build
data (`JoinLeftData::probe_completion`). `PartitionMode::Partitioned`
gives every output partition its own build side, so a NULL probe key -
routed by hash to a single partition - only silenced that partition's
build rows; the others still emitted their unmatched ones. With build
`c1` = 1..8 and probe `c2` = {1, NULL} over four hash partitions the join
returned `[3, 6, 8]` where `CollectLeft` correctly returned nothing.
`NullAwareMode::try_new` already rejected `Partitioned` for the
correlated `LeftAnti` and for `LeftMark`, which share the per-build-row
null bitmap for the same reason. Widen that arm to cover the uncorrelated
`LeftAnti` too, so the whole build-driven family is rejected
consistently.
The DataFusion planner never produced this plan - `physical_planner` does
not repartition null-aware joins and `JoinSelection` pins them to
`CollectLeft` - so the shape was only reachable from hand-built plans and
from protobuf, which decodes the partition mode as written and goes
through the same builder.
`test_null_aware_left_anti_swaps_to_right_anti` loses its `Partitioned`
case: that input plan can no longer be constructed, and the swap it
checks is still covered by the `CollectLeft` and `Auto` cases.
Closes apache#26496
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`statistical_join_selection_subrule`'s `PartitionMode::Partitioned` arm picked `CollectLeft` for a null-aware join before swapping its inputs. That branch is now unreachable: to get there the join must already be `Partitioned`, and `can_swap_hash_join` only lets a null-aware join swap when it is an uncorrelated `LeftAnti` - exactly the combination `NullAwareMode::try_new` now rejects at construction. Its comment named `RightAnti`, which never reached the line either: a null-aware `RightAnti` fails `can_swap_hash_join`, and `RightAnti` with `Partitioned` has always been rejected at construction. The sibling check in `partitioned_hash_join` stays. It runs for joins in `Auto` and `CollectLeft` mode, where a null-aware join is legal, and it is what keeps the planner from building one in `Partitioned` mode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
db6e142 to
19c5fbf
Compare
| if can_swap_hash_join(hash_join) | ||
| && should_swap_join_order(left, right, context)? | ||
| { | ||
| // Null-aware RightAnti only supports CollectLeft | ||
| let partition_mode = if hash_join.null_aware { | ||
| PartitionMode::CollectLeft | ||
| } else { | ||
| PartitionMode::Partitioned | ||
| }; | ||
| hash_join.swap_inputs(partition_mode).map(Some)? | ||
| // A null-aware join is never in `Partitioned` mode | ||
| // (`NullAwareMode::try_new` rejects it), so the swap | ||
| // keeps the mode this arm matched on. | ||
| hash_join | ||
| .swap_inputs(PartitionMode::Partitioned) | ||
| .map(Some)? | ||
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
On the second commit, in case it reads as unrelated cleanup, here is why that
branch cannot be reached.
Entering it requires three things at once:
partition_mode == PartitionMode::Partitioned— it is thatmatcharm;can_swap_hash_join(hash_join)— the enclosingif;hash_join.null_aware— the ternary'strueside.
fn can_swap_hash_join(hash_join: &HashJoinExec) -> bool {
hash_join.join_type().supports_swap()
&& (!hash_join.null_aware
|| (*hash_join.join_type() == JoinType::LeftAnti
&& hash_join.on().len() == 1
&& hash_join.filter().is_none()))
}Given (3), !hash_join.null_aware is false, so (2) can only hold through its
right-hand side: LeftAnti, a single key, no filter. NullAwareMode::try_new
derives correlated = num_keys > 1 || has_filter, so that is exactly
LeftAnti { correlated: false }. With (1) the join is therefore an uncorrelated
null-aware LeftAnti in Partitioned mode — the combination the first commit
rejects in try_new, which HashJoinExec::try_new, the builder and protobuf
decode all go through.
So on that line null_aware is always false and the chosen mode is always
Partitioned: for every input that can reach it, the old ternary and
swap_inputs(PartitionMode::Partitioned) compute the same value. Reverting the
first commit and re-running the crate's tests confirms the construction is what
blocks it, not the optimizer: the only two failures are the new
test_null_aware_validation_left_anti_partitioned and
try_from_proto_rejects_a_null_aware_partitioned_join.
The RightAnti named in the old comment never reached the line either: a
null-aware RightAnti fails can_swap_hash_join, and
(RightAnti, Partitioned) falls through try_new's
match (join_type, partition_mode) to plan_err.
This is not needed for the fix, which is why it is a separate commit — happy to
drop it if you would rather leave the branch in place.
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @cool9850311 👍🏻
## Which issue does this PR close? - None. ## Rationale for this change `HashJoinExec` supports SQL `NOT IN` through `null_aware = true`, in three forms: `LeftAnti`, `LeftMark` and `RightAnti`, each correlated or not. The rules for which rows such a join emits are spread out today: - `exec.rs`: `NullAwareMode`, four `JoinLeftData` fields and one `JoinBuildData` field that only some forms use, and the correlated per-row state built inside the build function. - `stream.rs`: about 700 lines inside the regular join stream. These are the batch-skip decision, the correlated marking pass and its helpers, the RightAnti NULL and UNKNOWN drops, and two final-stage functions. - `utils.rs`: a second copy of the final rule, used for the LeftMark column. The two copies of the final rule already drifted once: LeftMark checked logical NULLs since apache#25693, LeftAnti physical ones until apache#26578. This code changes often. Since July it gained four features (apache#23957, apache#21585, apache#19857, apache#26213) and seven correctness fixes (apache#23104, apache#23173, apache#25076, apache#25339, apache#25693, apache#26586, apache#26578), and apache#26214, apache#25679 and apache#26088 propose more. Each such change has to find the `NOT IN` rules in three files, usually about six functions, interleaved with the regular join. A change to the regular probe loop has to read past them too. This PR moves the rules into `hash_join/not_in.rs`, with one copy of the final rule, so a `NOT IN` change touches one module and a regular-join change doesn't need to read it. The join reaches it at four points: 1. `NotIn::try_new` decides when the plan is built or executed whether `null_aware` is legal for the join, and which form it is: `BuildRows { scope, output: Rows | Mark }` for LeftAnti and LeftMark, or `ProbeRows { scope }` for RightAnti, where `scope` is `Uncorrelated` or `Correlated`. 2. `NotInBuild::try_new` runs once per build. It has one variant per form, holding only that form's state. `JoinLeftData` keeps a single `Option<NotInBuild>`. 3. `NotInProbe::before_lookup` / `after_lookup` run around each probe batch's lookup, and `after_empty_build` replaces `after_lookup` when the build map is empty. They skip the batch, record what the probe saw, mark build rows or this batch's probe rows UNKNOWN, and drop the probe rows a RightAnti must not emit; for other joins they do nothing. 4. `NotInBuild::final_chunk` runs on the build rows emitted at the end. It applies one rule for an unmatched row's mark, used both by LeftAnti (keep the rows whose mark is FALSE) and by LeftMark (the mark column). ## What changes are included in this PR? Six commits, each compiling and passing clippy on its own: 1. **Move the correlated marking helpers into `not_in.rs`.** The code is unchanged, apart from one signature that now takes the build data and probe batch instead of the stream's private state structs. This commit also adds a test for correlated marking when every build value key is NULL. The full-key map is empty then, and the marking must still run before the empty-map shortcut; nothing covered this before. 2. **`NotIn` replaces `NullAwareMode`.** The legality checks, messages and order are the same. 3. **`NotInBuild` holds the per-form build state, with one final rule.** It replaces the `JoinLeftData` / `JoinBuildData` fields, the two final-stage functions in `stream.rs`, and `build_null_aware_left_mark_column` in `utils.rs`. 4. **The `NotInProbe` hooks replace the stream's `NOT IN` branches and fields.** The correlated RightAnti's per-batch UNKNOWN probe rows move from `ProcessProbeBatchState` into `NotInProbe`, which recomputes them on each batch's first lookup. 5. **Review follow-ups.** `HashJoinExec::not_in()` and the `not_in` parameters replace `null_aware_mode()` and `null_aware: Option<NotIn>`, the helpers used only inside `not_in.rs` become private, and `final_chunk` asserts that a probe side with a NULL key is not empty. 6. **Test the correlated RightAnti empty-build path.** `test_null_aware_correlated_all_null_build_values` also runs RightAnti, which must drop the UNKNOWN probe rows when the build map is empty (`NotInProbe::after_empty_build`). Types: `NullAwareMode` goes, and `NotIn`, `NotInScope`, `NotInOutput`, `NotInBuild` and `NotInProbe` come in. Gone: the `NOT IN` fields of `JoinLeftData`, `JoinBuildData`, `HashJoinStream` and `ProcessProbeBatchState` and their constructor parameters, 4 stream functions (`null_aware_skip_probe_batch`, `drop_null_probe_keys`, `null_aware_left_anti_final_indices`, `null_aware_left_mark_column`), and the second copy of the final rule. What does not change: - the planner rules, including the `JoinSelection` swap to RightAnti; - the dynamic-filter gate for null-aware joins; - the prefilter and prepared-build exclusions; - the cross-partition `ProbeCompletion`. ### Behavior notes No query results change. Two internal orders do: - **Reservation order.** For an uncorrelated LeftAnti or LeftMark join whose build key has newly materialized logical NULLs (a dictionary, REE or Null key), the NULL buffer is now charged to the memory reservation right after the visited bitmap. On main it was charged after the membership step's optional pruning bitmap. Under a tight memory limit, a different allocation may be the one that fails. The pruning bitmap only exists with a dynamic filter, which a null-aware join with a nullable build key doesn't get. - **The form is computed twice per execution.** The stream reads it from the build (`JoinLeftData::not_in`); `execute` derives it again (`HashJoinExec::not_in()`) for the dynamic filter. Main derived it three times (build, stream, accumulator). The two can only disagree if an executed plan is rebuilt and run again without `reset_state`, which is already unsupported. Main panicked in that case; this PR uses the build's form. ## Performance Benchmark bot (GKE `c4a-highmem-16`, aarch64) on `a9d4896d7e` against its merge base `702803d022`; the later commit only adds a test: - `null_aware_join`: geomean 0.989. Q01 and Q03 are 4% faster, Q02, Q05 and Q06 about 2% faster, Q08 2% slower (20.4 → 20.8 ms), the rest are within 1%. - `hj`: total 6870 → 6885 ms (+0.2%). Q22 and Q24 are unchanged. The queries the bot flags (Q11, Q14, Q15, Q17 slower; Q16, Q18 faster) move within their run-to-run spread. - `tpch10`: all 22 queries unchanged, total 7078 → 7049 ms. Earlier runs on an M-series Mac showed this refactor slowing `hj` Q24 by up to 9% through different LLVM inlining of the probe loop. That doesn't reproduce on the bot, so the PR leaves inlining to the compiler. Not measured on x86_64. ## What is the testing strategy for this PR? - New test: `test_null_aware_correlated_all_null_build_values`, covering LeftAnti, LeftMark and RightAnti with the probe in two batches, at every batch-size config. - Existing tests: - physical-plan unit tests; - the null-aware join fuzz test (`null_aware_join_fuzz`, with `extended_tests`), which covers uncorrelated and correlated RightAnti plans after the `JoinSelection` swap; - the `null_aware_*` and `subquery*` sqllogictest files, with no changes to expected results or `EXPLAIN` output. ## Are there any user-facing changes? No.
Which issue does this PR close?
NOT INhash join inPartitionedmode returns rows when the subquery contains NULL #26496.Rationale for this change
x NOT IN (SELECT y FROM t)must return no rows at all once the subqueryproduces a NULL: under SQL three-valued logic
x <> NULLis UNKNOWN, so theNOT INpredicate can never be TRUE for anyx.A null-aware
LeftAntihash join got this wrong inPartitionMode::Partitioned. With buildc1= 1..8, probec2= {1, NULL}and four hash partitions on each side, the join returned
where
PartitionMode::CollectLeftcorrectly returned nothing. This reproducesthe numbers in the issue.
The cause is where the "a probe partition saw a NULL
NOT INkey" fact isstored.
LeftAntiis build-driven: its rows are build rows emitted in thefinal stage, after the probe side is exhausted. The flag is therefore recorded
on the build data, in
JoinLeftData::probe_completion, written byJoinLeftData::record_probe_batchand read once throughreport_probe_completed.CollectLeftcollects the build side once, withprobe_threads_countset tothe number of probe partitions, so the flag covers every probe partition. One
NULL anywhere on the probe side suppresses the whole output. Correct.
Partitionedbuilds a separate build side per output partition, withprobe_threads_countof 1. A NULL probe key is routed by hash to exactly onepartition, so it only suppresses that partition's build rows; the other
partitions still emit their unmatched build rows.
NullAwareMode::try_newalready rejectedPartitionedforLeftMarkand forthe correlated
LeftAnti, which depend on the per-build-row null bitmap forthe same structural reason. The uncorrelated
LeftAntiwas the one member ofthat build-driven family still allowed through, which is the bug.
Why reject rather than make
Partitionedcorrectphysical_plannerexcludes null-awarejoins from
can_repartition_join;JoinSelection::partitioned_hash_joinpins a null-aware join to
CollectLeft;enforce_distributiononly rewritesa join that is already
Partitionedand never convertsCollectLeftintoit. So there is no workload that would benefit.
ProbeCompletionout of theper-partition
JoinLeftDatainto exec-level shared state, plus across-partition barrier in the final stage (today only the last partition to
finish gets
Some(summary), while inPartitionedmode every partitionneeds the global answer before emitting). That means touching the atomic
ordering invariants in
probe_completion.rs, which are covered by a loommodel check.
the whole family instead of two.
This is a correctness hole rather than a user-visible regression: with the
planner suppressions above, the shape is only reachable from hand-built plans
and from protobuf decode, which takes the partition mode as written. It is
worth closing anyway because the doc comment on
HashJoinExec::null_awarealready states the invariant ("such joins are pinned to
PartitionMode::CollectLeft") and relies on it for the positional key-orderingconvention. Before this PR that was a planner convention only; now it is
enforced at construction.
What changes are included in this PR?
Commit 1 (
fix):NullAwareMode::try_new: widen the arm that rejectsPartitionedfromLeftMark | LeftAnti { correlated: true }toLeftMark | LeftAnti { .. },and rewrite its comment to name both kinds of shared state. The error
message is the existing string, unchanged.
Commit 2 (
refactor, separable):if hash_join.null_aware { CollectLeft } else { Partitioned }from the
PartitionMode::Partitionedarm ofstatistical_join_selection_subrule. Reaching that line requires the join toalready be
Partitioned, andcan_swap_hash_joinonly lets a null-awarejoin swap when it is an uncorrelated
LeftAntiwith a single key and nofilter - exactly the combination commit 1 rejects at construction. So
null_awareis alwaysfalsethere and the swap keeps the mode the armmatched on. (Its comment named
RightAnti, which never reached the lineeither: a null-aware
RightAntifailscan_swap_hash_join, andRightAntiwith
Partitionedhas always been rejected at construction.)partitioned_hash_joinis deliberately kept. Thatfunction is called from the
AutoandCollectLeftarms, where a null-awarejoin is legal, and it is what keeps the planner from building one in
Partitionedmode in the first place. What this PR changes for it is theconsequence of removing it: previously a silently wrong plan, now a loud
plan_err.Commit 2 is a separate commit so it can be dropped on its own if a reviewer
prefers to keep the branch.
No public API change; no behaviour change for any plan the planner can build.
What is the testing strategy for this PR?
New tests:
test_null_aware_validation_left_anti_partitioned(
physical-plan,joins::hash_join::exec): an uncorrelated null-awareLeftAnti(single key, no filter) withPartitionedmust failHashJoinExec::try_new. Written first and confirmed red on unfixed code -try_newreturnedOkand the test dumped themode: Partitioned, null_aware: trueplan. It sits next to the existingtest_null_aware_validation_partitioned, whose comment notes it usesLeftMark"so the single-key check is skipped and the partition-mode checkis reached"; the
LeftAntiside of that rule was simply untested.try_from_proto_rejects_a_null_aware_partitioned_join(same file,mod proto_tests): the protobuf path is one of the two ways to reach theshape, and decode goes through the same builder, so it is rejected there too.
Mutation check: reverting the one-line
try_newchange and re-runningcargo test -p datafusion-physical-plan --lib --features proto joins::hash_join::execturns exactly those two tests red (591 passed, 2 failed) and nothing else, so
they are the guard rather than incidental coverage. Note the second one needs
--features proto; a defaultcargo test -p datafusion-physical-plandoes notcompile
mod proto_testsat all.Existing tests adjusted, both forced by the fix:
proto_tests::null_aware_round_tripsnow setspartition_mode = CollectLeft. Itsdecodable_node()helper hard-codesPartitioned, and the test then setjoin_type = LeftAntiandnull_aware = true, which is precisely the rejected shape; the node it builtcan no longer be decoded. This failure is the fix working.
test_null_aware_left_anti_swaps_to_right_anti(core,physical_optimizer::join_selection) loses its#[case(PartitionMode::Partitioned)]. The case is not asserting somethingthat changed - its input plan can no longer be constructed, so the rstest
case would fail in
try_newbeforeJoinSelectionever ran. What it checked(a null-aware
LeftAntiswaps to aRightAntiinCollectLeftwith thesmall side on the left and the schema preserved) is still covered by the
CollectLeftandAutocases, which reach the same assertion throughtry_collect_left. A doc comment on the rstest records whyPartitionedisabsent, so it is not helpfully added back later. The construction rejection
it would have exercised is now asserted directly, at the layer that owns it,
by
test_null_aware_validation_left_anti_partitioned.No
.sltcase: SQL cannot build this plan, so there is nothing forsqllogictest to express. The user-visible behaviour is already covered -
null_aware_anti_join.slt"Test 2: NOT IN with NULL in subquery - shouldreturn 0 rows" runs at the harness default
target_partitions = 4and expectsan empty result, and the file also asserts
mode=CollectLeftin a plan test.Full runs (clean worktree, release of this branch):
cargo test -p datafusion-physical-plan --lib joins::- 1528 passedcargo test -p datafusion --test core_integration- 1430 passedcargo test -p datafusion-proto- passedcargo test -p datafusion-physical-plan --lib --features proto joins::hash_join::exec::proto_tests- 19 passedcargo test -p datafusion-physical-optimizer- 37 passedjoinsandsubquery- passCLAUDE.md) - 12,756 passed / 0 failed /8 ignored, sqllogictest 535/535 files. Run both with and without commit 2,
with identical counts.
cargo fmt --allclean,cargo clippy --all-targets --all-features -D warningsclean.
Are there any user-facing changes?
No SQL-level change and no public API change. A
HashJoinExecbuilt by hand,or decoded from protobuf, with
null_aware = true,JoinType::LeftAntiandPartitionMode::Partitionednow fails at plan construction withnull_aware joins require PartitionMode::CollectLeft, got PartitionMode::Partitionedinstead of returning rows thatNOT INmust notreturn. Anyone constructing that plan was getting wrong results, so the error
is the fix.
Out of scope on purpose:
PartitionMode::Autowith a null-aware join is stillaccepted at construction and rejected in
execute(), as it is for every jointype (
Automust be resolved byJoinSelectionbefore execution). And thefallthrough message in
NullAwareMode::try_new("null_aware can only be truefor LeftAnti joins and RightAnti joins with
CollectLeftPartitionMode, orLeftMark joins, got ...") could now be reworded, since the
CollectLeftrequirement is no longer specific to
RightAnti; I left the wording alone to keep this PR single-purpose and can do it here orseparately if you prefer.
🤖 Generated with Claude Code