Rich-T-kid commented on code in PR #27025:
URL: https://github.com/apache/datafusion/pull/27025#discussion_r4450863147
##########
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:
Added one focused regression covering an eight-row chunk plus tail,
multi-column hashing, short output buffers, and consecutive non-null/nullable
calls.
##########
datafusion/common/src/hash_utils.rs:
##########
Review Comment:
Removed the dead short-circuit.
##########
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:
Moved the combine-or-replace logic into update_dictionary_hash.
##########
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:
Restored the dirty-buffer coverage in the focused regression. The nullable
key and value cases now run immediately after the non-null fast path.
##########
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:
Covered now with 11 rows, a 10-row output buffer, and the dictionary as the
second column.
##########
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:
Changed both to SAFETY comments and included the equal-length invariant.
##########
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:
The reuse gate and scatter dispatch now share dictionary_nulls.
##########
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:
I kept this dictionary-only. Non-null nested hashers such as lists and
structs still combine with the existing slot, so null_count == 0 is not enough
to skip initialization.
##########
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:
Done. There is now one reuse_buffer binding; the feature-specific helper
returns false.
##########
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:
The helper takes &DictionaryArray now and checks dict_hashes.len() against
values().len() before the unchecked loads.
--
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]
