kumarUjjawal commented on code in PR #23957:
URL: https://github.com/apache/datafusion/pull/23957#discussion_r3742604536
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -2360,6 +2368,9 @@ async fn collect_left_input(
bounds = None;
}
+ let build_has_null =
Review Comment:
`logical_null_count()` now runs for every hash join, but build_side_has_null
is only used by null-aware RightAnti. For dictionary/run-end keys this can add
an unnecessary O(build rows) pass to unrelated joins. Can we compute it only
for null-aware RightAnti?
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
Review Comment:
The optimizer no longer swaps filtered joins, which fixes the SQL planner
path. However, public HashJoinExec::try_new and protobuf decoding still accept
filtered null-aware RightAnti, where the build-NULL check runs before the
filter. I suggest rejecting that combination?
##########
datafusion/physical-plan/src/joins/hash_join/stream.rs:
##########
@@ -742,40 +742,58 @@ impl HashJoinStream {
let timer = self.join_metrics.join_time.timer();
// Null-aware anti join semantics:
+
// For LeftAnti: output LEFT (build) rows where LEFT.key NOT IN
RIGHT.key
// 1. If RIGHT (probe) contains NULL in any batch, no LEFT rows should
be output
// 2. LEFT rows with NULL keys should not be output (handled in final
stage)
+
+ // For RightAnti: output RIGHT (probe) rows where RIGHT.key NOT IN
LEFT.key
+ // 1. If LEFT (build) contains NULL, no RIGHT rows should be output
+ // 2. RIGHT rows with NULL keys should not be output
+ // 3. If LEFT (build) is empty, all RIGHT rows should be output
if self.null_aware {
- // Mark that we've seen a probe batch with actual rows (probe side
is non-empty)
- // Only set this if batch has rows - empty batches don't count
- // Use shared atomic state so all partitions can see this global
information
- if state.batch.num_rows() > 0 {
- build_side
- .left_data
- .probe_side_non_empty
- .store(true, Ordering::Relaxed);
- }
+ match self.join_type {
+ JoinType::RightAnti => {
+ if build_side.left_data.build_side_has_null {
+ timer.done();
+ self.state = HashJoinStreamState::FetchProbeBatch;
+ return Ok(StatefulStreamResult::Continue);
+ }
+ }
+ JoinType::LeftAnti => {
+ // Mark that we've seen a probe batch with actual rows
(probe side is non-empty)
+ // Only set this if batch has rows - empty batches don't
count
+ // Use shared atomic state so all partitions can see this
global information
+ if state.batch.num_rows() > 0 {
+ build_side
+ .left_data
+ .probe_side_non_empty
+ .store(true, Ordering::Relaxed);
+ }
- // Check if probe side (RIGHT) contains NULL
- // Since null_aware validation ensures single column join, we only
check the first column
- let probe_key_column = &state.values[0];
- if probe_key_column.null_count() > 0 {
- // Found NULL in probe side - set shared flag to prevent any
output
- build_side
- .left_data
- .probe_side_has_null
- .store(true, Ordering::Relaxed);
- }
+ // Check if probe side (RIGHT) contains NULL
+ // Since null_aware validation ensures single column join,
we only check the first column
+ let probe_key_column = &state.values[0];
+ if probe_key_column.logical_null_count() > 0 {
Review Comment:
This changes filtered LeftAnti results. A dictionary/run-end logical NULL is
recorded globally before the JoinFilterm runs. For example, outer (1, A),
subquery (NULL, B), with outer.group = inner.group: the NULL row is rejected,
so theouter row should survive, but this suppresses it. Please revert this
LeftAnti change for this PR, limit it to unfiltered joins, or make NULL
handling filter-aware.
--
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]
