This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-25555-31a4ca07fe1f874a5dec3dc79f4e53b8107a5a20 in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit c09448f8423344665c8b3ee2a85daa6563919828 Author: Liang-Chi Hsieh <[email protected]> AuthorDate: Mon Sep 21 14:26:14 2026 +0000 fix: compare ScalarValue maps using Arrow ordering (#25555) ## Which issue does this PR close? Close #25556 ## Rationale for this change `ScalarValue::partial_cmp` only compared Map keys and ignored their values. Maps with identical keys but different values were therefore considered equal. This could produce incorrect ordering when merging ordered aggregate states, such as `first_value`, `last_value`, and `nth_value`, and could incorrectly validate range partition split points. ## What changes are included in this PR? Use Arrow's Map comparator for `ScalarValue::Map` comparisons. This aligns scalar comparison with Arrow sorting and nested comparison semantics, including values, entry order, lengths, and nulls. Add unit tests covering these cases. ## What is the testing strategy for this PR? Added `test_map_partial_cmp` covering: - maps with identical keys and different values - lexicographic `(key, value)` entry ordering - maps of different lengths - null and empty maps ## Are there any user-facing changes? Map scalar comparisons now consider both keys and values and are consistent with Arrow ordering. --- datafusion/common/src/scalar/mod.rs | 59 ++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 18 deletions(-) diff --git a/datafusion/common/src/scalar/mod.rs b/datafusion/common/src/scalar/mod.rs index 34c9f9e03d..37ae21d90d 100644 --- a/datafusion/common/src/scalar/mod.rs +++ b/datafusion/common/src/scalar/mod.rs @@ -74,7 +74,7 @@ use arrow::array::{ Time32SecondArray, Time64MicrosecondArray, Time64NanosecondArray, TimestampMicrosecondArray, TimestampMillisecondArray, TimestampNanosecondArray, TimestampSecondArray, UInt8Array, UInt16Array, UInt32Array, UInt64Array, UnionArray, - downcast_run_array, new_empty_array, new_null_array, + downcast_run_array, make_comparator, new_empty_array, new_null_array, }; use arrow::buffer::{BooleanBuffer, ScalarBuffer}; use arrow::compute::kernels::cast::{CastOptions, cast_with_options}; @@ -920,23 +920,11 @@ fn partial_cmp_map(m1: &Arc<MapArray>, m2: &Arc<MapArray>) -> Option<Ordering> { return None; } - for col_index in 0..m1.len() { - let arr1 = m1.entries().column(col_index); - let arr2 = m2.entries().column(col_index); - - let lt_res = arrow::compute::kernels::cmp::lt(arr1, arr2).ok()?; - let eq_res = arrow::compute::kernels::cmp::eq(arr1, arr2).ok()?; - - for j in 0..lt_res.len() { - if lt_res.is_valid(j) && lt_res.value(j) { - return Some(Ordering::Less); - } - if eq_res.is_valid(j) && !eq_res.value(j) { - return Some(Ordering::Greater); - } - } - } - Some(Ordering::Equal) + // Keep ScalarValue ordering consistent with Arrow sorting and nested comparison. + // Maps compare lexicographically by entry (key, value), including offsets and nulls. + let comparator = + make_comparator(m1.as_ref(), m2.as_ref(), Default::default()).ok()?; + Some(comparator(0, 0)) } impl Eq for ScalarValue {} @@ -7424,6 +7412,41 @@ mod tests { assert_eq!(a.partial_cmp(&b), Some(Ordering::Greater)); } + #[test] + fn test_map_partial_cmp() { + fn map(entries: Option<Vec<(&str, Option<i32>)>>) -> ScalarValue { + ScalarValue::Map(Arc::new(MapArray::from_vec_of_maps::< + StringArray, + Int32Array, + _, + _, + >(vec![entries], false))) + } + + let a1 = map(Some(vec![("a", Some(1))])); + let a2 = map(Some(vec![("a", Some(2))])); + assert_eq!(a1.partial_cmp(&a2), Some(Ordering::Less)); + assert_eq!(a2.partial_cmp(&a1), Some(Ordering::Greater)); + + // Map entries compare as (key, value) pairs, rather than comparing all keys + // before all values. The first pair therefore determines this ordering. + let high_first_value = map(Some(vec![("a", Some(100)), ("b", Some(1))])); + let low_first_value = map(Some(vec![("a", Some(1)), ("c", Some(999))])); + assert_eq!( + high_first_value.partial_cmp(&low_first_value), + Some(Ordering::Greater) + ); + + let prefix = map(Some(vec![("a", Some(1))])); + let longer = map(Some(vec![("a", Some(1)), ("b", Some(2))])); + assert_eq!(prefix.partial_cmp(&longer), Some(Ordering::Less)); + + let null = map(None); + let empty = map(Some(vec![])); + assert_eq!(null.partial_cmp(&empty), Some(Ordering::Less)); + assert_eq!(empty.partial_cmp(&empty), Some(Ordering::Equal)); + } + #[test] fn scalar_value_to_array_u64() -> Result<()> { let value = ScalarValue::UInt64(Some(13u64)); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
