Skip to content

refactor: simplify null-aware and join-type branching in hash join - #24957

Merged
jayzhan211 merged 1 commit into
apache:mainfrom
jayzhan211:join-refactor
Sep 6, 2026
Merged

jayzhan211 merged 1 commit into
apache:mainfrom
jayzhan211:join-refactor

Conversation

@jayzhan211

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • N/A

Rationale for this change

HashJoinExec carries a null_aware: bool that is only legal for three
(join_type, partition_mode) combinations. Because the flag and the join type
were tracked separately, every use site had to re-derive which combination it
was in, with conditions like self.null_aware && self.join_type == JoinType::LeftAnti
scattered across the builder and the stream. process_probe_batch and
process_unmatched_build_batch were interrupted five times by such blocks, so
the main probe path was hard to follow, and the builder validated the legal
combinations with a five-clause hand-written truth table.

Separately, several predicates over JoinType were spelled as hand-maintained
lists of variants (maintains_input_order, the EmissionType match, the
swap-inputs projection check). Each list has to be revisited whenever a variant
is added, as happened when RightMark landed, and none of them says why a
variant belongs in the list.

What changes are included in this PR?

Two related cleanups, no behavior change:

  1. NullAwareMode enum (hash_join/exec.rs, hash_join/stream.rs)

    • New pub(super) enum NullAwareMode { LeftAnti, RightAnti, LeftMark { correlated: bool } },
      with a single try_new holding the whole legality table. The builder's
      validation becomes one call; the error messages are byte-identical.
    • HashJoinExec::null_aware stays a public bool, so there is no API change.
      The mode is derived once in execute and passed down.
    • collect_left_input takes one Option<NullAwareMode> instead of two
      separately derived booleans, which also lets a now-unfulfilled
      clippy::fn_params_excessive_bools expectation be dropped.
    • The null-aware logic in the stream moves into four named helpers
      (null_aware_skip_probe_batch, drop_null_probe_keys,
      null_aware_left_anti_final_indices, null_aware_left_mark_column), each
      documenting the three-valued-logic rule it implements. The probe path now
      reads top to bottom, and the final stage is a single match on the mode.
  2. Named predicates instead of variant lists (joins/utils.rs)

    • emits_unmatched_left_rows drives the EmissionType decision.
    • is_existence_join drives the swap-inputs projection check.
    • maintains_input_order becomes !need_produce_result_in_final(join_type),
      whose complement was exactly the old hand-written list, with a comment
      explaining that emitting rows from the build-side bitmap breaks probe order.
    • lr_is_preserved is left alone; it already reads as a clear truth table.

Note for reviewers: emits_unmatched_left_rows deliberately excludes LeftSemi
so the EmissionType result is unchanged, even though the hash join does emit
LeftSemi rows from the bitmap in the final stage. That inconsistency is
pre-existing and out of scope here.

What is the testing strategy for this PR?

No new tests: the change is behavior-preserving, so the value is in the existing
coverage continuing to pass.

  • cargo test -p datafusion-physical-plan --lib hash_join
  • sqllogictest files null_aware_anti_join.slt, null_aware_mark_join.slt,
    joins.slt, subquery.slt
  • cargo clippy --all-targets --all-features -- -D warnings and cargo fmt --all

The builder validation tests in hash_join/exec.rs already assert on the exact
null-aware error strings, which are unchanged, so they cover NullAwareMode::try_new.

Are there any user-facing changes?

No. No public API changes, and EXPLAIN output is unchanged. The three
JoinType predicates were each verified to cover exactly the same set of
variants as the lists they replace.

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Sep 6, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.68786% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.62%. Comparing base (406c0c6) to head (da55917).
⚠️ Report is 34 commits behind head on main.

Files with missing lines Patch % Lines
...fusion/physical-plan/src/joins/hash_join/stream.rs 97.47% 2 Missing and 1 partial ⚠️
...tafusion/physical-plan/src/joins/hash_join/exec.rs 97.82% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24957      +/-   ##
==========================================
- Coverage   81.62%   81.62%   -0.01%     
==========================================
  Files        1123     1124       +1     
  Lines      409524   412193    +2669     
  Branches   409524   412193    +2669     
==========================================
+ Hits       334294   336437    +2143     
- Misses      55594    55939     +345     
- Partials    19636    19817     +181     

☔ 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.

JoinType::Inner
| JoinType::LeftSemi
| JoinType::RightSemi
| JoinType::Right

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

here says everything not emitting unmatched build rows is emitted incrementally. LeftSemi emits matched build rows from the final bitmap, not incrementally.

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.

Good catch! I'll open an issue or a PR for this.

@jayzhan211
jayzhan211 added this pull request to the merge queue Sep 6, 2026
Merged via the queue into apache:main with commit 3266eaa Sep 6, 2026
41 checks passed
@jayzhan211
jayzhan211 deleted the join-refactor branch September 6, 2026 06:10
discord9 added a commit to GreptimeTeam/datafusion that referenced this pull request Nov 10, 2026
…tion saw NULL

