Skip to content

Commit 03204c0

Browse files
tohuya6neilconway
andauthored
fix: keep the running hash for nested dictionaries in Spark xxhash64 (#25813)
## Which issue does this PR close? - Closes #25799. ## Rationale for this change Spark `xxhash64` returns different results for the same data depending on whether a value inside a struct or list is dictionary-encoded. The dictionary fast path restarted from the default seed, so earlier arguments and earlier list elements had no effect. ## What changes are included in this PR? Ports the fix from apache/datafusion-comet#5757: - The dictionary fast path is used only when every row has the same running hash. - The fast path starts from that running hash instead of 42. ## What is the testing strategy for this PR? Two unit tests ported from Comet, both fail on main: - `test_dictionary_element_in_list_matches_decoded`: a dictionary inside a list hashes the same as its decoded values. - `test_dictionary_with_nonuniform_seeds_matches_decoded`: rows with different running hashes hash the same as the decoded values. The existing Spark hash unit tests and sqllogictests pass. Both queries from the issue now return matching results for dictionary and plain values. ## Are there any user-facing changes? `xxhash64` now matches Spark for dictionary-encoded values inside structs and lists. No API changes. Co-authored-by: tohuya6 <201355151+tohuya6@users.noreply.github.com> Co-authored-by: Neil Conway <neil.conway@gmail.com>
1 parent 4978b30 commit 03204c0

2 files changed

Lines changed: 70 additions & 5 deletions

File tree

‎datafusion/spark/src/function/hash/utils.rs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -898,7 +898,14 @@ macro_rules! create_hashes_internal {
898898
DataType::Decimal128(_, _) => {
899899
$crate::hash_array_decimal!(Decimal128Array, col, $hashes_buffer, $hash_method);
900900
}
901-
DataType::Dictionary(index_type, _) => match **index_type {
901+
DataType::Dictionary(index_type, _) => {
902+
// Only use the fast path if every row starts from the same hash
903+
let first_col = first_col
904+
&& match $hashes_buffer.first() {
905+
None => true,
906+
Some(first) => $hashes_buffer.iter().all(|h| h == first),
907+
};
908+
match **index_type {
902909
DataType::Int8 => {
903910
$create_dictionary_hash_method::<Int8Type>(col, $hashes_buffer, first_col)?;
904911
}
@@ -957,7 +964,8 @@ macro_rules! create_hashes_internal {
957964
col.data_type(),
958965
)))
959966
}
960-
},
967+
}
968+
}
961969
DataType::List(field) => {
962970
let list_array = col.as_any().downcast_ref::<ListArray>().unwrap();
963971
let values = list_array.values();

‎datafusion/spark/src/function/hash/xxhash64.rs‎

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,17 +105,19 @@ fn spark_compatible_xxhash64<T: AsRef<[u8]>>(data: T, seed: u64) -> u64 {
105105
fn create_xxhash64_hashes_dictionary<K: ArrowDictionaryKeyType>(
106106
array: &ArrayRef,
107107
hashes_buffer: &mut [u64],
108-
first_col: bool,
108+
seeds_are_pristine: bool,
109109
) -> Result<()> {
110110
let dict_array = array.as_any().downcast_ref::<DictionaryArray<K>>().unwrap();
111-
if !first_col {
111+
if !seeds_are_pristine {
112112
let unpacked = take(dict_array.values().as_ref(), dict_array.keys(), None)?;
113113
create_xxhash64_hashes(&[unpacked], hashes_buffer)?;
114114
} else {
115115
// Hash each dictionary value once, then look up by key. This avoids
116116
// redundant hashing of large dictionary entries (e.g. long strings).
117117
let dict_values = Arc::clone(dict_array.values());
118-
let mut dict_hashes = vec![DEFAULT_SEED; dict_values.len()];
118+
// Every row has the same running hash here, so start from it instead of 42
119+
let seed = hashes_buffer.first().copied().unwrap_or(DEFAULT_SEED);
120+
let mut dict_hashes = vec![seed; dict_values.len()];
119121
create_xxhash64_hashes(&[dict_values], &mut dict_hashes)?;
120122

121123
for (hash, key) in hashes_buffer.iter_mut().zip(dict_array.keys().iter()) {
@@ -343,6 +345,61 @@ mod tests {
343345
assert_eq!(hashes[4], DEFAULT_SEED);
344346
}
345347

348+
/// A dictionary inside a list keeps the running hash instead of restarting from 42
349+
#[test]
350+
fn test_dictionary_element_in_list_matches_decoded() {
351+
use arrow::array::ListArray;
352+
use arrow::buffer::OffsetBuffer;
353+
use arrow::datatypes::Int8Type;
354+
355+
let values: ArrayRef = Arc::new(Int32Array::from(vec![10, 20]));
356+
let keys = arrow::array::Int8Array::from(vec![0i8, 1]);
357+
let dict: ArrayRef =
358+
Arc::new(DictionaryArray::<Int8Type>::try_new(keys, values).unwrap());
359+
let decoded: ArrayRef = Arc::new(Int32Array::from(vec![10, 20]));
360+
361+
let as_list = |elems: ArrayRef| -> ArrayRef {
362+
Arc::new(ListArray::new(
363+
Arc::new(Field::new("item", elems.data_type().clone(), true)),
364+
OffsetBuffer::new(vec![0i32, 2].into()),
365+
elems,
366+
None,
367+
))
368+
};
369+
370+
let mut from_dict = vec![DEFAULT_SEED; 1];
371+
create_xxhash64_hashes(&[as_list(dict)], &mut from_dict).unwrap();
372+
let mut from_decoded = vec![DEFAULT_SEED; 1];
373+
create_xxhash64_hashes(&[as_list(decoded)], &mut from_decoded).unwrap();
374+
assert_eq!(from_dict, from_decoded);
375+
}
376+
377+
/// Rows with different running hashes skip the fast path
378+
#[test]
379+
fn test_dictionary_with_nonuniform_seeds_matches_decoded() {
380+
use arrow::datatypes::Int8Type;
381+
382+
let values: ArrayRef = Arc::new(Int32Array::from(vec![Some(10), Some(20), None]));
383+
let keys =
384+
arrow::array::Int8Array::from(vec![Some(0), Some(1), Some(2), None, Some(0)]);
385+
let dict: ArrayRef =
386+
Arc::new(DictionaryArray::<Int8Type>::try_new(keys, values).unwrap());
387+
let decoded: ArrayRef = Arc::new(Int32Array::from(vec![
388+
Some(10),
389+
Some(20),
390+
None,
391+
None,
392+
Some(10),
393+
]));
394+
395+
let seeds: Vec<u64> = vec![7, 38, 69, 100, 131];
396+
let mut from_dict = seeds.clone();
397+
create_xxhash64_hashes(&[dict], &mut from_dict).unwrap();
398+
let mut from_decoded = seeds;
399+
create_xxhash64_hashes(&[decoded], &mut from_decoded).unwrap();
400+
assert_eq!(from_dict, from_decoded);
401+
}
402+
346403
#[test]
347404
fn test_xxhash64_dictionary_non_first_column() {
348405
use arrow::array::DictionaryArray;

0 commit comments

Comments
 (0)