lakshaydevoo-arch commented on code in PR #26966:
URL: https://github.com/apache/datafusion/pull/26966#discussion_r4502489868
##########
datafusion/common/src/hash_utils.rs:
##########
@@ -563,19 +563,23 @@ 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) {
- // SAFETY: `idx` is a valid Arrow dict key, in-bounds for
`dict_hashes`.
- let dict_hash = unsafe { *dict_hashes.get_unchecked(idx) };
- *hash = if MULTI_COL {
- combine_hashes(dict_hash, *hash)
- } else {
- dict_hash
- };
- }
+ if HAS_NULL_KEYS && let Some(key_nulls) = array.keys().nulls() {
+ // Visit only valid key slots: null slots keep their hash, and their
+ // (possibly out-of-range) key values are never read
+ let keys = array.keys().values();
+ // Like the `zip` in the loop below, stop at the end of `hashes_buffer`
+ let len = hashes_buffer.len();
+ for i in key_nulls.valid_indices().take_while(|&i| i < len) {
Review Comment:
i've switched to the 64-row word loop you suggested and added the
short-buffer regression test, including the case where an all-valid word starts
beyond the buffer length.
i've also merged main and kept both sets of tests
--
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]
