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?
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