kosiew commented on code in PR #26006:
URL: https://github.com/apache/datafusion/pull/26006#discussion_r4345705719
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -5293,4 +5215,165 @@ mod tests {
Inexact(10)
);
}
+
+ #[test]
+ fn test_compare_join_arrays_extended_types() -> Result<()> {
+ use arrow::array::{
+ Decimal256Array, Time32MillisecondArray, Time64NanosecondArray,
+ };
+ let sort_opts = vec![SortOptions::default()];
+
+ // 1. Timezone-aware timestamp
+ let ts_left: ArrayRef = Arc::new(
+ TimestampMicrosecondArray::from(vec![1_000_000, 2_000_000])
+ .with_timezone("UTC"),
+ );
+ let ts_right: ArrayRef = Arc::new(
+ TimestampMicrosecondArray::from(vec![1_000_000, 3_000_000])
+ .with_timezone("UTC"),
+ );
+ assert_eq!(
+ compare_join_arrays(
+ &[Arc::clone(&ts_left)],
+ 0,
+ &[Arc::clone(&ts_right)],
+ 0,
+ &sort_opts,
+ NullEquality::NullEqualsNothing
+ )?,
+ Ordering::Equal
+ );
+ assert_eq!(
+ compare_join_arrays(
+ &[ts_left],
+ 1,
+ &[ts_right],
+ 1,
+ &sort_opts,
+ NullEquality::NullEqualsNothing
+ )?,
+ Ordering::Less
+ );
+
+ // 2. Dictionary(Int32, Utf8)
+ let dict_left: ArrayRef = Arc::new(
Review Comment:
Could we also test equal dictionary values with different codes using
independently ordered dictionaries built with `DictionaryArray::try_new`? The
existing `banana < cherry` check already catches raw-code ordering, but the
equal `apple` values both use code 0, so equality across different encodings
remains untested. Please keep the existing inequality assertion.
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -5293,4 +5215,165 @@ mod tests {
Inexact(10)
);
}
+
+ #[test]
+ fn test_compare_join_arrays_extended_types() -> Result<()> {
Review Comment:
As a non-blocking cleanup, consider collecting the five `(name, left,
right)` array pairs into a small table and running the shared equal/less
assertions in a loop, with the case name in assertion messages. This would
remove repeated comparator calls and make additional type coverage easier to
maintain.
##########
datafusion/physical-plan/src/joins/sort_merge_join/bitwise_stream.rs:
##########
@@ -1216,30 +1216,10 @@ fn keys_match(
null_equality: NullEquality,
) -> Result<bool> {
debug_assert!(left_arrays.iter().all(|a| a.len() == 1));
- if left_arrays
- .iter()
- .any(|array| array.data_type().is_floating())
- {
- // The scalar comparator's partial_cmp panics on NaN. Match the merge
- // scan's floating-point equality, including its signed-zero handling.
- let right_keys = slice_keys(right_arrays, 0);
- return Ok(JoinKeyComparator::new(
- left_arrays,
- &right_keys,
- sort_options,
- null_equality,
- )?
- .is_equal(0, 0));
- }
- let cmp = compare_join_arrays(
- left_arrays,
- 0,
- right_arrays,
- 0,
- sort_options,
- null_equality,
- )?;
- Ok(cmp == Ordering::Equal)
+ let right_keys = slice_keys(right_arrays, 0);
+ let cmp =
+ JoinKeyComparator::new(left_arrays, &right_keys, sort_options,
null_equality)?;
Review Comment:
Could we add a non-blocking execution-level regression using the existing
`join_float_key_batch_boundaries` harness with timezone-aware timestamp keys?
Please cover groups ending and continuing across batch boundaries, asserting
semi/anti row IDs and mark bits, since the current tests exercise the helper
rather than this stream's continuation path.
--
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]
