cool9850311 commented on code in PR #26586:
URL: https://github.com/apache/datafusion/pull/26586#discussion_r4346199384
##########
datafusion/physical-optimizer/src/join_selection.rs:
##########
@@ -319,13 +319,12 @@ fn statistical_join_selection_subrule(
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
}
Review 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:
1. `partition_mode == PartitionMode::Partitioned` — it is that `match` arm;
2. `can_swap_hash_join(hash_join)` — the enclosing `if`;
3. `hash_join.null_aware` — the ternary's `true` side.
```rust
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
