kumarUjjawal commented on code in PR #27025:
URL: https://github.com/apache/datafusion/pull/27025#discussion_r4450789891
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -544,6 +565,47 @@ fn hash_generic_byte_view_array<T: ByteViewType>(
}
}
+/// Scatters non-null dictionary hashes in independent lanes to overlap
key-indexed loads.
+#[cfg(not(feature = "force_hash_collisions"))]
+#[inline(always)]
+fn scatter_non_null_dictionary<K: ArrowDictionaryKeyType, const MULTI_COL:
bool>(
+ keys: &[K::Native],
+ dict_hashes: &[u64],
+ hashes: &mut [u64],
+) {
+ const LANES: usize = 8;
+ let len = keys.len().min(hashes.len());
+ let keys = &keys[..len];
+ let hashes = &mut hashes[..len];
+
+ let (key_chunks, key_tail) = keys.as_chunks::<LANES>();
Review Comment:
No unit test reaches the new 8-lane chunk loop in
scatter_non_null_dictionary. The only dictionary hash test
(create_hashes_for_dict_arrays) has 5 rows and nulls, so it never touches the
new function, and nothing checks the chunk/tail boundary or the MULTI_COL=true
chunk path.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -158,8 +170,17 @@ where
let mut buffer = cell.try_borrow_mut()
.map_err(|_| _internal_datafusion_err!("with_hashes cannot be
called reentrantly on the same thread"))?;
Review Comment:
Commit cb54f47 deleted test_non_null_dictionary_hash_fast_path. It was the
only test that runs with_hashes over a dirty thread-local buffer, so the new
skip-clear path and the conditions that must still clear are now untested.**
Failure scenario: A later edit wrongly widens
`dictionary_overwrites_hash_buffer`, e.g. it stops checking
values().null_count(). Then with_hashes([&seed]) followed by
with_hashes([&dict_with_null_values]) returns the seed's stale hashes in null
rows instead of 0. No unit test fails: the remaining with_hashes tests use
fresh primitive or string inputs, and create_hashes_for_dict_arrays calls
create_hashes with a zeroed Vec.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -562,6 +624,14 @@ fn hash_dictionary_scatter<
dict_hashes: &[u64],
hashes_buffer: &mut [u64],
) {
+ if !HAS_NULL_KEYS && !HAS_NULL_VALUES {
+ scatter_non_null_dictionary::<K, MULTI_COL>(
+ array.keys().values(),
+ dict_hashes,
+ hashes_buffer,
+ );
+ return;
Review Comment:
Add correctness tests for the new non-null fast paths
The existing hash utility dictionary tests contain null keys, so they bypass
this new scatter path; the `with_hashes` tests use primitive/string arrays and
do not exercise dictionary buffer reuse. The benchmark checks only output
length. Add regression tests covering a full eight-lane chunk plus a tail,
multi-column combining, short output buffers, and consecutive `with_hashes`
calls switching between non-null and nullable dictionaries. These checks would
catch lane-order errors and stale hashes that the current tests cannot detect.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -158,8 +170,17 @@ where
let mut buffer = cell.try_borrow_mut()
.map_err(|_| _internal_datafusion_err!("with_hashes cannot be
called reentrantly on the same thread"))?;
- // Ensure buffer has sufficient length, clearing old values
- buffer.clear();
+ // A dictionary with non-null keys and values overwrites every output
Review Comment:
Altitude: skipping the clear is special-cased to dictionaries, but every
first-column hasher overwrites all slots when the column has no nulls, so the
gate is narrower than the property it relies on.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -158,8 +170,17 @@ where
let mut buffer = cell.try_borrow_mut()
.map_err(|_| _internal_datafusion_err!("with_hashes cannot be
called reentrantly on the same thread"))?;
- // Ensure buffer has sufficient length, clearing old values
- buffer.clear();
+ // A dictionary with non-null keys and values overwrites every output
+ // slot, so retain the initialized storage instead of zeroing it first.
+ #[cfg(not(feature = "force_hash_collisions"))]
Review Comment:
The force_hash_collisions cfg is split across a cfg-gated helper and two
cfg'd `let reuse_buffer` bindings. A single helper that returns false under
force_hash_collisions would do the same job.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -544,6 +565,47 @@ fn hash_generic_byte_view_array<T: ByteViewType>(
}
}
+/// Scatters non-null dictionary hashes in independent lanes to overlap
key-indexed loads.
+#[cfg(not(feature = "force_hash_collisions"))]
+#[inline(always)]
+fn scatter_non_null_dictionary<K: ArrowDictionaryKeyType, const MULTI_COL:
bool>(
+ keys: &[K::Native],
+ dict_hashes: &[u64],
+ hashes: &mut [u64],
+) {
+ const LANES: usize = 8;
+ let len = keys.len().min(hashes.len());
+ let keys = &keys[..len];
+ let hashes = &mut hashes[..len];
+
+ let (key_chunks, key_tail) = keys.as_chunks::<LANES>();
+ let (hash_chunks, hash_tail) = hashes.as_chunks_mut::<LANES>();
+ for (keys, hashes) in key_chunks.iter().zip(hash_chunks) {
+ let gathered: [u64; LANES] = std::array::from_fn(|lane| unsafe {
+ let key = keys[lane];
+ // DictionaryArray guarantees that every non-null key is in bounds.
+ *dict_hashes.get_unchecked(key.as_usize())
+ });
+ if MULTI_COL {
+ for (hash, dict_hash) in hashes.iter_mut().zip(gathered) {
+ *hash = combine_hashes(dict_hash, *hash);
+ }
+ } else {
+ *hashes = gathered;
+ }
+ }
+
+ for (hash, key) in hash_tail.iter_mut().zip(key_tail) {
Review Comment:
The tail loop in scatter_non_null_dictionary copies the per-element combine
logic from the chunk body (`if MULTI_COL { combine_hashes(dict_hash, *hash) }
else { dict_hash }`).
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -544,6 +565,47 @@ fn hash_generic_byte_view_array<T: ByteViewType>(
}
}
+/// Scatters non-null dictionary hashes in independent lanes to overlap
key-indexed loads.
+#[cfg(not(feature = "force_hash_collisions"))]
+#[inline(always)]
+fn scatter_non_null_dictionary<K: ArrowDictionaryKeyType, const MULTI_COL:
bool>(
+ keys: &[K::Native],
+ dict_hashes: &[u64],
+ hashes: &mut [u64],
+) {
+ const LANES: usize = 8;
+ let len = keys.len().min(hashes.len());
+ let keys = &keys[..len];
+ let hashes = &mut hashes[..len];
+
+ let (key_chunks, key_tail) = keys.as_chunks::<LANES>();
+ let (hash_chunks, hash_tail) = hashes.as_chunks_mut::<LANES>();
+ for (keys, hashes) in key_chunks.iter().zip(hash_chunks) {
+ let gathered: [u64; LANES] = std::array::from_fn(|lane| unsafe {
+ let key = keys[lane];
+ // DictionaryArray guarantees that every non-null key is in bounds.
+ *dict_hashes.get_unchecked(key.as_usize())
Review Comment:
The new unsafe blocks use comments like '// DictionaryArray guarantees...'
instead of the '// SAFETY:' prefix the neighboring code uses (e.g.
hash_dictionary_scatter).
##########
datafusion/common/src/hash_utils.rs:
##########
Review Comment:
https://github.com/apache/datafusion/blob/cb54f47fd46c7a859c909c6e42416d1cfa6e6d61/datafusion/common/src/hash_utils.rs#L650
After the early return, the non-null-keys `else` branch in
hash_dictionary_scatter now only runs when HAS_NULL_VALUES is true, so its
`!HAS_NULL_VALUES ||` short-circuit is dead.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -96,6 +96,18 @@ pub fn combine_hashes(l: u64, r: u64) -> u64 {
/// to this size.
const MAX_BUFFER_SIZE: usize = 524_288;
+#[cfg(not(feature = "force_hash_collisions"))]
+fn dictionary_overwrites_hash_buffer(array: &dyn Array) -> bool {
Review Comment:
dictionary_overwrites_hash_buffer re-implements the scatter's dispatch
condition (keys null_count == 0 && values null_count == 0) in a separate place
from dispatch_dictionary_scatter, so correctness depends on two separate checks
staying in sync.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -544,6 +565,47 @@ fn hash_generic_byte_view_array<T: ByteViewType>(
}
}
+/// Scatters non-null dictionary hashes in independent lanes to overlap
key-indexed loads.
+#[cfg(not(feature = "force_hash_collisions"))]
+#[inline(always)]
+fn scatter_non_null_dictionary<K: ArrowDictionaryKeyType, const MULTI_COL:
bool>(
Review Comment:
scatter_non_null_dictionary is a safe fn that dereferences
dict_hashes.get_unchecked(key) from raw slices. Its safety depends on the
caller passing dict_hashes sized to the dictionary's values, but the signature
no longer ties keys to that array (the old loop took &DictionaryArray).
--
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]
