Skip to content

Commit 173440e

Browse files
authored
fix: keep null-aware anti-join NULLs in the pushed dynamic filter (#23104)
## Which issue does this close? Closes #23103. ## Rationale for this change A hash join pushes a build-side dynamic filter (`key IN build_keys`) down to the probe scan. For a null-aware anti join (`NOT IN`), that filter drops the probe's NULL rows. But `NOT IN` three-valued logic needs a probe-side NULL to collapse the whole result to zero rows. With the NULL filtered away at the scan, before the join's null-aware check runs, the join returns rows that shouldn't be there. ## What changes are included in this PR? `SharedBuildAccumulator::build_filter` now ORs `probe_key IS NULL` into the pushed predicate when the join is `null_aware`. Non-NULL probe rows still get filtered, so the optimization stays. `HashJoinExec`'s `null_aware` validation already guarantees a single probe key. ## Are these changes tested? Yes. Added a parquet-backed case to `null_aware_anti_join.slt`. The existing cases use in-memory `VALUES`, whose scans never apply the pushed filter, so they passed despite the bug. The new one sets `parquet.pushdown_filters = true` so the filter runs row-level. Without the fix it returns `1, 3`; with it, zero rows. ## Are there any user-facing changes? A `NOT IN` over a NULL-bearing inner now returns zero rows instead of leaking rows, when join dynamic filter pushdown and row-level scan filtering are both on.
1 parent 65f9726 commit 173440e

3 files changed

Lines changed: 104 additions & 3 deletions

File tree

datafusion/physical-plan/src/joins/hash_join/exec.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1405,6 +1405,7 @@ impl ExecutionPlan for HashJoinExec {
14051405
filter,
14061406
on_right,
14071407
repartition_random_state,
1408+
self.null_aware,
14081409
))
14091410
})))
14101411
})

datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ use datafusion_common::{DataFusionError, Result, ScalarValue, SharedResult};
3737
use datafusion_expr::Operator;
3838
use datafusion_functions::core::r#struct as struct_func;
3939
use datafusion_physical_expr::expressions::{
40-
BinaryExpr, CaseExpr, DynamicFilterPhysicalExpr, InListExpr, lit,
40+
BinaryExpr, CaseExpr, DynamicFilterPhysicalExpr, InListExpr, IsNullExpr, lit,
4141
};
4242
use datafusion_physical_expr::{PhysicalExpr, PhysicalExprRef, ScalarFunctionExpr};
4343

@@ -255,6 +255,9 @@ pub(crate) struct SharedBuildAccumulator {
255255
repartition_random_state: SeededRandomState,
256256
/// Schema of the probe (right) side for evaluating filter expressions
257257
probe_schema: Arc<Schema>,
258+
/// Null-aware anti join (`NOT IN`). A probe-side NULL must reach the join so its
259+
/// three-valued logic can collapse the result, so the pushed filter keeps NULL rows.
260+
null_aware: bool,
258261
}
259262

260263
/// Strategy for filter pushdown (decided at collection time)
@@ -358,6 +361,7 @@ impl SharedBuildAccumulator {
358361
dynamic_filter: Arc<DynamicFilterPhysicalExpr>,
359362
on_right: Vec<PhysicalExprRef>,
360363
repartition_random_state: SeededRandomState,
364+
null_aware: bool,
361365
) -> Self {
362366
// Troubleshooting: If partition counts are incorrect, verify this logic matches
363367
// the actual execution pattern in collect_build_side()
@@ -404,6 +408,7 @@ impl SharedBuildAccumulator {
404408
on_right,
405409
repartition_random_state,
406410
probe_schema: right_child.schema(),
411+
null_aware,
407412
}
408413
}
409414

@@ -579,7 +584,8 @@ impl SharedBuildAccumulator {
579584
if let Some(filter_expr) =
580585
combine_membership_and_bounds(membership_expr, bounds_expr)
581586
{
582-
self.dynamic_filter.update(filter_expr)?;
587+
self.dynamic_filter
588+
.update(self.null_aware_filter(filter_expr))?;
583589
}
584590
}
585591
PartitionStatus::Pending => {
@@ -685,12 +691,40 @@ impl SharedBuildAccumulator {
685691
)?) as Arc<dyn PhysicalExpr>
686692
};
687693

688-
self.dynamic_filter.update(filter_expr)?;
694+
self.dynamic_filter
695+
.update(self.null_aware_filter(filter_expr))?;
689696
}
690697
}
691698

