Repository navigation
refactor: keep the hash join NOT IN rules in one module - #26647
jayzhan211 wants to merge 5 commits into
Conversation
caddf57 to
655d848
Compare
viirya
left a comment
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 => { |
There was a problem hiding this comment.
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)] |
There was a problem hiding this comment.
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)] |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
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.
Which issue does this PR close?
Rationale for this change
HashJoinExecsupports SQLNOT INthroughnull_aware = true, in three forms:LeftAnti,LeftMarkandRightAnti, each correlated or not. The rules for which rows such a join emits are spread out today:exec.rs:NullAwareMode, fourJoinLeftDatafields and oneJoinBuildDatafield 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 INrules 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 aNOT INchange touches one module and a regular-join change doesn't need to read it. The join reaches it at four points:NotIn::try_newdecides when the plan is built or executed whethernull_awareis legal for the join, and which form it is:BuildRows { scope: Uncorrelated | Correlated, output: Rows | Mark }, orProbeRowsfor RightAnti.NotInBuild::try_newruns once per build. It has one variant per form, holding only that form's state.JoinLeftDatakeeps a singleOption<NotInBuild>.NotInProbe::before_lookup/after_lookuprun 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.NotInBuild::final_chunkruns 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:
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.NotInreplacesNullAwareMode. The legality checks, messages and order are the same.NotInBuildholds the per-form build state, with one final rule. It replaces theJoinLeftData/JoinBuildDatafields, the two final-stage functions instream.rs, andbuild_null_aware_left_mark_columninutils.rs.NotInProbehooks replace the stream'sNOT INbranches and fields. This commit also keepsArrayMap::get_matched_indices_with_limit_offsetout of line (see Performance).process_probe_batchinlined intopoll_next_impl, as on main (see Performance).Types:
NullAwareModegoes, andNotIn,NotInScope,NotInOutput,NotInBuildandNotInProbecome in. Gone: 11NOT INfields and constructor parameters, 4 stream functions, and the second copy of the final rule.What does not change:
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:
JoinLeftData::not_in);executederives 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 withoutreset_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:
null_aware_join(12 queries, criterion)hj(25 queries)With the
NOT INcode 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_offsetwas inlined intoprocess_probe_batch:hjQ22 (high fanout) 4–6% slower. Commit 4 keeps it out of line (#[inline(never)]).process_probe_batchwas no longer inlined intopoll_next_impl:hjQ24 6–9% andnull_aware_joinQ06 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
hjQ24.What is the testing strategy for this PR?
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.proto;core_integration;join_fuzz,null_aware_join_fuzz, withextended_tests);EXPLAINoutput.Are there any user-facing changes?
No.