Skip to content

refactor: keep the hash join NOT IN rules in one module - #26647

Open
jayzhan211 wants to merge 5 commits into
apache:mainfrom
jayzhan211:hj-not-in/1-move-helpers
Open

jayzhan211 wants to merge 5 commits into
apache:mainfrom
jayzhan211:hj-not-in/1-move-helpers

Conversation

@jayzhan211

@jayzhan211 jayzhan211 commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • None.

Rationale for this change

HashJoinExec supports SQL NOT IN through null_aware = true, in three forms: LeftAnti, LeftMark and RightAnti, each correlated or not. The rules for which rows such a join emits are spread out today:

  • exec.rs: NullAwareMode, four JoinLeftData fields and one JoinBuildData field that only some forms use, and the correlated per-row state built inside the build function.
  • stream.rs: about 520 lines inside the regular join stream. These are the batch-skip decision, the correlated marking pass and its helpers, the RightAnti NULL drop, and two final-stage functions.
  • utils.rs: a second copy of the final rule, used for the LeftMark column.

The two copies of the final rule have already drifted: LeftMark checks logical NULLs since #25693, LeftAnti still checks physical ones (#26568).

This code changes often. Since July it gained three features (#23957, #21585, #19857) and six correctness fixes (#23104, #23173, #25076, #25339, #25693, #26586), and #26213, #26214, #25679, #26088 and #26089 propose more. Each such change has to find the NOT IN rules in three files, usually about six functions, interleaved with the regular join. A change to the regular probe loop has to read past them too.

This PR moves the rules into hash_join/not_in.rs, with one copy of the final rule, so a NOT IN change touches one module and a regular-join change doesn't need to read it. The join reaches it at four points:

  1. NotIn::try_new decides when the plan is built or executed whether null_aware is legal for the join, and which form it is: BuildRows { scope: Uncorrelated | Correlated, output: Rows | Mark }, or ProbeRows for RightAnti.
  2. NotInBuild::try_new runs once per build. It has one variant per form, holding only that form's state. JoinLeftData keeps a single Option<NotInBuild>.
  3. NotInProbe::before_lookup / after_lookup run around each probe batch's lookup. They skip the batch, record what the probe saw, or mark build rows UNKNOWN; for other joins they do nothing.
  4. NotInBuild::final_chunk runs on the build rows emitted at the end. It applies one rule for an unmatched row's mark, used both by LeftAnti (keep the rows whose mark is FALSE) and by LeftMark (the mark column).

What changes are included in this PR?

Five commits, each compiling and passing the tests on its own:

  1. Move the correlated marking helpers into not_in.rs. The code is unchanged, apart from one signature that now takes the build data and probe batch instead of the stream's private state structs. This commit also adds a test for correlated marking when every build value key is NULL. The full-key map is empty then, and the marking must still run before the empty-map shortcut; nothing covered this before.
  2. NotIn replaces NullAwareMode. The legality checks, messages and order are the same.
  3. NotInBuild holds the per-form build state, with one final rule. It replaces the JoinLeftData / JoinBuildData fields, the two final-stage functions in stream.rs, and build_null_aware_left_mark_column in utils.rs.
  4. The NotInProbe hooks replace the stream's NOT IN branches and fields. This commit also keeps ArrayMap::get_matched_indices_with_limit_offset out of line (see Performance).
  5. Keep process_probe_batch inlined into poll_next_impl, as on main (see Performance).

Types: NullAwareMode goes, and NotIn, NotInScope, NotInOutput, NotInBuild and NotInProbe come in. Gone: 11 NOT IN fields and constructor parameters, 4 stream functions, and the second copy of the final rule.

What does not change:

  • the planner rules;
  • the dynamic-filter gate for null-aware joins;
  • the prefilter and prepared-build exclusions;
  • the cross-partition ProbeCompletion.

LeftAnti still checks physical NULLs on the build key, as it does on main; #26578 fixes that separately.

Behavior notes

No query results change. Two internal orders do:

  • Reservation order. For an uncorrelated LeftMark join whose build key has newly materialized logical NULLs (a dictionary, REE or Null key), the NULL buffer is now charged to the memory reservation right after the visited bitmap. On main it was charged after the membership step's optional pruning bitmap. Under a tight memory limit, a different allocation may be the one that fails. The pruning bitmap only exists with a dynamic filter, which a null-aware join with a nullable build key doesn't get.
  • The form is computed twice per execution. The stream reads it from the build (JoinLeftData::not_in); execute derives it again (null_aware_mode()) for the dynamic filter. Main derived it three times (build, stream, accumulator). The two can only disagree if an executed plan is rebuilt and run again without reset_state, which is already unsupported. Main panicked in that case; this PR uses the build's form.

Performance

Measured on an M-series Mac with release LTO builds of this PR and its base, built with the same settings, alternating which side runs first, 3 rounds:

benchmark change / main
null_aware_join (12 queries, criterion) geomean 0.990
hj (25 queries) geomean 0.995; no query slower in every round

With the NOT IN code gone from the probe loop, LLVM inlines the loop differently. That cost up to ~9% on three queries until the last two commits pinned main's code generation:

  • ArrayMap::get_matched_indices_with_limit_offset was inlined into process_probe_batch: hj Q22 (high fanout) 4–6% slower. Commit 4 keeps it out of line (#[inline(never)]).
  • process_probe_batch was no longer inlined into poll_next_impl: hj Q24 6–9% and null_aware_join Q06 4–6% slower. Commit 5 keeps it inlined (#[inline(always)]).

Single queries also move by ±2–3% with code placement alone: main built with and without symbols differs by 2.5% on hj Q24.

What is the testing strategy for this PR?

  • New test: test_null_aware_correlated_all_null_build_values, covering LeftAnti and LeftMark with the probe in two batches, at every batch-size config. It also passes on main.
  • Existing tests:
    • physical-plan unit and integration tests, including proto;
    • core_integration;
    • the join fuzz tests (join_fuzz, null_aware_join_fuzz, with extended_tests);
    • the sqllogictest suite, with no changes to expected results or EXPLAIN output.

Are there any user-facing changes?

No.

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Oct 23, 2026
@jayzhan211 jayzhan211 changed the title refactor: move the NOT IN marking helpers into hash_join/not_in.rs refactor: keep the hash join NOT IN rules in one module Oct 23, 2026
@jayzhan211
jayzhan211 force-pushed the hj-not-in/1-move-helpers branch from caddf57 to 655d848 Compare October 23, 2026 08:20
@github-actions github-actions Bot added optimizer Optimizer rules core Core DataFusion crate labels Oct 23, 2026
@jayzhan211
jayzhan211 marked this pull request as ready for review October 23, 2026 13:27

@viirya viirya left a comment

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.

Thanks, this is a nice consolidation. Having one copy of the final rule removes the kind of drift #26568 came from, and the probe loop is much easier to follow with two hook calls than with the NOT IN branches interleaved. I compared the moved rules with main form by form (legality order, the uncorrelated skip/record, correlated marking before the empty-map return, and the LeftAnti/LeftMark final rules) and they line up.

#26578 has merged since this was opened, so it needs a rebase. That should make NotInBuild::try_new a little simpler, see inline. The description's notes on LeftAnti physical NULLs and on the reservation order will need updating too; the latter now applies to LeftAnti as well.

The new test_null_aware_correlated_all_null_build_values is a good addition. It pins exactly the ordering this refactor moves: before_lookup must run before the empty-map return, or LeftAnti would emit all three rows.

My main question is about the two inlining pins, inline. The rest are small.

} => {
// `LeftMark` uses logical NULLs; `LeftAnti` still uses
// physical ones (#26568).
let key_nulls = match output {

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.

With #26578 merged, LeftAnti uses logical nulls too, so this match output can collapse to keys[0].logical_nulls() and the #26568 comment can go. The reservation check below then covers both outputs the same way.

}
// A probe batch with a NULL key also has rows: `has_null` implies
// `non_empty`.
Self::Uncorrelated { output, .. } if summary.has_null => {

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.

Main's LeftMark rule didn't depend on has_null ⇒ non_empty, but this ordering does: a NULL build key under has_null relies on non_empty being true. It holds today because before_lookup is the only caller of record_probe_batch, but ProbeSideSummary is a plain struct with public fields. Would a debug_assert!(!summary.has_null || summary.non_empty) here be worth it, so a future caller that breaks it fails a test instead of silently changing marks?


// Kept out of line: inlined into `HashJoinStream::process_probe_batch`,
// this loop measured ~4-6% slower on high-fanout probes.
#[inline(never)]

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.

The effects measured for this pin and for the #[inline(always)] on process_probe_batch (4–9%) aren't far above the 2.5% that symbols alone moved Q24, and they were measured on one aarch64 machine. Do they hold on x86_64? If not clearly, I'd lean toward leaving both out of this refactor or moving them to a separate perf PR. They encode one LLVM version's inlining decisions, and the percentages in the comments will go stale as the surrounding code changes. This one is also on a pub method, so it applies to every caller, not just the hash join probe.

/// Updates state to `FetchProbeBatch`
// Kept inlined into `poll_next_impl`: called out of line, the probe
// loop measured ~4-9% slower (`hj` Q24, `null_aware_join` Q06).
#[inline(always)]

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.

Same question as on ArrayMap::get_matched_indices_with_limit_offset: was this measured on x86_64 as well?

/// to `+0.0` for key comparison. The dynamic filters and
/// `build_side_has_null` were derived from the original keys instead.
/// to `+0.0` for key comparison. The dynamic filters and the `NOT IN`
/// state were derived from the original keys instead.

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.

This (and the JoinBuildData::values doc at line 214) now says the NOT IN state was derived from the original keys. The NULL-value masks are, but the correlated NullValueBuildRows::scope_values are filtered from normalized_values. The old wording named only build_side_has_null, which was accurate. Maybe say "the dynamic filters and the NOT IN NULL masks"?

/// After a lookup: [`NotIn::ProbeRows`] drops the probe rows whose key is
/// NULL, since `NULL NOT IN (..)` is never TRUE. `valid_keys` is the probe
/// keys' validity (`matchable_join_keys`), when the batch has one.
pub(super) fn after_lookup(

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.

before_lookup is a &mut self method that dispatches on the build, while this is an associated fn on NotInProbe that takes &NotInBuild and only does anything for ProbeRows. It uses none of NotInProbe's scratch state, so would it read more naturally as a method on NotInBuild, next to final_chunk?


/// Returns the rows where at least one of `value_keys` is NULL, or `None` when
/// there is no such row.
pub(super) fn null_value_key_mask(value_keys: &[ArrayRef]) -> Option<BooleanArray> {

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.

Nit: null_value_key_mask and the NullValueBuildRows fields are only used inside this module now, so they can drop pub(super). Also, HashJoinExec::null_aware_mode() and the null_aware: Option<NotIn> parameters in collect_left_input / SharedBuildAccumulator mix the two names. Renaming them would make it easier to grep for not_in.

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

Labels

core Core DataFusion crate optimizer Optimizer rules physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants