Repository navigation
refactor: keep the hash join NOT IN rules in one module - #26647
Conversation
jayzhan211
changed the title
caddf57 to
655d848
Compare
viirya
left a comment
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.
There was a problem hiding this comment.
I only measured these on aarch64, so I've left both pins out. The Performance section of the description now says what they recovered there and that it isn't measured on x86_64. If one turns out to be worth keeping, I'll send it as its own PR.
There was a problem hiding this comment.
Thanks for taking them out. The bot's hj run on a9d4896 shows Q22 and Q24 unchanged (both slightly faster at the minimum), and the few flagged queries (Q11/Q14/Q15/Q17 slower, Q16/Q18 faster) look like run-to-run variance given their stddevs; tpch10 is flat. So the earlier regression doesn't reproduce on this aarch64 runner without the pins. Once null_aware_join is in too, this looks good to me without them.
There was a problem hiding this comment.
null_aware_join is in as well: 11 of 12 queries are unchanged or faster. Q08 is 2% slower (20.4 → 20.8 ms), but Q06, with the same NULL fraction and no scope key, is 2% faster, and Q08's marking path is moved unchanged, so I read it as code placement. I'm fine leaving the pins out.
| /// 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?
There was a problem hiding this comment.
After the rebase onto #26213, after_lookup also drops the probe rows that before_lookup marked UNKNOWN for a correlated RightAnti. That bitmap is per-batch scratch, so it lives in NotInProbe, and after_lookup is now a &self method there that uses it.
There was a problem hiding this comment.
Makes sense. With unknown_probe_rows living in NotInProbe, keeping it there reads fine.
|
|
||
| /// 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.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @jayzhan211 and @viirya
…ssert probe summary
c0d59c7 to
a9d4896
Compare
|
run benchmark hj null_aware_join tpch10 |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing hj-not-in/1-move-helpers (a9d4896) to 702803d (merge-base) diff Run configurationrun benchmark hjResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing hj-not-in/1-move-helpers (a9d4896) to 702803d (merge-base) diff Run configurationrun benchmark tpch10Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing hj-not-in/1-move-helpers (a9d4896) to 702803d (merge-base) diff Run configurationrun benchmark null_aware_joinResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing hj-not-in/1-move-helpers (a9d4896) to 702803d (merge-base) diff Run configurationrun benchmark hjCPU Details (lscpu)Details
Resource Usagehj — base (merge-base)
hj — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing hj-not-in/1-move-helpers (a9d4896) to 702803d (merge-base) diff Run configurationrun benchmark tpch10CPU Details (lscpu)Details
Resource Usagetpch10 — base (merge-base)
tpch10 — branch
File an issue against this benchmark runner |
| ]; | ||
|
|
||
| let mut results = vec![]; | ||
| for join_type in [JoinType::LeftAnti, JoinType::LeftMark] { |
There was a problem hiding this comment.
after_empty_build is new here and only matters for a correlated RightAnti, which this loop doesn't run. Would it be worth adding JoinType::RightAnti (with the all-NULL table still as the build side) and one more probe row in an empty scope, e.g. (9, 4)? That leaves the LeftAnti/LeftMark expectations unchanged, and RightAnti should emit only (9, 4). If after_empty_build stopped filtering, (5, 1) and (7, 2) would come back. As far as I can tell only the fuzz test reaches that path today.
There was a problem hiding this comment.
Thanks. Putting (9, 4) in the same batch as (7, 2) is better than what I suggested: it also checks that the empty-build path drops rows, not whole batches.
|
|
||
| // 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.
Thanks for taking them out. The bot's hj run on a9d4896 shows Q22 and Q24 unchanged (both slightly faster at the minimum), and the few flagged queries (Q11/Q14/Q15/Q17 slower, Q16/Q18 faster) look like run-to-run variance given their stddevs; tpch10 is flat. So the earlier regression doesn't reproduce on this aarch64 runner without the pins. Once null_aware_join is in too, this looks good to me without them.
| /// 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.
Makes sense. With unknown_probe_rows living in NotInProbe, keeping it there reads fine.
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing hj-not-in/1-move-helpers (a9d4896) to 702803d (merge-base) diff Run configurationrun benchmark null_aware_joinCPU Details (lscpu)Details
Resource Usagenull_aware_join — base (merge-base)
null_aware_join — branch
File an issue against this benchmark runner |
viirya
left a comment
viirya
left a comment
There was a problem hiding this comment.
Thanks for the follow-ups, LGTM.
| ]; | ||
|
|
||
| let mut results = vec![]; | ||
| for join_type in [JoinType::LeftAnti, JoinType::LeftMark] { |
There was a problem hiding this comment.
Thanks. Putting (9, 4) in the same batch as (7, 2) is better than what I suggested: it also checks that the empty-build path drops rows, not whole batches.
|
|
||
| // 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.
null_aware_join is in as well: 11 of 12 queries are unchanged or faster. Q08 is 2% slower (20.4 → 20.8 ms), but Q06, with the same NULL fraction and no scope key, is 2% faster, and Q08's marking path is moved unchanged, so I read it as code placement. I'm fine leaving the pins out.
|
Thanks for getting this done folks, sorry I couldn't get to it! |
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 700 lines inside the regular join stream. These are the batch-skip decision, the correlated marking pass and its helpers, the RightAnti NULL and UNKNOWN drops, 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 already drifted once: LeftMark checked logical NULLs since #25693, LeftAnti physical ones until #26578.
This code changes often. Since July it gained four features (#23957, #21585, #19857, #26213) and seven correctness fixes (#23104, #23173, #25076, #25339, #25693, #26586, #26578), and #26214, #25679 and #26088 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, output: Rows | Mark }for LeftAnti and LeftMark, orProbeRows { scope }for RightAnti, wherescopeisUncorrelatedorCorrelated.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, andafter_empty_buildreplacesafter_lookupwhen the build map is empty. They skip the batch, record what the probe saw, mark build rows or this batch's probe rows UNKNOWN, and drop the probe rows a RightAnti must not emit; 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?
Six commits, each compiling and passing clippy 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. The correlated RightAnti's per-batch UNKNOWN probe rows move fromProcessProbeBatchStateintoNotInProbe, which recomputes them on each batch's first lookup.HashJoinExec::not_in()and thenot_inparameters replacenull_aware_mode()andnull_aware: Option<NotIn>, the helpers used only insidenot_in.rsbecome private, andfinal_chunkasserts that a probe side with a NULL key is not empty.test_null_aware_correlated_all_null_build_valuesalso runs RightAnti, which must drop the UNKNOWN probe rows when the build map is empty (NotInProbe::after_empty_build).Types:
NullAwareModegoes, andNotIn,NotInScope,NotInOutput,NotInBuildandNotInProbecome in. Gone: theNOT INfields ofJoinLeftData,JoinBuildData,HashJoinStreamandProcessProbeBatchStateand their constructor parameters, 4 stream functions (null_aware_skip_probe_batch,drop_null_probe_keys,null_aware_left_anti_final_indices,null_aware_left_mark_column), and the second copy of the final rule.What does not change:
JoinSelectionswap to RightAnti;ProbeCompletion.Behavior notes
No query results change. Two internal orders do:
JoinLeftData::not_in);executederives it again (HashJoinExec::not_in()) 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
Benchmark bot (GKE
c4a-highmem-16, aarch64) ona9d4896d7eagainst its merge base702803d022; the later commit only adds a test:null_aware_join: geomean 0.989. Q01 and Q03 are 4% faster, Q02, Q05 and Q06 about 2% faster, Q08 2% slower (20.4 → 20.8 ms), the rest are within 1%.hj: total 6870 → 6885 ms (+0.2%). Q22 and Q24 are unchanged. The queries the bot flags (Q11, Q14, Q15, Q17 slower; Q16, Q18 faster) move within their run-to-run spread.tpch10: all 22 queries unchanged, total 7078 → 7049 ms.Earlier runs on an M-series Mac showed this refactor slowing
hjQ24 by up to 9% through different LLVM inlining of the probe loop. That doesn't reproduce on the bot, so the PR leaves inlining to the compiler. Not measured on x86_64.What is the testing strategy for this PR?
test_null_aware_correlated_all_null_build_values, covering LeftAnti, LeftMark and RightAnti with the probe in two batches, at every batch-size config.null_aware_join_fuzz, withextended_tests), which covers uncorrelated and correlated RightAnti plans after theJoinSelectionswap;null_aware_*andsubquery*sqllogictest files, with no changes to expected results orEXPLAINoutput.Are there any user-facing changes?
No.