fix: re-enable null-equal join dynamic filters with an IS NULL predicate - #23106
fix: re-enable null-equal join dynamic filters with an IS NULL predicate#23106mdashti wants to merge 3 commits into
Conversation
github-actions
Bot
added
sqllogictest
64e1820 to
3721aa9
Compare
|
@adriangb Can you please take a look? |
mdashti
changed the title
3721aa9 to
9f4e40c
Compare
| let any_key_is_null = self | ||
| .on_right | ||
| .iter() | ||
| .filter(|key| key.nullable(&self.probe_schema).unwrap_or(true)) |
There was a problem hiding this 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 ?
There was a problem hiding this 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.
There was a problem hiding this 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 #3238
Though I am not sure what's the consensus for things like these, so a commiter's input would be helpful here.
There was a problem hiding this comment.
That's a fair point. The call sites already return Result, so propagation costs nothing. Done in d090cf5, with a test to check the error path.
mdashti
changed the title
9f4e40c to
9620b97
Compare
9620b97 to
8ecd68f
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23106 +/- ##
=======================================
Coverage 80.88% 80.88%
=======================================
Files 1102 1102
Lines 375813 375870 +57
Branches 375813 375870 +57
=======================================
+ Hits 303966 304021 +55
+ Misses 53745 53741 -4
- Partials 18102 18108 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
build-side predicate prunes a probe-side NULL that can null-match a build-side NULL. Push the filter with `OR key IS NULL` over the nullable probe keys instead, the way apache#23104 does for null-aware anti joins. A NOT NULL key never widens the filter, so an all-NOT-NULL join keeps full selectivity.
The `unwrap_or(true)` widening on an unresolved nullability check wasn't obvious. An extra NULL row is safe; dropping a needed one isn't.
A `nullable` failure means a key out of sync with the probe schema, a construction bug that should surface. The call sites already return `Result`, so nothing new can fail.
164ee1b to
d090cf5
Compare
mdashti
left a comment
mdashti
left a comment
There was a problem hiding this comment.
@RatulDawar Thanks for the review.
@adriangb Can you please take a look?
| let any_key_is_null = self | ||
| .on_right | ||
| .iter() | ||
| .filter(|key| key.nullable(&self.probe_schema).unwrap_or(true)) |
There was a problem hiding this comment.
That's a fair point. The call sites already return Result, so propagation costs nothing. Done in d090cf5, with a test to check the error path.
| 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) |
There was a problem hiding this comment.
Can we add an EXPLAIN or EXPLAIN ANALYZE for this query to show the filter is being applied?
| # 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 |
There was a problem hiding this 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
| // 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 { |
There was a problem hiding this 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?
4 participants
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?
return falseinallow_join_dynamic_filter_pushdown.key IS NULLfor 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 reject unit test flips to assert pushdown is allowed, and
preserve_probe_nullsunit tests cover both the mixed nullable/NOT NULL case (only the nullable key widens) and the all-NOT-NULL case (no widening).Are there any user-facing changes?
Null-equal joins regain dynamic filter pushdown, so they prune the probe scan again while returning correct results.