kosiew commented on code in PR #23843:
URL: https://github.com/apache/datafusion/pull/23843#discussion_r3690170084
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -2196,78 +2192,28 @@ pub(super) fn equal_rows_arr(
right_arrays: &[ArrayRef],
null_equality: NullEquality,
) -> Result<(UInt64Array, UInt32Array)> {
- let mut iter = left_arrays.iter().zip(right_arrays.iter());
-
- let Some((first_left, first_right)) = iter.next() else {
+ if left_arrays.is_empty() {
return Ok((Vec::<u64>::new().into(), Vec::<u32>::new().into()));
- };
-
- let arr_left = take(first_left.as_ref(), indices_left, None)?;
- let arr_right = take(first_right.as_ref(), indices_right, None)?;
+ }
- let mut equal: BooleanArray = eq_dyn_null(&arr_left, &arr_right,
null_equality)?;
+ let sort_options = vec![SortOptions::default(); left_arrays.len()];
+ let comparator =
+ JoinKeyComparator::new(left_arrays, right_arrays, &sort_options,
null_equality)?;
- // Use map and try_fold to iterate over the remaining pairs of arrays.
- // In each iteration, take is used on the pair of arrays and their
equality is determined.
- // The results are then folded (combined) using the and function to get a
final equality result.
- equal = iter
- .map(|(left, right)| {
- let arr_left = take(left.as_ref(), indices_left, None)?;
- let arr_right = take(right.as_ref(), indices_right, None)?;
- eq_dyn_null(arr_left.as_ref(), arr_right.as_ref(), null_equality)
- })
- .try_fold(equal, |acc, equal2| and(&acc, &equal2?))?;
+ let mut left_filtered = Vec::with_capacity(indices_left.len());
+ let mut right_filtered = Vec::with_capacity(indices_right.len());
- let filter_builder = FilterBuilder::new(&equal).optimize().build();
+ for (left, right) in
indices_left.values().iter().zip(indices_right.values()) {
Review Comment:
`indices_left` and `indices_right` are expected to be parallel candidate
pair arrays, but this loop now uses `zip`, which would silently truncate if
that invariant were ever broken.
Would it make sense to add a `debug_assert_eq!(indices_left.len(),
indices_right.len())` (or return an internal error) before the loop? That way,
if a future caller accidentally violates the invariant, it will fail loudly
instead of quietly dropping candidate pairs.
--
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]
