adriangb commented on code in PR #25658:
URL: https://github.com/apache/datafusion/pull/25658#discussion_r4162265418
##########
datafusion/physical-plan/src/joins/hash_join/stream.rs:
##########
@@ -903,11 +915,15 @@ impl HashJoinStream {
&mut self.probe_indices_buffer,
&mut self.build_indices_buffer,
)?;
- (
- UInt64Array::from(self.build_indices_buffer.clone()),
- UInt32Array::from(self.probe_indices_buffer.clone()),
- next_offset,
- )
+ let build_indices: UInt64Array =
+ std::mem::take(&mut self.build_indices_buffer).into();
Review Comment:
Optional: this take-then-give-back of the index buffers repeats the pattern
inside `lookup_join_hashmap`. A small helper that both paths call would keep
the two in step if one of them changes later.
##########
datafusion/physical-plan/src/joins/hash_join/stream.rs:
##########
@@ -1568,6 +1592,7 @@ fn for_each_scope_match(
offset,
probe_indices_buffer,
build_indices_buffer,
+ &mut None,
Review Comment:
This loop calls `lookup_join_hashmap` once per chunk with the same
`build_scope_values` and `probe_scope_values`. Because each call gets a new
`&mut None`, the null-aware scope path still builds the comparator again for
each chunk. Can you move the slot out of the loop so that this path also builds
it once?
```rust
let mut offset = (0, None);
let mut key_comparator = None;
loop {
let (build_indices, probe_indices, next_offset) = lookup_join_hashmap(
// ...
&mut key_comparator,
)?;
```
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -2286,12 +2286,19 @@ pub(crate) fn matchable_join_keys(
}
}
+/// Keeps only the candidate pairs whose join keys are equal.
+///
+/// `comparator` caches the general-path [`JoinKeyComparator`] between calls
+/// that share the same `left_arrays` and `right_arrays`, so its setup cost is
+/// paid once rather than per call. Pass an empty slot whenever either side's
Review Comment:
Nit: nothing enforces this rule, so a caller that keeps the slot across
different arrays gets wrong matches with no error. Can you also say here that
`&mut None` is always safe (it only costs the comparator setup on each call)?
That explains the `&mut None` at the symmetric hash join call site.
--
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]
