You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mdashti (via GitHub)" <gi...@apache.org> on 2026/06/23 03:08:08 UTC
[PR] Re-enabled null-equal join dynamic filters with an IS NULL predicate. [datafusion]
mdashti opened a new pull request, #23106:
URL: https://github.com/apache/datafusion/pull/23106
> [!NOTE]
> Stacked on #23104, which adds the shared probe-NULL helper this builds on. That one should land first; until it does, this PR's diff includes its two commits.
## Which issue does this close?
Re-enables the dynamic filter that #22965 disabled (#22964), with the proper null-equal semantics.
## Rationale for this change
#22965 disabled hash-join dynamic filter pushdown for null-equal joins: the build-side bounds and membership predicates evaluate to NULL for a probe-side NULL key, so they prune rows that should null-match a build-side NULL. Its description already named the better fix, "generate a predicate with `OR IS NULL`". #23104 does that for null-aware anti joins; this re-enables the null-equal case the same way.
## What changes are included in this PR?
- Revert the null-equal `return false` in `allow_join_dynamic_filter_pushdown`.
- Generalize the shared probe-NULL helper to cover both null-aware (single-key) and null-equal (multi-key) joins: OR `key IS NULL` for every nullable probe key. A NOT NULL key never widens the filter, so an all-NOT-NULL join keeps full selectivity.
## Are these changes tested?
Yes. #22965's SLT now asserts the filter is back on the probe with the result unchanged, plus a multi-key null-equal case. The unit test flips to assert pushdown is allowed.
## Are there any user-facing changes?
Null-equal joins regain dynamic filter pushdown, so they prune the probe scan again while returning correct results.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "mdashti (via GitHub)" <gi...@apache.org>.
mdashti commented on code in PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#discussion_r3707619666
##########
datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs:
##########
@@ -685,12 +699,49 @@ impl SharedBuildAccumulator {
)?) as Arc<dyn PhysicalExpr>
};
- self.dynamic_filter.update(filter_expr)?;
+ self.dynamic_filter
+ .update(self.preserve_probe_nulls(filter_expr))?;
}
}
Ok(())
}
+
+ /// Keeps probe rows with a NULL key when the join semantics need them.
+ ///
+ /// The build-side predicate drops probe rows whose key is NULL. A null-aware anti join
+ /// (`NOT IN`) needs that NULL to reach the join so three-valued logic can collapse the
+ /// result, and a null-equal join needs it to match a build-side NULL. OR-ing `key IS NULL`
+ /// keeps those rows while preserving the filter's selectivity for the rest; the join refines
+ /// whatever the widened filter lets through.
+ fn preserve_probe_nulls(
+ &self,
+ filter_expr: Arc<dyn PhysicalExpr>,
+ ) -> Arc<dyn PhysicalExpr> {
+ if self.null_equality != NullEquality::NullEqualsNull && !self.null_aware {
+ return filter_expr;
+ }
+ // Only a key that can actually be NULL needs the disjunct; a NOT NULL key never widens.
+ // Null-aware joins are single-key; null-equal joins can be multi-key, so OR every nullable
+ // key. If every key is NOT NULL the filter is left untouched, at full selectivity.
+ let any_key_is_null = self
+ .on_right
+ .iter()
+ .filter(|key| key.nullable(&self.probe_schema).unwrap_or(true))
Review Comment:
That's a fair point. The call sites already return `Result`, so propagation costs nothing. Done in d090cf529, with a test to check the error path.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "mdashti (via GitHub)" <gi...@apache.org>.
mdashti commented on code in PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#discussion_r3470181133
##########
datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs:
##########
@@ -685,12 +699,49 @@ impl SharedBuildAccumulator {
)?) as Arc<dyn PhysicalExpr>
};
- self.dynamic_filter.update(filter_expr)?;
+ self.dynamic_filter
+ .update(self.preserve_probe_nulls(filter_expr))?;
}
}
Ok(())
}
+
+ /// Keeps probe rows with a NULL key when the join semantics need them.
+ ///
+ /// The build-side predicate drops probe rows whose key is NULL. A null-aware anti join
+ /// (`NOT IN`) needs that NULL to reach the join so three-valued logic can collapse the
+ /// result, and a null-equal join needs it to match a build-side NULL. OR-ing `key IS NULL`
+ /// keeps those rows while preserving the filter's selectivity for the rest; the join refines
+ /// whatever the widened filter lets through.
+ fn preserve_probe_nulls(
+ &self,
+ filter_expr: Arc<dyn PhysicalExpr>,
+ ) -> Arc<dyn PhysicalExpr> {
+ if self.null_equality != NullEquality::NullEqualsNull && !self.null_aware {
+ return filter_expr;
+ }
+ // Only a key that can actually be NULL needs the disjunct; a NOT NULL key never widens.
+ // Null-aware joins are single-key; null-equal joins can be multi-key, so OR every nullable
+ // key. If every key is NOT NULL the filter is left untouched, at full selectivity.
+ let any_key_is_null = self
+ .on_right
+ .iter()
+ .filter(|key| key.nullable(&self.probe_schema).unwrap_or(true))
Review Comment:
This is a should-never-happen (as you said: keys out of sync with the probe schema), so I kept `unwrap_or(true)` as the safe degradation: over-widening only loses a little selectivity, while `false` could drop a NULL the join needs. Documented it in `9620b97`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "RatulDawar (via GitHub)" <gi...@apache.org>.
RatulDawar commented on code in PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#discussion_r3476185541
##########
datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs:
##########
@@ -685,12 +699,49 @@ impl SharedBuildAccumulator {
)?) as Arc<dyn PhysicalExpr>
};
- self.dynamic_filter.update(filter_expr)?;
+ self.dynamic_filter
+ .update(self.preserve_probe_nulls(filter_expr))?;
}
}
Ok(())
}
+
+ /// Keeps probe rows with a NULL key when the join semantics need them.
+ ///
+ /// The build-side predicate drops probe rows whose key is NULL. A null-aware anti join
+ /// (`NOT IN`) needs that NULL to reach the join so three-valued logic can collapse the
+ /// result, and a null-equal join needs it to match a build-side NULL. OR-ing `key IS NULL`
+ /// keeps those rows while preserving the filter's selectivity for the rest; the join refines
+ /// whatever the widened filter lets through.
+ fn preserve_probe_nulls(
+ &self,
+ filter_expr: Arc<dyn PhysicalExpr>,
+ ) -> Arc<dyn PhysicalExpr> {
+ if self.null_equality != NullEquality::NullEqualsNull && !self.null_aware {
+ return filter_expr;
+ }
+ // Only a key that can actually be NULL needs the disjunct; a NOT NULL key never widens.
+ // Null-aware joins are single-key; null-equal joins can be multi-key, so OR every nullable
+ // key. If every key is NOT NULL the filter is left untouched, at full selectivity.
+ let any_key_is_null = self
+ .on_right
+ .iter()
+ .filter(|key| key.nullable(&self.probe_schema).unwrap_or(true))
Review Comment:
I was thinking how an invalid state if achieved somehow should be handled, instead of silently handling it shouldn't we propagate the error further.
The fail safe check was added here https://github.com/apache/datafusion/pull/3238
Though I am not sure how things like these are handled across the repository, so a commiter's input would be helpful here.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "mdashti (via GitHub)" <gi...@apache.org>.
mdashti commented on code in PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#discussion_r3714939364
##########
datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs:
##########
@@ -692,38 +700,51 @@ impl SharedBuildAccumulator {
};
self.dynamic_filter
- .update(self.null_aware_filter(filter_expr))?;
+ .update(self.preserve_probe_nulls(filter_expr)?)?;
}
}
Ok(())
}
- /// Wraps a pushdown filter so a null-aware anti join keeps its probe-side NULL rows.
+ /// Keeps probe rows with a NULL key when the join semantics need them.
///
- /// The build-side predicate drops probe rows whose key is NULL, but `NOT IN` three-valued
- /// logic needs that NULL to reach the join. OR-ing `probe_key IS NULL` preserves the dynamic
- /// filter's selectivity for non-NULL rows while letting the NULL through.
- fn null_aware_filter(
+ /// The build-side predicate drops probe rows whose key is NULL. A null-aware anti join
+ /// (`NOT IN`) needs that NULL to reach the join so three-valued logic can collapse the
+ /// result, and a null-equal join needs it to match a build-side NULL. OR-ing `key IS NULL`
+ /// keeps those rows while preserving the filter's selectivity for the rest; the join refines
+ /// whatever the widened filter lets through.
+ fn preserve_probe_nulls(
&self,
filter_expr: Arc<dyn PhysicalExpr>,
- ) -> Arc<dyn PhysicalExpr> {
- if !self.null_aware {
- return filter_expr;
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ if self.null_equality != NullEquality::NullEqualsNull && !self.null_aware {
+ return Ok(filter_expr);
+ }
+ // Only a key that can actually be NULL needs the disjunct; a NOT NULL key never widens.
+ // Null-aware joins are single-key; null-equal joins can be multi-key, so OR every nullable
+ // key. If every key is NOT NULL the filter is left untouched, at full selectivity.
+ let mut any_key_is_null: Option<Arc<dyn PhysicalExpr>> = None;
+ for key in &self.on_right {
Review Comment:
Done in 75125ad96. The build report now carries a per-array `null_count` check, and a NULL-free build skips the widening entirely; the new `EXPLAIN ANALYZE` in 95d7aee73 shows that filter with no `IS NULL` and the probe NULL pruned. Two edges: a null-aware anti join keeps the wrap no matter what the build holds, since one probe NULL collapses `NOT IN` for every build row, and a canceled partition counts as holding a NULL because its content is unknown.
##########
datafusion/sqllogictest/test_files/push_down_filter_parquet.slt:
##########
@@ -1066,6 +1066,33 @@ statement ok
drop table nej_probe;
+# Multi-key null-equal join: the IS NULL disjunct covers every nullable key, so a probe row with a
+# NULL in either key still reaches the join and null-matches the build side.
+statement ok
+COPY (SELECT * FROM (VALUES (1, 10), (2, NULL), (NULL, 30)) v(a, b)) TO 'test_files/scratch/push_down_filter_parquet/mnej_probe.parquet' STORED AS PARQUET;
+
+statement ok
+COPY (SELECT * FROM (VALUES (1, 10), (2, NULL)) v(a, b)) TO 'test_files/scratch/push_down_filter_parquet/mnej_build.parquet' STORED AS PARQUET;
+
+statement ok
+CREATE EXTERNAL TABLE mnej_probe STORED AS PARQUET LOCATION 'test_files/scratch/push_down_filter_parquet/mnej_probe.parquet';
+
+statement ok
+CREATE EXTERNAL TABLE mnej_build STORED AS PARQUET LOCATION 'test_files/scratch/push_down_filter_parquet/mnej_build.parquet';
+
+query IIII rowsort
+SELECT mnej_build.a, mnej_build.b, mnej_probe.a, mnej_probe.b FROM mnej_build JOIN mnej_probe ON (mnej_build.a IS NOT DISTINCT FROM mnej_probe.a) AND (mnej_build.b IS NOT DISTINCT FROM mnej_probe.b)
Review Comment:
Added an `EXPLAIN ANALYZE` in 95d7aee73. The populated filter shows the `IS NULL` disjuncts sitting ahead of the bounds and membership checks.
##########
datafusion/sqllogictest/test_files/push_down_filter_parquet.slt:
##########
@@ -1050,14 +1050,14 @@ SELECT nej_build.id, nej_probe.id FROM nej_build JOIN nej_probe ON nej_build.id
11 11
NULL NULL
-# No DynamicFilter predicate may appear on the probe side of a null-equal join
+# The probe side now carries a DynamicFilter for a null-equal join (widened with IS NULL at runtime)
query TT
EXPLAIN SELECT nej_build.id, nej_probe.id FROM nej_build JOIN nej_probe ON nej_build.id IS NOT DISTINCT FROM nej_probe.id
Review Comment:
Added in 95d7aee73. Two parquet files per side plus a zeroed `hash_join_single_partition_threshold`, so the plan runs real `Hash([id], 4)` routing with `RepartitionExec` on both sides. The probe NULL still comes through the partitioned CASE filter.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#discussion_r3714997833
##########
datafusion/sqllogictest/test_files/push_down_filter_parquet.slt:
##########
@@ -1050,14 +1050,14 @@ SELECT nej_build.id, nej_probe.id FROM nej_build JOIN nej_probe ON nej_build.id
11 11
NULL NULL
-# No DynamicFilter predicate may appear on the probe side of a null-equal join
+# The probe side now carries a DynamicFilter for a null-equal join (widened with IS NULL at runtime)
query TT
EXPLAIN SELECT nej_build.id, nej_probe.id FROM nej_build JOIN nej_probe ON nej_build.id IS NOT DISTINCT FROM nej_probe.id
Review Comment:
Can we make this an `EXPLAIN ANALYZE` or add an `EXPLAIN ANALYZE` so we can see the final shape of the dynamic filter?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#issuecomment-5182595262
Thanks for [75125ad](https://github.com/apache/datafusion/pull/23106/commits/75125ad960d9d9cc462275599ef4ed18ea6f84fc)!
Can we add an SLT test for no-null build side keys?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enabled null-equal join dynamic filters with an IS NULL predicate. [datafusion]
Posted by "RatulDawar (via GitHub)" <gi...@apache.org>.
RatulDawar commented on code in PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#discussion_r3462118015
##########
datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs:
##########
@@ -685,12 +699,49 @@ impl SharedBuildAccumulator {
)?) as Arc<dyn PhysicalExpr>
};
- self.dynamic_filter.update(filter_expr)?;
+ self.dynamic_filter
+ .update(self.preserve_probe_nulls(filter_expr))?;
}
}
Ok(())
}
+
+ /// Keeps probe rows with a NULL key when the join semantics need them.
+ ///
+ /// The build-side predicate drops probe rows whose key is NULL. A null-aware anti join
+ /// (`NOT IN`) needs that NULL to reach the join so three-valued logic can collapse the
+ /// result, and a null-equal join needs it to match a build-side NULL. OR-ing `key IS NULL`
+ /// keeps those rows while preserving the filter's selectivity for the rest; the join refines
+ /// whatever the widened filter lets through.
+ fn preserve_probe_nulls(
+ &self,
+ filter_expr: Arc<dyn PhysicalExpr>,
+ ) -> Arc<dyn PhysicalExpr> {
+ if self.null_equality != NullEquality::NullEqualsNull && !self.null_aware {
+ return filter_expr;
+ }
+ // Only a key that can actually be NULL needs the disjunct; a NOT NULL key never widens.
+ // Null-aware joins are single-key; null-equal joins can be multi-key, so OR every nullable
+ // key. If every key is NOT NULL the filter is left untouched, at full selectivity.
+ let any_key_is_null = self
+ .on_right
+ .iter()
+ .filter(|key| key.nullable(&self.probe_schema).unwrap_or(true))
Review Comment:
Should we widen when we are unable to check column nullability ? i.e. unwrap_or(true).
From what I see this can only happen when on_right and schema are out of sync which seems to be an invalid state ?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "mdashti (via GitHub)" <gi...@apache.org>.
mdashti commented on PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#issuecomment-5233023369
@adriangb Thanks. Can you please take another look?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "RatulDawar (via GitHub)" <gi...@apache.org>.
RatulDawar commented on code in PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#discussion_r3476185541
##########
datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs:
##########
@@ -685,12 +699,49 @@ impl SharedBuildAccumulator {
)?) as Arc<dyn PhysicalExpr>
};
- self.dynamic_filter.update(filter_expr)?;
+ self.dynamic_filter
+ .update(self.preserve_probe_nulls(filter_expr))?;
}
}
Ok(())
}
+
+ /// Keeps probe rows with a NULL key when the join semantics need them.
+ ///
+ /// The build-side predicate drops probe rows whose key is NULL. A null-aware anti join
+ /// (`NOT IN`) needs that NULL to reach the join so three-valued logic can collapse the
+ /// result, and a null-equal join needs it to match a build-side NULL. OR-ing `key IS NULL`
+ /// keeps those rows while preserving the filter's selectivity for the rest; the join refines
+ /// whatever the widened filter lets through.
+ fn preserve_probe_nulls(
+ &self,
+ filter_expr: Arc<dyn PhysicalExpr>,
+ ) -> Arc<dyn PhysicalExpr> {
+ if self.null_equality != NullEquality::NullEqualsNull && !self.null_aware {
+ return filter_expr;
+ }
+ // Only a key that can actually be NULL needs the disjunct; a NOT NULL key never widens.
+ // Null-aware joins are single-key; null-equal joins can be multi-key, so OR every nullable
+ // key. If every key is NOT NULL the filter is left untouched, at full selectivity.
+ let any_key_is_null = self
+ .on_right
+ .iter()
+ .filter(|key| key.nullable(&self.probe_schema).unwrap_or(true))
Review Comment:
I was thinking how an invalid state if achieved somehow should be handled, instead of silently handling it shouldn't we propagate the error further.
The fail safe check was added here https://github.com/apache/datafusion/pull/3238
Though I am not sure what's the consensus for things like these, so a commiter's input would be helpful here.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "mdashti (via GitHub)" <gi...@apache.org>.
mdashti commented on code in PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#discussion_r3738121746
##########
datafusion/sqllogictest/test_files/push_down_filter_parquet.slt:
##########
@@ -1050,14 +1050,14 @@ SELECT nej_build.id, nej_probe.id FROM nej_build JOIN nej_probe ON nej_build.id
11 11
NULL NULL
-# No DynamicFilter predicate may appear on the probe side of a null-equal join
+# The probe side now carries a DynamicFilter for a null-equal join (widened with IS NULL at runtime)
query TT
EXPLAIN SELECT nej_build.id, nej_probe.id FROM nej_build JOIN nej_probe ON nej_build.id IS NOT DISTINCT FROM nej_probe.id
Review Comment:
Converted it to `EXPLAIN ANALYZE` in 476de9bcb. The final shape is like `id@0 IS NULL OR id@0 >= 11 AND id@0 <= 11 AND id@0 IN (SET) ([11, NULL])`, with the probe NULL surviving and only the non-matching row pruned.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#discussion_r3708279325
##########
datafusion/sqllogictest/test_files/push_down_filter_parquet.slt:
##########
@@ -1066,6 +1066,33 @@ statement ok
drop table nej_probe;
+# Multi-key null-equal join: the IS NULL disjunct covers every nullable key, so a probe row with a
+# NULL in either key still reaches the join and null-matches the build side.
+statement ok
+COPY (SELECT * FROM (VALUES (1, 10), (2, NULL), (NULL, 30)) v(a, b)) TO 'test_files/scratch/push_down_filter_parquet/mnej_probe.parquet' STORED AS PARQUET;
+
+statement ok
+COPY (SELECT * FROM (VALUES (1, 10), (2, NULL)) v(a, b)) TO 'test_files/scratch/push_down_filter_parquet/mnej_build.parquet' STORED AS PARQUET;
+
+statement ok
+CREATE EXTERNAL TABLE mnej_probe STORED AS PARQUET LOCATION 'test_files/scratch/push_down_filter_parquet/mnej_probe.parquet';
+
+statement ok
+CREATE EXTERNAL TABLE mnej_build STORED AS PARQUET LOCATION 'test_files/scratch/push_down_filter_parquet/mnej_build.parquet';
+
+query IIII rowsort
+SELECT mnej_build.a, mnej_build.b, mnej_probe.a, mnej_probe.b FROM mnej_build JOIN mnej_probe ON (mnej_build.a IS NOT DISTINCT FROM mnej_probe.a) AND (mnej_build.b IS NOT DISTINCT FROM mnej_probe.b)
Review Comment:
Can we add an `EXPLAIN` or `EXPLAIN ANALYZE` for this query to show the filter is being applied?
##########
datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs:
##########
@@ -692,38 +700,51 @@ impl SharedBuildAccumulator {
};
self.dynamic_filter
- .update(self.null_aware_filter(filter_expr))?;
+ .update(self.preserve_probe_nulls(filter_expr)?)?;
}
}
Ok(())
}
- /// Wraps a pushdown filter so a null-aware anti join keeps its probe-side NULL rows.
+ /// Keeps probe rows with a NULL key when the join semantics need them.
///
- /// The build-side predicate drops probe rows whose key is NULL, but `NOT IN` three-valued
- /// logic needs that NULL to reach the join. OR-ing `probe_key IS NULL` preserves the dynamic
- /// filter's selectivity for non-NULL rows while letting the NULL through.
- fn null_aware_filter(
+ /// The build-side predicate drops probe rows whose key is NULL. A null-aware anti join
+ /// (`NOT IN`) needs that NULL to reach the join so three-valued logic can collapse the
+ /// result, and a null-equal join needs it to match a build-side NULL. OR-ing `key IS NULL`
+ /// keeps those rows while preserving the filter's selectivity for the rest; the join refines
+ /// whatever the widened filter lets through.
+ fn preserve_probe_nulls(
&self,
filter_expr: Arc<dyn PhysicalExpr>,
- ) -> Arc<dyn PhysicalExpr> {
- if !self.null_aware {
- return filter_expr;
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ if self.null_equality != NullEquality::NullEqualsNull && !self.null_aware {
+ return Ok(filter_expr);
+ }
+ // Only a key that can actually be NULL needs the disjunct; a NOT NULL key never widens.
+ // Null-aware joins are single-key; null-equal joins can be multi-key, so OR every nullable
+ // key. If every key is NOT NULL the filter is left untouched, at full selectivity.
+ let mut any_key_is_null: Option<Arc<dyn PhysicalExpr>> = None;
+ for key in &self.on_right {
Review Comment:
Could we also track dynamically if any build side rows are actually null and leave the filter unchanged if none of them are null?
##########
datafusion/sqllogictest/test_files/push_down_filter_parquet.slt:
##########
@@ -1050,14 +1050,14 @@ SELECT nej_build.id, nej_probe.id FROM nej_build JOIN nej_probe ON nej_build.id
11 11
NULL NULL
-# No DynamicFilter predicate may appear on the probe side of a null-equal join
+# The probe side now carries a DynamicFilter for a null-equal join (widened with IS NULL at runtime)
query TT
EXPLAIN SELECT nej_build.id, nej_probe.id FROM nej_build JOIN nej_probe ON nej_build.id IS NOT DISTINCT FROM nej_probe.id
Review Comment:
Can we make sure we have some tests that run in partitioned mode? These are all `CollectLeft`. Making multiple files might work, or you can use `set target_partitions`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #23106:
URL: https://github.com/apache/datafusion/pull/23106
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#issuecomment-5234639735
Thanks @mdashti !
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Re-enabled null-equal join dynamic filters with an IS NULL predicate. [datafusion]
Posted by "mdashti (via GitHub)" <gi...@apache.org>.
mdashti commented on PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#issuecomment-4775534284
@adriangb Can you please take a look?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: re-enable null-equal join dynamic filters with an IS NULL predicate [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23106:
URL: https://github.com/apache/datafusion/pull/23106#issuecomment-5111158853
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/23106?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `97.10145%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.70%. Comparing base ([`b0b9dae`](https://app.codecov.io/gh/apache/datafusion/commit/b0b9dae41095780fec5f8c0033ba48c0d81077be?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8ecd68f`](https://app.codecov.io/gh/apache/datafusion/commit/8ecd68fa8d3e17d179ac0b6a6b02d6a988655b37?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/23106?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...physical-plan/src/joins/hash\_join/shared\_bounds.rs](https://app.codecov.io/gh/apache/datafusion/pull/23106?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fjoins%2Fhash_join%2Fshared_bounds.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9qb2lucy9oYXNoX2pvaW4vc2hhcmVkX2JvdW5kcy5ycw==) | 96.96% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/23106?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #23106 +/- ##
=======================================
Coverage 80.69% 80.70%
=======================================
Files 1095 1095
Lines 372529 372603 +74
Branches 372529 372603 +74
=======================================
+ Hits 300626 300716 +90
+ Misses 53942 53916 -26
- Partials 17961 17971 +10
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/23106?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org