kumarUjjawal commented on code in PR #27025:
URL: https://github.com/apache/datafusion/pull/27025#discussion_r4559708907
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -2284,6 +2366,78 @@ mod tests {
assert_eq!(result, expected_hashes);
}
+ #[test]
+ #[cfg(not(feature = "force_hash_collisions"))]
+ fn test_non_null_dictionary_hash_fast_path() {
+ let random_state = RandomState::with_seed(0);
+ let keys = vec![2, 0, 1, 2, 1, 0, 2, 0, 1, 2, 0];
+ let non_null: ArrayRef = Arc::new(
+ DictionaryArray::<Int32Type>::try_new(
+ Int32Array::from(keys.clone()),
+ Arc::new(StringArray::from(vec!["a", "b", "c"])),
Review Comment:
Add the requested sliced and alternate-key fast-path tests, this test still
uses only unsliced Int32 dictionaries. The explanatory comment does not provide
regression coverage for offset handling or other key representations in the new
unchecked eight-lane loop. Add nonzero-offset slices and u8/u16/i64 cases
containing a full chunk plus a tail, comparing both `create_hashes` and
`with_hashes` against decoded values.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -2284,6 +2366,78 @@ mod tests {
assert_eq!(result, expected_hashes);
}
+ #[test]
+ #[cfg(not(feature = "force_hash_collisions"))]
+ fn test_non_null_dictionary_hash_fast_path() {
Review Comment:
u8/u16/i64 key widths and a sliced dictionary on the 8-lane fast path and
the reuse path is not added
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -96,6 +96,20 @@ 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_nulls<K: ArrowDictionaryKeyType>(
+ array: &DictionaryArray<K>,
+) -> (bool, bool) {
+ (
+ array.keys().null_count() != 0,
+ array.values().null_count() != 0,
+ )
+}
+
+fn dictionary_overwrites_hash_buffer(array: &dyn Array) -> bool {
Review Comment:
`dictionary_overwrites_hash_buffer` is no longer gated on
`force_hash_collisions` and returns true in that configuration.
Under `--features force_hash_collisions`, with_hashes skips `clear()` for
non-null dictionaries. It is harmless only because that build's
`hash_single_array` writes 0 to every slot. The resolution kumarUjjawal
accepted no longer matches the code, and the reuse rule now depends on two
different `hash_single_array` implementations both overwriting every slot, with
nothing documenting that.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -96,6 +96,20 @@ 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_nulls<K: ArrowDictionaryKeyType>(
+ array: &DictionaryArray<K>,
+) -> (bool, bool) {
+ (
+ array.keys().null_count() != 0,
+ array.values().null_count() != 0,
+ )
+}
+
+fn dictionary_overwrites_hash_buffer(array: &dyn Array) -> bool {
+ matches!(array.data_type(), DataType::Dictionary(_, _)) &&
!array.is_nullable()
Review Comment:
The buffer-reuse gate again uses a different null test (`is_nullable()`,
logical) from the scatter dispatch (`dictionary_nulls`, physical `null_count`).
This brings back the two-separate-checks coupling that i asked earlier to
remove.
If arrow changes an `is_nullable` override (Union/RunArray/NullArray
values), or someone edits one predicate, with_hashes skips `clear()` while a
null path leaves slots unwritten, and callers get stale hashes from the
previous batch. There is also a cost in the other direction: when keys carry an
all-valid null buffer (common from IPC/parquet decoding), `is_nullable()` is
true, so the buffer is cleared even though the fast path overwrites every slot.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -544,6 +564,65 @@ 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;
+ debug_assert_eq!(dict_hashes.len(), array.values().len());
Review Comment:
`scatter_non_null_dictionary` is a safe fn that does
`get_unchecked(key.as_usize())` into `dict_hashes`, and its only bounds check
is now a `debug_assert_eq!`, so release builds have no check at all.**
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -96,6 +96,20 @@ 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_nulls<K: ArrowDictionaryKeyType>(
+ array: &DictionaryArray<K>,
+) -> (bool, bool) {
+ (
+ array.keys().null_count() != 0,
+ array.values().null_count() != 0,
+ )
+}
+
+fn dictionary_overwrites_hash_buffer(array: &dyn Array) -> bool {
+ matches!(array.data_type(), DataType::Dictionary(_, _)) &&
!array.is_nullable()
Review Comment:
The reuse gate calls `array.is_nullable()` on every `with_hashes` call. For
a dictionary that recurses into `values.is_nullable()` →
`logical_null_count()`, which is O(values) for nested dictionary or REE values.
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -544,6 +564,65 @@ 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;
+ debug_assert_eq!(dict_hashes.len(), array.values().len());
+ debug_assert_eq!(dictionary_nulls(array), (false, false));
+ let keys = array.keys().values();
+ let len = keys.len().min(hashes.len());
Review Comment:
The dictionary fast path caps the work at `keys.len().min(hashes.len())` and
checks full coverage only with a `debug_assert`. Every other hasher
(`hash_array_primitive`, `hash_array`, `hash_string_view_array_inner`) does a
release `assert_eq!` on the lengths, so dictionaries follow a different,
inconsistent contract.
--
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]
