kosiew commented on code in PR #22893:
URL: https://github.com/apache/datafusion/pull/22893#discussion_r3425518947
##########
datafusion/physical-plan/src/joins/hash_join/stream.rs:
##########
@@ -516,7 +523,7 @@ impl HashJoinStream {
join_type: JoinType,
left_data: &JoinLeftData,
) -> HashJoinStreamState {
- if left_data.map().is_empty()
+ if left_data.batch().num_rows() == 0
Review Comment:
Nice fix avoiding the incorrect `map.is_empty()` shortcut for outer, anti,
and mark joins.
One small thought: this also gives up a safe early-completion path for join
types where the result is empty when there are no matchable build rows, like
`Inner`, `LeftSemi`, and `RightSemi`. It may be worth encoding that narrower
condition and checking `left_data.map().is_empty()` for those join types, so
all-NULL build keys can still skip scanning the probe side when the output is
guaranteed to be empty.
##########
datafusion/physical-plan/src/joins/hash_join/stream.rs:
##########
@@ -679,6 +686,7 @@ impl HashJoinStream {
// Precalculate hash values for fetched batch
let keys_values =
evaluate_expressions_to_arrays(&self.on_right, &batch)?;
+ let mut valid_keys = None;
Review Comment:
Small clarity nit: this mutable temporary could be scoped just to the
`HashMap` case.
For example, something like `let valid_keys = if let Map::HashMap(_) = ... {
create_hashes(...)?; matchable_join_keys(&keys_values, self.null_equality) }
else { None };` would make the invariant clearer that `valid_keys` only exists
for `HashMap` lookup, without needing mutation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