692699
Ok(())
693700
}
701+
702+
/// Wraps a pushdown filter so a null-aware anti join keeps its probe-side NULL rows.
703+
///
704+
/// The build-side predicate drops probe rows whose key is NULL, but `NOT IN` three-valued
705+
/// logic needs that NULL to reach the join. OR-ing `probe_key IS NULL` preserves the dynamic
706+
/// filter's selectivity for non-NULL rows while letting the NULL through.
707+
fn null_aware_filter(
708+
&self,
709+
filter_expr: Arc<dyn PhysicalExpr>,
710+
) -> Arc<dyn PhysicalExpr> {
711+
if !self.null_aware {
712+
return filter_expr;
713+
}
714+
debug_assert_eq!(
715+
self.on_right.len(),
716+
1,
717+
"null_aware anti join must have exactly one probe key"
718+
);
719+
let probe_key_is_null: Arc<dyn PhysicalExpr> =
720+
Arc::new(IsNullExpr::new(Arc::clone(&self.on_right[0])));
721+
// Cheap null check first short-circuits before the costlier dynamic filter.
722+
Arc::new(BinaryExpr::new(
723+
probe_key_is_null,
724+
Operator::Or,
725+
filter_expr,
726+
))
727+
}
694728
}
695729

696730
impl fmt::Debug for SharedBuildAccumulator {
@@ -722,6 +756,7 @@ pub(super) fn make_partitioned_accumulator_for_test(
722756
on_right: vec![],
723757
repartition_random_state: SeededRandomState::with_seed(1),
724758
probe_schema,
759+
null_aware: false,
725760
}
726761
}
727762

datafusion/sqllogictest/test_files/null_aware_anti_join.slt

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -451,3 +451,68 @@ DROP TABLE customers_test;
451451

452452
statement ok
453453
DROP TABLE all_null_banned;
454+
455+
#############
456+
## Test: dynamic filter pushdown must not drop inner (probe-side) NULLs.
457+
## With join dynamic filter pushdown on, the build-side filter pushed to the probe scan would drop
458+
## inner NULLs, but NOT IN three-valued logic needs them to collapse the result to zero rows. The
459+
## in-memory VALUES scans above never apply the pushed filter, so this case needs a parquet scan.
460+
#############
461+
462+
statement ok
463+
set datafusion.optimizer.enable_join_dynamic_filter_pushdown = true;
464+
465+
# Row-level parquet filtering, so the pushed filter actually drops matching rows instead of only
466+
# pruning row groups. Without this the single row group is read whole and the NULL never gets dropped.
467+
statement ok
468+
set datafusion.execution.parquet.pushdown_filters = true;
469+
470+
statement ok
471+
CREATE TABLE asa_outer(id INT) AS VALUES (1), (2), (3);
472+
473+
statement ok
474+
CREATE TABLE asa_inner(eid INT) AS VALUES (2), (NULL);
475+
476+
query I
477+
COPY asa_outer TO 'test_files/scratch/null_aware_anti_join/asa_outer.parquet' STORED AS PARQUET;
478+
----
479+
3
480+
481+
query I
482+
COPY asa_inner TO 'test_files/scratch/null_aware_anti_join/asa_inner.parquet' STORED AS PARQUET;
483+
----
484+
2
485+
486+
statement ok
487+
CREATE EXTERNAL TABLE asa_outer_parquet(id INT)
488+
STORED AS PARQUET
489+
LOCATION 'test_files/scratch/null_aware_anti_join/asa_outer.parquet';
490+
491+
statement ok
492+
CREATE EXTERNAL TABLE asa_inner_parquet(eid INT)
493+
STORED AS PARQUET
494+
LOCATION 'test_files/scratch/null_aware_anti_join/asa_inner.parquet';
495+
496+
# Expected: zero rows. Before the fix the pushed dynamic filter dropped inner NULLs, so the join
497+
# wrongly returned id = 1 and id = 3.
498+
query I
499+
SELECT id FROM asa_outer_parquet WHERE id NOT IN (SELECT eid FROM asa_inner_parquet) ORDER BY id;
500+
----
501+
502+
statement ok
503+
DROP TABLE asa_outer;
504+
505+
statement ok
506+
DROP TABLE asa_inner;
507+
508+
statement ok
509+
DROP TABLE asa_outer_parquet;
510+
511+
statement ok
512+
DROP TABLE asa_inner_parquet;
513+
514+
statement ok
515+
RESET datafusion.execution.parquet.pushdown_filters;
516+
517+
statement ok
518+
RESET datafusion.optimizer.enable_join_dynamic_filter_pushdown;

0 commit comments

Comments
 (0)