kumarUjjawal commented on code in PR #27025:
URL: https://github.com/apache/datafusion/pull/27025#discussion_r4451002650
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -158,8 +181,14 @@ 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:
Skipping buffer.clear() only saves zeroing at most required_size u64s (about
64KB at batch size 8192), yet it adds a correctness coupling between the shared
buffer code and one array type's scatter.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -2284,6 +2368,78 @@ mod tests {
assert_eq!(result, expected_hashes);
}
+ #[test]
+ #[cfg(not(feature = "force_hash_collisions"))]
Review Comment:
The new test only covers Int32 keys and an unsliced dictionary, so neither
the generic key widths (u8/u16/i64) nor a sliced dictionary is tested on the
8-lane fast path or the reuse path.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -544,6 +573,58 @@ 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>(
+ array: &DictionaryArray<K>,
+ dict_hashes: &[u64],
+ hashes: &mut [u64],
+) {
+ const LANES: usize = 8;
+ assert_eq!(dict_hashes.len(), array.values().len());
+ assert_eq!(array.keys().null_count(), 0);
+ let keys = array.keys().values();
+ let len = keys.len().min(hashes.len());
+ let keys = &keys[..len];
+ let hashes = &mut hashes[..len];
Review Comment:
scatter_non_null_dictionary re-checks keys().null_count()==0 and
dict_hashes.len() with assert_eq! on every call, after
dispatch_dictionary_scatter has already chosen this path from the same null
counts.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -158,8 +181,14 @@ 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.
+ let reuse_buffer = iter
Review Comment:
Correctness of with_hashes now depends silently on
scatter_non_null_dictionary writing every output slot, and nothing checks that,
not even a debug_assert.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -158,8 +181,14 @@ 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.
Review Comment:
The reuse check calls dictionary_nulls through a separate
downcast_dictionary_array! dispatch, then hash_single_array downcasts again and
dispatch_dictionary_scatter recomputes the same null pair.
--
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]
