From 745d8d2ddea05d7713dfd273f938f65d6010634f Mon Sep 17 00:00:00 2001 From: Jiawei Zhao Date: Thu, 18 Jun 2026 22:27:39 +0800 Subject: [PATCH] refactor: name build-row and matchable-map presence checks in hash join `state_after_build_ready` distinguishes two facts about the collected build side: whether it physically has rows, and whether its hash map has any matchable entries. These differ under `NullEquality::NullEqualsNothing`, where build rows with a NULL join key are omitted from the map -- the distinction that was the source of the bug fixed in #22893. Expose it as named methods on `JoinLeftData` (`has_build_rows` / `has_matchable_build_rows`) instead of repeating raw `batch().num_rows()` / `map().is_empty()` checks at the call site, so the invariant is visible at the API surface and future uses are harder to get wrong. No behavior change. Closes #23008 Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Jiawei Zhao --- .../physical-plan/src/joins/hash_join/exec.rs | 19 +++++++++++++++++++ .../src/joins/hash_join/stream.rs | 4 ++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/datafusion/physical-plan/src/joins/hash_join/exec.rs b/datafusion/physical-plan/src/joins/hash_join/exec.rs index 5ef767ebddb16..5b68470c8652c 100644 --- a/datafusion/physical-plan/src/joins/hash_join/exec.rs +++ b/datafusion/physical-plan/src/joins/hash_join/exec.rs @@ -228,6 +228,25 @@ impl JoinLeftData { &self.batch } + /// Returns `true` if the build side physically contains rows. + /// + /// This is distinct from [`Self::has_matchable_build_rows`]: a build side + /// can hold rows while its hash map is empty (see that method). + pub(super) fn has_build_rows(&self) -> bool { + self.batch().num_rows() > 0 + } + + /// Returns `true` if the build-side hash map has any matchable entries. + /// + /// Under [`NullEquality::NullEqualsNothing`] build rows whose join key is + /// NULL are omitted from the map, so this can be `false` even when + /// [`Self::has_build_rows`] is `true`. + /// + /// [`NullEquality::NullEqualsNothing`]: datafusion_common::NullEquality::NullEqualsNothing + pub(super) fn has_matchable_build_rows(&self) -> bool { + !self.map().is_empty() + } + /// returns a reference to the build side expressions values pub(super) fn values(&self) -> &[ArrayRef] { &self.values diff --git a/datafusion/physical-plan/src/joins/hash_join/stream.rs b/datafusion/physical-plan/src/joins/hash_join/stream.rs index ed605301ad4a7..82f9508d872d9 100644 --- a/datafusion/physical-plan/src/joins/hash_join/stream.rs +++ b/datafusion/physical-plan/src/joins/hash_join/stream.rs @@ -523,12 +523,12 @@ impl HashJoinStream { join_type: JoinType, left_data: &JoinLeftData, ) -> HashJoinStreamState { - let build_empty = left_data.batch().num_rows() == 0; + let build_empty = !left_data.has_build_rows(); // The map can be empty even when the build side has rows: under // `NullEqualsNothing`, build rows with a NULL join key are omitted. For // join types whose every output row requires a build match, that still // guarantees an empty result, so we can skip scanning the probe side. - let map_empty = left_data.map().is_empty(); + let map_empty = !left_data.has_matchable_build_rows(); if (build_empty && join_type.empty_build_side_produces_empty_result()) || (map_empty && join_type.empty_map_produces_empty_result())