Skip to content

fix: re-enable null-equal join dynamic filters with an IS NULL predicate - #23106

Open
mdashti wants to merge 2 commits into
apache:mainfrom
paradedb:moe/null-equal-dynamic-filter
Open

fix: re-enable null-equal join dynamic filters with an IS NULL predicate#23106
mdashti wants to merge 2 commits into
apache:mainfrom
paradedb:moe/null-equal-dynamic-filter

Conversation

@mdashti

@mdashti mdashti commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

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 reject unit test flips to assert pushdown is allowed, and preserve_probe_nulls unit 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.

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Jun 23, 2026
@mdashti
mdashti force-pushed the moe/null-equal-dynamic-filter branch 2 times, most recently from 64e1820 to 3721aa9 Compare June 23, 2026 04:04
@mdashti

mdashti commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

@adriangb Can you please take a look?

@mdashti mdashti changed the title Re-enabled null-equal join dynamic filters with an IS NULL predicate. fix: re-enabled null-equal join dynamic filters with an IS NULL predicate. Jun 23, 2026
@mdashti
mdashti force-pushed the moe/null-equal-dynamic-filter branch from 3721aa9 to 9f4e40c Compare June 23, 2026 04:23
let any_key_is_null = self
.on_right
.iter()
.filter(|key| key.nullable(&self.probe_schema).unwrap_or(true))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RatulDawar RatulDawar Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mdashti mdashti changed the title fix: re-enabled null-equal join dynamic filters with an IS NULL predicate. fix: re-enable null-equal join dynamic filters with an IS NULL predicate Jun 24, 2026
@mdashti
mdashti force-pushed the moe/null-equal-dynamic-filter branch from 9f4e40c to 9620b97 Compare June 24, 2026 20:45
@mdashti
mdashti requested a review from RatulDawar June 24, 2026 21:45
mdashti added 2 commits July 28, 2026 16:40
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.
@mdashti
mdashti force-pushed the moe/null-equal-dynamic-filter branch from 9620b97 to 8ecd68f Compare July 28, 2026 23:44
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.10145% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.70%. Comparing base (b0b9dae) to head (8ecd68f).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...physical-plan/src/joins/hash_join/shared_bounds.rs 96.96% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           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     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants