dwsmith1983 commented on code in PR #25658:
URL: https://github.com/apache/datafusion/pull/25658#discussion_r4165334846
##########
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:
Good catch, done in 31f5ae673: the slot now lives outside the loop, so each
scope lookup builds the comparator once. None of the existing null-aware tests
had a multi-column scope, so I added
`test_null_aware_left_anti_multi_column_scope`, which splits both scope lookups
into several chunks at the smaller batch sizes.
##########
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:
I looked at sharing it, but the two paths hold the buffers for different
spans. `lookup_join_hashmap` takes them and gives them straight back once the
filtered arrays are built, while the array map path hands its arrays out as the
chunk's indices and only reclaims the buffers after the output batch is done. A
shared helper would wrap the two one-line conversions without making either
path shorter, so I left them as they are. Happy to revisit if you feel strongly.
##########
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:
Added in 31f5ae673: the doc now says nothing checks the rule and that `&mut
None` is always correct, at the cost of rebuilding the comparator on each call.
--
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]