RE-IMPLEMENTATION, not a faithful cherry-pick. apache#25076
(5747e87) does not apply here: this fork's base
predates the null-aware branching refactor (apache#24957, `NullAwareMode`) and the
chunked final emission (apache#25028, `EmitUnmatchedBuildRowsState`), and
`git show 5747e87 | git apply --check` fails on `exec.rs`, `stream.rs` and the
three manifests. The semantics below are the upstream ones; the shape is this
fork's.

The bug: the final stage read the shared "probe side saw NULL" flag and only
then decremented the probe-partition counter, so a partition could read
`false`, let a sibling record a NULL and finish, then become the last partition
itself and emit build rows that `NOT IN` must suppress. The counter also
decremented with `Relaxed`, so nothing ordered a sibling's store before the
last partition's load.

Changes:

- New `hash_join/probe_completion.rs`, ported nearly verbatim: `ProbeCompletion`
  owns the probe-partition counter and the two shared flags,
  `report_completed()` decrements with `AcqRel` and returns a
  `ProbeSideSummary` only to the last partition, so the final stage cannot read
  the flags before its own decrement. Its sequential unit tests and the
  `--cfg datafusion_loom` model-checked tests come along with the workspace
  `loom` dependency and the `cfg(datafusion_loom)` check-cfg registration.
- `JoinLeftData` holds one `ProbeCompletion` instead of
  `probe_threads_counter`/`probe_side_non_empty`/`probe_side_has_null`. The
  probe phase records through `record_probe_batch()` and keeps a best-effort
  `probe_side_has_null_hint()` early exit.
- `process_unmatched_build_batch` decrements first and hands the resulting
  summary to the new `null_aware_left_anti_final_indices`, which holds the
  `LeftAnti` rules: a probe NULL seen by any partition suppresses every build
  row; otherwise a NULL build key is dropped unless the probe side was empty.
- This fork has no null-aware `LeftMark`, so the mark half of the upstream PR
  has no counterpart here.

Tests (`hash_join/exec.rs`):
- `test_null_aware_anti_join_probe_null_in_other_partition`: two probe
  partitions, only partition 0 sees the NULL, drained in both finishing orders;
  no rows may be emitted.
- `test_null_aware_anti_join_probe_logical_null_in_other_partition`: the same
  with a dictionary key whose NULL is logical only.
- `test_null_aware_anti_join_empty_probe_keeps_null_build_key`: pins
  `NULL NOT IN (empty) = TRUE` (added here; upstream covers it via slt only).

(backport of apache#25076, 5747e87)

Signed-off-by: discord9 <[email protected]>
(cherry picked from commit 3fb9a3084e39781b90d5dbffa01f2664f64dcc2d)
discord9 added a commit to GreptimeTeam/datafusion that referenced this pull request Nov 10, 2026
…results

RE-IMPLEMENTATION, not a faithful cherry-pick. apache#25339
(95bb0a0) builds on the correlated null-aware
machinery of apache#21585/apache#24957 (correlation scope maps, `NullAwareMode`,
`null_indices_bitmap`, shared null-mark buffers), none of which exists here;
`git show 95bb0a0 | git apply --check` fails on `exec.rs`, `stream.rs` and
`join_selection.rs`, and it also edits benchmark files this fork does not have.

The bug: a correlated `NOT IN` whose correlation cannot become an equi-join key
leaves a residual join filter, and the executor ignored that filter when
deciding whether a NULL on the subquery side makes `NOT IN` UNKNOWN, so a NULL
the filter excludes still poisoned every outer row.

Same semantics for the shapes this fork can plan (`on[0]` is the `NOT IN` value
key and the residual correlation lives in the join filter; the null-aware
validation still requires a single join key, where upstream also accepted
`on[1..]` scope keys):

- `JoinLeftData` gains the shared `null_indices_bitmap` of build rows whose
  `NOT IN` is UNKNOWN, allocated with the build side for correlated joins.
- A correlated null-aware join (one with a join filter) no longer lets a probe
  NULL poison the whole join: `mark_null_candidates_for_probe_batch` records,
  per build row, whether a NULL that the residual filter keeps makes `NOT IN`
  UNKNOWN - case 1, a NULL build key with a probe row that passes the filter;
  case 2, a probe row with a NULL key that passes the filter. Without scope
  keys both cases are cross products, already-marked build rows are skipped,
  and the filter is not re-evaluated for them.
- The final stage drops the marked build rows instead of consulting the
  probe-side flags, so outer rows whose subquery is empty keep
  `NOT IN (empty) = TRUE`.
- The uncorrelated probe-side NULL check now uses logical nulls too, so
  dictionary keys are covered on both paths.

Tests:
- `hash_join/exec.rs`: `test_null_aware_left_anti_filter_only` (a `LeftAnti`
  with a second-column `>` filter and no scope keys; only the rows with an
  empty subquery result are emitted), from the upstream PR.
- `sqllogictest/test_files/null_aware_anti_join.slt`: new correlated-residual
  section - the query from the upstream issue, an `EXPLAIN` assertion that the
  shape stays `LeftAnti ... filter=..., null_aware` on a single equi key, and
  the case where the residual leaves the subquery empty for every outer row.

(backport of apache#25339, 95bb0a0)

Signed-off-by: discord9 <[email protected]>
(cherry picked from commit 163cc1d5fb8a8565825a13ebfc82c010bc8750fa)
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 v56.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants