From a76b7ddd24ef229b638c4b5aecf828f31f4f2fa1 Mon Sep 17 00:00:00 2001 From: RIchard Baah Date: Fri, 9 Oct 2026 15:29:08 -0400 Subject: [PATCH 1/6] perf: speed up DictionaryArray hashing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rework hash_dictionary_scatter so the key-null handling runs in 64-bit BitChunks instead of per-row Option unwrap, and route all-valid chunks plus the no-null-keys fast path through an 8-way unrolled dense scatter that keeps the dict gather pipeline full. Mixed chunks use a branchless csel blend (safe-clamped indices) when dict values are all valid; the rarer HAS_NULL_VALUES mixed path falls back to the per-bit scan for correctness. Benchmarks (datafusion/common/benches/with_hashes.rs, Apple M4 Max): dictionary_utf8_int32 single, no nulls : 2.91 µs -> 1.83 µs (-37%) dictionary_utf8_int32 multiple, no nulls: 9.10 µs -> 5.67 µs (-38%) dictionary_utf8_int32 single, nulls : 7.11 µs -> 2.81 µs (-60%) dictionary_utf8_int32 multiple, nulls : 20.90 µs -> 8.21 µs (-61%) Non-dict benchmarks (int64, utf8_view small) stay within noise. --- datafusion/common/src/hash_utils.rs | 226 +++++++++++++++++++++++++--- 1 file changed, 206 insertions(+), 20 deletions(-) diff --git a/datafusion/common/src/hash_utils.rs b/datafusion/common/src/hash_utils.rs index a51278b4556a..b84a3fdbe095 100644 --- a/datafusion/common/src/hash_utils.rs +++ b/datafusion/common/src/hash_utils.rs @@ -519,6 +519,138 @@ fn hash_generic_byte_view_array( } } +/// Dense-path unroll width; 8 lanes keep the gather pipeline full on ARM64/x86. +#[cfg(not(feature = "force_hash_collisions"))] +const DICT_SCATTER_LANES: usize = 8; + +/// Fold `dict_hash` into `prev` when `MULTI_COL`, else +/// overwrite with `dict_hash`. +#[cfg(not(feature = "force_hash_collisions"))] +#[inline(always)] +fn maybe_combine_dict_hash(prev: u64, dict_hash: u64) -> u64 { + if MULTI_COL { + combine_hashes(dict_hash, prev) + } else { + dict_hash + } +} + +/// Scatter precomputed dict-value hashes into every key position with no +/// null checks, 8-way unrolled. +/// +/// The 8-wide unroll is for performance, splitting the loop into +/// `std::array::from_fn` (gather 8 dict hashes by key index) followed by a +/// fixed-length inner loop encourages the compiler to overlap 8 +/// independent load latencies, saturating the gather pipeline instead of +/// serializing one lookup at a time. +/// +/// Invariants: `keys.len() == hashes.len()`; no key or dict value referenced +/// here is null. +#[cfg(not(feature = "force_hash_collisions"))] +#[inline(always)] +fn scatter_dict_hashes_no_null_checks< + K: ArrowDictionaryKeyType, + const MULTI_COL: bool, +>( + keys: &[K::Native], + dict_hashes: &[u64], + hashes: &mut [u64], +) { + let full_len = (keys.len() / DICT_SCATTER_LANES) * DICT_SCATTER_LANES; + let (keys_full, keys_tail) = keys.split_at(full_len); + let (hashes_full, hashes_tail) = hashes.split_at_mut(full_len); + for (key_group, hash_group) in keys_full + .chunks_exact(DICT_SCATTER_LANES) + .zip(hashes_full.chunks_exact_mut(DICT_SCATTER_LANES)) + { + let lane_hashes: [u64; DICT_SCATTER_LANES] = std::array::from_fn(|lane| unsafe { + *dict_hashes.get_unchecked(key_group[lane].as_usize()) + }); + for lane in 0..DICT_SCATTER_LANES { + hash_group[lane] = + maybe_combine_dict_hash::(hash_group[lane], lane_hashes[lane]); + } + } + // need to handle the remainder for non 64 byte multiples + for (key, hash) in keys_tail.iter().zip(hashes_tail.iter_mut()) { + let dict_hash = unsafe { *dict_hashes.get_unchecked(key.as_usize()) }; + *hash = maybe_combine_dict_hash::(*hash, dict_hash); + } +} + +/// Scatter dict-value hashes to every key position, skipping positions +/// where the dict value is null. Exists for the non-null-keys + +/// nullable-values case; the per-lane `is_valid` check is unavoidable. +#[cfg(not(feature = "force_hash_collisions"))] +#[inline(always)] +fn scatter_dict_hashes_skip_value_nulls< + K: ArrowDictionaryKeyType, + const MULTI_COL: bool, +>( + keys: &[K::Native], + dict_values: &dyn Array, + dict_hashes: &[u64], + hashes: &mut [u64], +) { + for (key, hash) in keys.iter().zip(hashes.iter_mut()) { + let value_idx = key.as_usize(); + if dict_values.is_valid(value_idx) { + let dict_hash = unsafe { *dict_hashes.get_unchecked(value_idx) }; + *hash = maybe_combine_dict_hash::(*hash, dict_hash); + } + } +} + +/// Scatter inside a mixed-validity 64-key chunk governed by `mask`. +#[cfg(not(feature = "force_hash_collisions"))] +#[inline(always)] +fn scatter_dict_hashes_in_masked_chunk< + K: ArrowDictionaryKeyType, + const HAS_NULL_VALUES: bool, + const MULTI_COL: bool, +>( + keys: &[K::Native], + dict_values: &dyn Array, + dict_hashes: &[u64], + hashes: &mut [u64], + mask: u64, +) { + debug_assert_eq!(keys.len(), 64); + debug_assert_eq!(hashes.len(), 64); + if HAS_NULL_VALUES { + let mut remaining = mask; + while remaining != 0 { + let lane = remaining.trailing_zeros() as usize; + remaining &= remaining - 1; + let value_idx = unsafe { keys.get_unchecked(lane).as_usize() }; + if dict_values.is_valid(value_idx) { + let dict_hash = unsafe { *dict_hashes.get_unchecked(value_idx) }; + let slot = unsafe { hashes.get_unchecked_mut(lane) }; + *slot = maybe_combine_dict_hash::(*slot, dict_hash); + } + } + return; + } + // null-key lanes carry garbage indices; we gather them anyway and mask the result out below + let max_valid_idx = dict_hashes.len().saturating_sub(1); + for lane_base in (0..64).step_by(DICT_SCATTER_LANES) { + // gather 8 dict hashes in parallel; clamp keeps garbage indices in-bounds + let lane_hashes: [u64; DICT_SCATTER_LANES] = std::array::from_fn(|lane_offset| { + let raw_idx = + unsafe { keys.get_unchecked(lane_base + lane_offset).as_usize() }; + let clamped_idx = raw_idx.min(max_valid_idx); + unsafe { *dict_hashes.get_unchecked(clamped_idx) } + }); + for lane_offset in 0..DICT_SCATTER_LANES { + let slot = unsafe { hashes.get_unchecked_mut(lane_base + lane_offset) }; + let combined_hash = + maybe_combine_dict_hash::(*slot, lane_hashes[lane_offset]); + let is_valid = ((mask >> (lane_base + lane_offset)) & 1) == 1; + *slot = if is_valid { combined_hash } else { *slot }; + } + } +} + /// Scatter precomputed dictionary value hashes to key positions. /// /// Uses const generics to eliminate runtime branching in the hot loop: @@ -538,28 +670,82 @@ fn hash_dictionary_scatter< hashes_buffer: &mut [u64], ) { let dict_values = array.values(); - if HAS_NULL_KEYS { - for (hash, key) in hashes_buffer.iter_mut().zip(array.keys().iter()) { - if let Some(key) = key { - let idx = key.as_usize(); - if !HAS_NULL_VALUES || dict_values.is_valid(idx) { - if MULTI_COL { - *hash = combine_hashes(dict_hashes[idx], *hash); - } else { - *hash = dict_hashes[idx]; - } - } + let keys = array.keys().values(); + + if !HAS_NULL_KEYS { + if HAS_NULL_VALUES { + scatter_dict_hashes_skip_value_nulls::( + keys, + dict_values.as_ref(), + dict_hashes, + hashes_buffer, + ); + } else { + scatter_dict_hashes_no_null_checks::( + keys, + dict_hashes, + hashes_buffer, + ); + } + return; + } + + let null_buffer = array.keys().nulls().expect("HAS_NULL_KEYS implies nulls"); + let bit_chunks = null_buffer.inner().bit_chunks(); + let chunk_len = bit_chunks.chunk_len(); + + // Walk the key null buffer 64 bits at a time; bit i set = lane i's key is non-null. + for (chunk_idx, mask) in bit_chunks.iter().enumerate() { + let base = chunk_idx * 64; + let keys_chunk = &keys[base..base + 64]; + let hashes_chunk = &mut hashes_buffer[base..base + 64]; + if mask == u64::MAX { + // all 64 keys valid: dense helper + if HAS_NULL_VALUES { + scatter_dict_hashes_skip_value_nulls::( + keys_chunk, + dict_values.as_ref(), + dict_hashes, + hashes_chunk, + ); + } else { + scatter_dict_hashes_no_null_checks::( + keys_chunk, + dict_hashes, + hashes_chunk, + ); } + } else if mask != 0 { + // mixed: masked-chunk helper (mask == 0 falls through, nothing to write) + scatter_dict_hashes_in_masked_chunk::( + keys_chunk, + dict_values.as_ref(), + dict_hashes, + hashes_chunk, + mask, + ); } - } else { - for (hash, key) in hashes_buffer.iter_mut().zip(array.keys().values()) { - let idx = key.as_usize(); - if !HAS_NULL_VALUES || dict_values.is_valid(idx) { - if MULTI_COL { - *hash = combine_hashes(dict_hashes[idx], *hash); - } else { - *hash = dict_hashes[idx]; - } + } + + // non-64 multiple: scalar path for the tail + let remainder_len = bit_chunks.remainder_len(); + if remainder_len > 0 { + let remainder = bit_chunks.remainder_bits(); + let base = chunk_len * 64; + let mut remaining = remainder; + while remaining != 0 { + let lane = remaining.trailing_zeros() as usize; + remaining &= remaining - 1; + if lane >= remainder_len { + break; + } + let value_idx = keys[base + lane].as_usize(); + if !HAS_NULL_VALUES || dict_values.is_valid(value_idx) { + let dict_hash = unsafe { *dict_hashes.get_unchecked(value_idx) }; + hashes_buffer[base + lane] = maybe_combine_dict_hash::( + hashes_buffer[base + lane], + dict_hash, + ); } } } From d5b570e7554894b3f1f55cf3a93fb0266cbb0da0 Mon Sep 17 00:00:00 2001 From: RIchard Baah Date: Fri, 9 Oct 2026 15:55:01 -0400 Subject: [PATCH 2/6] test: cover dict hash chunks --- datafusion/common/src/hash_utils.rs | 100 ++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 6 deletions(-) diff --git a/datafusion/common/src/hash_utils.rs b/datafusion/common/src/hash_utils.rs index b84a3fdbe095..b2dfff5cb109 100644 --- a/datafusion/common/src/hash_utils.rs +++ b/datafusion/common/src/hash_utils.rs @@ -566,12 +566,12 @@ fn scatter_dict_hashes_no_null_checks< let lane_hashes: [u64; DICT_SCATTER_LANES] = std::array::from_fn(|lane| unsafe { *dict_hashes.get_unchecked(key_group[lane].as_usize()) }); - for lane in 0..DICT_SCATTER_LANES { + for (lane, dict_hash) in lane_hashes.iter().copied().enumerate() { hash_group[lane] = - maybe_combine_dict_hash::(hash_group[lane], lane_hashes[lane]); + maybe_combine_dict_hash::(hash_group[lane], dict_hash); } } - // need to handle the remainder for non 64 byte multiples + // Handle the non-8-lane tail. for (key, hash) in keys_tail.iter().zip(hashes_tail.iter_mut()) { let dict_hash = unsafe { *dict_hashes.get_unchecked(key.as_usize()) }; *hash = maybe_combine_dict_hash::(*hash, dict_hash); @@ -641,10 +641,9 @@ fn scatter_dict_hashes_in_masked_chunk< let clamped_idx = raw_idx.min(max_valid_idx); unsafe { *dict_hashes.get_unchecked(clamped_idx) } }); - for lane_offset in 0..DICT_SCATTER_LANES { + for (lane_offset, dict_hash) in lane_hashes.iter().copied().enumerate() { let slot = unsafe { hashes.get_unchecked_mut(lane_base + lane_offset) }; - let combined_hash = - maybe_combine_dict_hash::(*slot, lane_hashes[lane_offset]); + let combined_hash = maybe_combine_dict_hash::(*slot, dict_hash); let is_valid = ((mask >> (lane_base + lane_offset)) & 1) == 1; *slot = if is_valid { combined_hash } else { *slot }; } @@ -1990,6 +1989,95 @@ mod tests { assert_ne!(dict_hashes[0], dict_hashes[2]); } + #[test] + #[cfg(not(feature = "force_hash_collisions"))] + fn create_hashes_for_chunked_dict_arrays_match_strings() { + use arrow::buffer::{NullBuffer, ScalarBuffer}; + + let random_state = RandomState::with_seed(0); + let assert_matches = |case: &str, + dict_array: DictionaryArray, + strings: Vec>| { + let string_array: ArrayRef = + Arc::new(strings.into_iter().collect::()); + let dict_array: ArrayRef = Arc::new(dict_array); + + let mut string_hashes = vec![0; string_array.len()]; + create_hashes(&[string_array], &random_state, &mut string_hashes).unwrap(); + + let mut dict_hashes = vec![0; dict_array.len()]; + create_hashes(&[dict_array], &random_state, &mut dict_hashes).unwrap(); + + assert_eq!(string_hashes, dict_hashes, "{case}"); + }; + + let dense_values = [Some("zero"), Some("one"), Some("two"), Some("three")]; + let dense_expected = (0..140) + .map(|i| dense_values[i % dense_values.len()]) + .collect::>(); + let dense_keys = (0..140) + .map(|i| Some((i % dense_values.len()) as i32)) + .collect::>(); + let dense_dict = DictionaryArray::::try_new( + Int32Array::from(dense_keys), + Arc::new(dense_values.iter().cloned().collect::()), + ) + .unwrap() + .slice(5, 130); + assert_matches( + "sliced dense keys", + dense_dict, + dense_expected[5..135].to_vec(), + ); + + let mixed_values = [Some("alpha"), Some("beta"), Some("gamma")]; + let null_keys = [0, 7, 63, 64, 65, 127, 129]; + let raw_keys = (0..130) + .map(|i| { + if null_keys.contains(&i) { + 10_000 + i as i32 + } else { + (i % mixed_values.len()) as i32 + } + }) + .collect::>(); + let valid_keys = (0..130) + .map(|i| !null_keys.contains(&i)) + .collect::>(); + let mixed_dict = DictionaryArray::::try_new( + Int32Array::new( + ScalarBuffer::from(raw_keys), + Some(NullBuffer::from(valid_keys)), + ), + Arc::new(mixed_values.iter().cloned().collect::()), + ) + .unwrap(); + let mixed_expected = (0..130) + .map(|i| { + if null_keys.contains(&i) { + None + } else { + mixed_values[i % mixed_values.len()] + } + }) + .collect(); + assert_matches("mixed key nulls", mixed_dict, mixed_expected); + + let nullable_values = [Some("left"), None, Some("right")]; + let nullable_keys = (0..130) + .map(|i| Some((i % nullable_values.len()) as i32)) + .collect::>(); + let nullable_dict = DictionaryArray::::try_new( + Int32Array::from(nullable_keys), + Arc::new(nullable_values.iter().cloned().collect::()), + ) + .unwrap(); + let nullable_expected = (0..130) + .map(|i| nullable_values[i % nullable_values.len()]) + .collect(); + assert_matches("nullable values", nullable_dict, nullable_expected); + } + #[test] // Tests actual values of hashes, which are different if forcing collisions #[cfg(not(feature = "force_hash_collisions"))] From 2e011c96725ea98909b9a55adddf57a2e518e8a3 Mon Sep 17 00:00:00 2001 From: RIchard Baah Date: Fri, 9 Oct 2026 16:22:02 -0400 Subject: [PATCH 3/6] test: move buffer imports --- datafusion/common/src/hash_utils.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/datafusion/common/src/hash_utils.rs b/datafusion/common/src/hash_utils.rs index b2dfff5cb109..6096d7bdc7e3 100644 --- a/datafusion/common/src/hash_utils.rs +++ b/datafusion/common/src/hash_utils.rs @@ -1489,6 +1489,11 @@ mod tests { use std::hash::{BuildHasherDefault, Hasher}; use std::sync::Arc; + use arrow::array::*; + use arrow::buffer::{NullBuffer, ScalarBuffer}; + #[cfg(not(feature = "force_hash_collisions"))] + use arrow::datatypes::*; + use super::*; #[cfg(not(feature = "force_hash_collisions"))] @@ -1992,8 +1997,6 @@ mod tests { #[test] #[cfg(not(feature = "force_hash_collisions"))] fn create_hashes_for_chunked_dict_arrays_match_strings() { - use arrow::buffer::{NullBuffer, ScalarBuffer}; - let random_state = RandomState::with_seed(0); let assert_matches = |case: &str, dict_array: DictionaryArray, @@ -2130,8 +2133,6 @@ mod tests { // Tests actual values of hashes, which are different if forcing collisions #[cfg(not(feature = "force_hash_collisions"))] fn create_hashes_for_list_view_arrays() { - use arrow::buffer::{NullBuffer, ScalarBuffer}; - // Create values array: [0, 1, 2, 3, null, 5] let values = Arc::new(Int32Array::from(vec![ Some(0), @@ -2180,8 +2181,6 @@ mod tests { // Tests actual values of hashes, which are different if forcing collisions #[cfg(not(feature = "force_hash_collisions"))] fn create_hashes_for_large_list_view_arrays() { - use arrow::buffer::{NullBuffer, ScalarBuffer}; - // Create values array: [0, 1, 2, 3, null, 5] let values = Arc::new(Int32Array::from(vec![ Some(0), From 732211b873ff7f2f2996b8073f8290fbcf5ac81a Mon Sep 17 00:00:00 2001 From: RIchard Baah Date: Fri, 9 Oct 2026 16:38:22 -0400 Subject: [PATCH 4/6] perf: remove dict hash bounds checks --- datafusion/common/src/hash_utils.rs | 207 ++-------------------------- 1 file changed, 14 insertions(+), 193 deletions(-) diff --git a/datafusion/common/src/hash_utils.rs b/datafusion/common/src/hash_utils.rs index 6096d7bdc7e3..d8fbaf636b3a 100644 --- a/datafusion/common/src/hash_utils.rs +++ b/datafusion/common/src/hash_utils.rs @@ -519,10 +519,6 @@ fn hash_generic_byte_view_array( } } -/// Dense-path unroll width; 8 lanes keep the gather pipeline full on ARM64/x86. -#[cfg(not(feature = "force_hash_collisions"))] -const DICT_SCATTER_LANES: usize = 8; - /// Fold `dict_hash` into `prev` when `MULTI_COL`, else /// overwrite with `dict_hash`. #[cfg(not(feature = "force_hash_collisions"))] @@ -535,121 +531,6 @@ fn maybe_combine_dict_hash(prev: u64, dict_hash: u64) -> } } -/// Scatter precomputed dict-value hashes into every key position with no -/// null checks, 8-way unrolled. -/// -/// The 8-wide unroll is for performance, splitting the loop into -/// `std::array::from_fn` (gather 8 dict hashes by key index) followed by a -/// fixed-length inner loop encourages the compiler to overlap 8 -/// independent load latencies, saturating the gather pipeline instead of -/// serializing one lookup at a time. -/// -/// Invariants: `keys.len() == hashes.len()`; no key or dict value referenced -/// here is null. -#[cfg(not(feature = "force_hash_collisions"))] -#[inline(always)] -fn scatter_dict_hashes_no_null_checks< - K: ArrowDictionaryKeyType, - const MULTI_COL: bool, ->( - keys: &[K::Native], - dict_hashes: &[u64], - hashes: &mut [u64], -) { - let full_len = (keys.len() / DICT_SCATTER_LANES) * DICT_SCATTER_LANES; - let (keys_full, keys_tail) = keys.split_at(full_len); - let (hashes_full, hashes_tail) = hashes.split_at_mut(full_len); - for (key_group, hash_group) in keys_full - .chunks_exact(DICT_SCATTER_LANES) - .zip(hashes_full.chunks_exact_mut(DICT_SCATTER_LANES)) - { - let lane_hashes: [u64; DICT_SCATTER_LANES] = std::array::from_fn(|lane| unsafe { - *dict_hashes.get_unchecked(key_group[lane].as_usize()) - }); - for (lane, dict_hash) in lane_hashes.iter().copied().enumerate() { - hash_group[lane] = - maybe_combine_dict_hash::(hash_group[lane], dict_hash); - } - } - // Handle the non-8-lane tail. - for (key, hash) in keys_tail.iter().zip(hashes_tail.iter_mut()) { - let dict_hash = unsafe { *dict_hashes.get_unchecked(key.as_usize()) }; - *hash = maybe_combine_dict_hash::(*hash, dict_hash); - } -} - -/// Scatter dict-value hashes to every key position, skipping positions -/// where the dict value is null. Exists for the non-null-keys + -/// nullable-values case; the per-lane `is_valid` check is unavoidable. -#[cfg(not(feature = "force_hash_collisions"))] -#[inline(always)] -fn scatter_dict_hashes_skip_value_nulls< - K: ArrowDictionaryKeyType, - const MULTI_COL: bool, ->( - keys: &[K::Native], - dict_values: &dyn Array, - dict_hashes: &[u64], - hashes: &mut [u64], -) { - for (key, hash) in keys.iter().zip(hashes.iter_mut()) { - let value_idx = key.as_usize(); - if dict_values.is_valid(value_idx) { - let dict_hash = unsafe { *dict_hashes.get_unchecked(value_idx) }; - *hash = maybe_combine_dict_hash::(*hash, dict_hash); - } - } -} - -/// Scatter inside a mixed-validity 64-key chunk governed by `mask`. -#[cfg(not(feature = "force_hash_collisions"))] -#[inline(always)] -fn scatter_dict_hashes_in_masked_chunk< - K: ArrowDictionaryKeyType, - const HAS_NULL_VALUES: bool, - const MULTI_COL: bool, ->( - keys: &[K::Native], - dict_values: &dyn Array, - dict_hashes: &[u64], - hashes: &mut [u64], - mask: u64, -) { - debug_assert_eq!(keys.len(), 64); - debug_assert_eq!(hashes.len(), 64); - if HAS_NULL_VALUES { - let mut remaining = mask; - while remaining != 0 { - let lane = remaining.trailing_zeros() as usize; - remaining &= remaining - 1; - let value_idx = unsafe { keys.get_unchecked(lane).as_usize() }; - if dict_values.is_valid(value_idx) { - let dict_hash = unsafe { *dict_hashes.get_unchecked(value_idx) }; - let slot = unsafe { hashes.get_unchecked_mut(lane) }; - *slot = maybe_combine_dict_hash::(*slot, dict_hash); - } - } - return; - } - // null-key lanes carry garbage indices; we gather them anyway and mask the result out below - let max_valid_idx = dict_hashes.len().saturating_sub(1); - for lane_base in (0..64).step_by(DICT_SCATTER_LANES) { - // gather 8 dict hashes in parallel; clamp keeps garbage indices in-bounds - let lane_hashes: [u64; DICT_SCATTER_LANES] = std::array::from_fn(|lane_offset| { - let raw_idx = - unsafe { keys.get_unchecked(lane_base + lane_offset).as_usize() }; - let clamped_idx = raw_idx.min(max_valid_idx); - unsafe { *dict_hashes.get_unchecked(clamped_idx) } - }); - for (lane_offset, dict_hash) in lane_hashes.iter().copied().enumerate() { - let slot = unsafe { hashes.get_unchecked_mut(lane_base + lane_offset) }; - let combined_hash = maybe_combine_dict_hash::(*slot, dict_hash); - let is_valid = ((mask >> (lane_base + lane_offset)) & 1) == 1; - *slot = if is_valid { combined_hash } else { *slot }; - } - } -} - /// Scatter precomputed dictionary value hashes to key positions. /// /// Uses const generics to eliminate runtime branching in the hot loop: @@ -669,82 +550,22 @@ fn hash_dictionary_scatter< hashes_buffer: &mut [u64], ) { let dict_values = array.values(); - let keys = array.keys().values(); - - if !HAS_NULL_KEYS { - if HAS_NULL_VALUES { - scatter_dict_hashes_skip_value_nulls::( - keys, - dict_values.as_ref(), - dict_hashes, - hashes_buffer, - ); - } else { - scatter_dict_hashes_no_null_checks::( - keys, - dict_hashes, - hashes_buffer, - ); - } - return; - } - - let null_buffer = array.keys().nulls().expect("HAS_NULL_KEYS implies nulls"); - let bit_chunks = null_buffer.inner().bit_chunks(); - let chunk_len = bit_chunks.chunk_len(); - - // Walk the key null buffer 64 bits at a time; bit i set = lane i's key is non-null. - for (chunk_idx, mask) in bit_chunks.iter().enumerate() { - let base = chunk_idx * 64; - let keys_chunk = &keys[base..base + 64]; - let hashes_chunk = &mut hashes_buffer[base..base + 64]; - if mask == u64::MAX { - // all 64 keys valid: dense helper - if HAS_NULL_VALUES { - scatter_dict_hashes_skip_value_nulls::( - keys_chunk, - dict_values.as_ref(), - dict_hashes, - hashes_chunk, - ); - } else { - scatter_dict_hashes_no_null_checks::( - keys_chunk, - dict_hashes, - hashes_chunk, - ); + if HAS_NULL_KEYS { + for (hash, key) in hashes_buffer.iter_mut().zip(array.keys().iter()) { + if let Some(key) = key { + let idx = key.as_usize(); + if !HAS_NULL_VALUES || dict_values.is_valid(idx) { + let dict_hash = unsafe { *dict_hashes.get_unchecked(idx) }; + *hash = maybe_combine_dict_hash::(*hash, dict_hash); + } } - } else if mask != 0 { - // mixed: masked-chunk helper (mask == 0 falls through, nothing to write) - scatter_dict_hashes_in_masked_chunk::( - keys_chunk, - dict_values.as_ref(), - dict_hashes, - hashes_chunk, - mask, - ); } - } - - // non-64 multiple: scalar path for the tail - let remainder_len = bit_chunks.remainder_len(); - if remainder_len > 0 { - let remainder = bit_chunks.remainder_bits(); - let base = chunk_len * 64; - let mut remaining = remainder; - while remaining != 0 { - let lane = remaining.trailing_zeros() as usize; - remaining &= remaining - 1; - if lane >= remainder_len { - break; - } - let value_idx = keys[base + lane].as_usize(); - if !HAS_NULL_VALUES || dict_values.is_valid(value_idx) { - let dict_hash = unsafe { *dict_hashes.get_unchecked(value_idx) }; - hashes_buffer[base + lane] = maybe_combine_dict_hash::( - hashes_buffer[base + lane], - dict_hash, - ); + } else { + for (hash, key) in hashes_buffer.iter_mut().zip(array.keys().values()) { + let idx = key.as_usize(); + if !HAS_NULL_VALUES || dict_values.is_valid(idx) { + let dict_hash = unsafe { *dict_hashes.get_unchecked(idx) }; + *hash = maybe_combine_dict_hash::(*hash, dict_hash); } } } From 88d50e09c9580ed07a6f343093c8dfc8c0f02b84 Mon Sep 17 00:00:00 2001 From: RIchard Baah Date: Fri, 9 Oct 2026 16:40:31 -0400 Subject: [PATCH 5/6] test: remove dict chunk regression --- datafusion/common/src/hash_utils.rs | 90 ----------------------------- 1 file changed, 90 deletions(-) diff --git a/datafusion/common/src/hash_utils.rs b/datafusion/common/src/hash_utils.rs index d8fbaf636b3a..d677a6889953 100644 --- a/datafusion/common/src/hash_utils.rs +++ b/datafusion/common/src/hash_utils.rs @@ -1310,10 +1310,7 @@ mod tests { use std::hash::{BuildHasherDefault, Hasher}; use std::sync::Arc; - use arrow::array::*; use arrow::buffer::{NullBuffer, ScalarBuffer}; - #[cfg(not(feature = "force_hash_collisions"))] - use arrow::datatypes::*; use super::*; @@ -1815,93 +1812,6 @@ mod tests { assert_ne!(dict_hashes[0], dict_hashes[2]); } - #[test] - #[cfg(not(feature = "force_hash_collisions"))] - fn create_hashes_for_chunked_dict_arrays_match_strings() { - let random_state = RandomState::with_seed(0); - let assert_matches = |case: &str, - dict_array: DictionaryArray, - strings: Vec>| { - let string_array: ArrayRef = - Arc::new(strings.into_iter().collect::()); - let dict_array: ArrayRef = Arc::new(dict_array); - - let mut string_hashes = vec![0; string_array.len()]; - create_hashes(&[string_array], &random_state, &mut string_hashes).unwrap(); - - let mut dict_hashes = vec![0; dict_array.len()]; - create_hashes(&[dict_array], &random_state, &mut dict_hashes).unwrap(); - - assert_eq!(string_hashes, dict_hashes, "{case}"); - }; - - let dense_values = [Some("zero"), Some("one"), Some("two"), Some("three")]; - let dense_expected = (0..140) - .map(|i| dense_values[i % dense_values.len()]) - .collect::>(); - let dense_keys = (0..140) - .map(|i| Some((i % dense_values.len()) as i32)) - .collect::>(); - let dense_dict = DictionaryArray::::try_new( - Int32Array::from(dense_keys), - Arc::new(dense_values.iter().cloned().collect::()), - ) - .unwrap() - .slice(5, 130); - assert_matches( - "sliced dense keys", - dense_dict, - dense_expected[5..135].to_vec(), - ); - - let mixed_values = [Some("alpha"), Some("beta"), Some("gamma")]; - let null_keys = [0, 7, 63, 64, 65, 127, 129]; - let raw_keys = (0..130) - .map(|i| { - if null_keys.contains(&i) { - 10_000 + i as i32 - } else { - (i % mixed_values.len()) as i32 - } - }) - .collect::>(); - let valid_keys = (0..130) - .map(|i| !null_keys.contains(&i)) - .collect::>(); - let mixed_dict = DictionaryArray::::try_new( - Int32Array::new( - ScalarBuffer::from(raw_keys), - Some(NullBuffer::from(valid_keys)), - ), - Arc::new(mixed_values.iter().cloned().collect::()), - ) - .unwrap(); - let mixed_expected = (0..130) - .map(|i| { - if null_keys.contains(&i) { - None - } else { - mixed_values[i % mixed_values.len()] - } - }) - .collect(); - assert_matches("mixed key nulls", mixed_dict, mixed_expected); - - let nullable_values = [Some("left"), None, Some("right")]; - let nullable_keys = (0..130) - .map(|i| Some((i % nullable_values.len()) as i32)) - .collect::>(); - let nullable_dict = DictionaryArray::::try_new( - Int32Array::from(nullable_keys), - Arc::new(nullable_values.iter().cloned().collect::()), - ) - .unwrap(); - let nullable_expected = (0..130) - .map(|i| nullable_values[i % nullable_values.len()]) - .collect(); - assert_matches("nullable values", nullable_dict, nullable_expected); - } - #[test] // Tests actual values of hashes, which are different if forcing collisions #[cfg(not(feature = "force_hash_collisions"))] From 9e86750dbbaa6bf9ae214873a59c4095a1ee462d Mon Sep 17 00:00:00 2001 From: RIchard Baah Date: Sun, 11 Oct 2026 16:07:44 -0400 Subject: [PATCH 6/6] perf: inline maybe_combine_dict_hash and add SAFETY comments Address review feedback on PR #26159: shrink the diff by removing the maybe_combine_dict_hash helper and inlining it at both call sites, and add SAFETY comments documenting the invariant for the unsafe dict_hashes.get_unchecked(idx) accesses. --- datafusion/common/src/hash_utils.rs | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/datafusion/common/src/hash_utils.rs b/datafusion/common/src/hash_utils.rs index d677a6889953..46b86d1a0561 100644 --- a/datafusion/common/src/hash_utils.rs +++ b/datafusion/common/src/hash_utils.rs @@ -519,18 +519,6 @@ fn hash_generic_byte_view_array( } } -/// Fold `dict_hash` into `prev` when `MULTI_COL`, else -/// overwrite with `dict_hash`. -#[cfg(not(feature = "force_hash_collisions"))] -#[inline(always)] -fn maybe_combine_dict_hash(prev: u64, dict_hash: u64) -> u64 { - if MULTI_COL { - combine_hashes(dict_hash, prev) - } else { - dict_hash - } -} - /// Scatter precomputed dictionary value hashes to key positions. /// /// Uses const generics to eliminate runtime branching in the hot loop: @@ -555,8 +543,13 @@ fn hash_dictionary_scatter< if let Some(key) = key { let idx = key.as_usize(); if !HAS_NULL_VALUES || dict_values.is_valid(idx) { + // SAFETY: `idx` is a valid Arrow dict key, in-bounds for `dict_hashes`. let dict_hash = unsafe { *dict_hashes.get_unchecked(idx) }; - *hash = maybe_combine_dict_hash::(*hash, dict_hash); + *hash = if MULTI_COL { + combine_hashes(dict_hash, *hash) + } else { + dict_hash + }; } } } @@ -564,8 +557,13 @@ fn hash_dictionary_scatter< for (hash, key) in hashes_buffer.iter_mut().zip(array.keys().values()) { let idx = key.as_usize(); if !HAS_NULL_VALUES || dict_values.is_valid(idx) { + // SAFETY: `idx` is a valid Arrow dict key, in-bounds for `dict_hashes`. let dict_hash = unsafe { *dict_hashes.get_unchecked(idx) }; - *hash = maybe_combine_dict_hash::(*hash, dict_hash); + *hash = if MULTI_COL { + combine_hashes(dict_hash, *hash) + } else { + dict_hash + }; } } } @@ -1310,6 +1308,7 @@ mod tests { use std::hash::{BuildHasherDefault, Hasher}; use std::sync::Arc; + #[cfg(not(feature = "force_hash_collisions"))] use arrow::buffer::{NullBuffer, ScalarBuffer}; use super::*;