Skip to content

perf: speed up DictionaryArray hashing #26159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alamb merged 6 commits into from
Oct 11, 2026
+15 −12
Merged

perf: speed up DictionaryArray hashing #26159

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a76b7dd
perf: speed up DictionaryArray hashing
Rich-T-kid Oct 9, 2026
d5b570e
test: cover dict hash chunks
Rich-T-kid Oct 9, 2026
2e011c9
test: move buffer imports
Rich-T-kid Oct 9, 2026
732211b
perf: remove dict hash bounds checks
Rich-T-kid Oct 9, 2026
88d50e0
test: remove dict chunk regression
Rich-T-kid Oct 9, 2026
9e86750
perf: inline maybe_combine_dict_hash and add SAFETY comments
Rich-T-kid Oct 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions datafusion/common/src/hash_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,23 +543,27 @@ fn hash_dictionary_scatter<
if let Some(key) = key {
let idx = key.as_usize();
if !HAS_NULL_VALUES || dict_values.is_valid(idx) {
Comment thread
Rich-T-kid marked this conversation as resolved.
if MULTI_COL {
*hash = combine_hashes(dict_hashes[idx], *hash);
// SAFETY: `idx` is a valid Arrow dict key, in-bounds for `dict_hashes`.
let dict_hash = unsafe { *dict_hashes.get_unchecked(idx) };

@Rich-T-kid Rich-T-kid Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should add a saftey comment

@alamb alamb Oct 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the safety is predicated o the fact that the key came from a valid Arrow array and thus is guaranteed to be in bound of the dictionary

@alamb alamb Oct 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(should also post unsafe comment below too)

*hash = if MULTI_COL {
combine_hashes(dict_hash, *hash)
} else {
*hash = dict_hashes[idx];
}
dict_hash
};
}
}
}
} else {
for (hash, key) in hashes_buffer.iter_mut().zip(array.keys().values()) {
let idx = key.as_usize();
if !HAS_NULL_VALUES || dict_values.is_valid(idx) {
if MULTI_COL {
*hash = combine_hashes(dict_hashes[idx], *hash);
// 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 {
*hash = dict_hashes[idx];
}
dict_hash
};
}
}
}
Expand Down Expand Up @@ -1304,6 +1308,9 @@ mod tests {
use std::hash::{BuildHasherDefault, Hasher};
use std::sync::Arc;

#[cfg(not(feature = "force_hash_collisions"))]
use arrow::buffer::{NullBuffer, ScalarBuffer};

use super::*;

#[cfg(not(feature = "force_hash_collisions"))]
Expand Down Expand Up @@ -1856,8 +1863,6 @@ mod tests {
// Tests actual values of hashes, which are different if forcing collisions
#[cfg(not(feature = "force_hash_collisions"))]
fn create_hashes_for_list_view_arrays() {
use arrow::buffer::{NullBuffer, ScalarBuffer};

Comment on lines -1859 to -1860

@Rich-T-kid Rich-T-kid Oct 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quality of life changes here

// Create values array: [0, 1, 2, 3, null, 5]
let values = Arc::new(Int32Array::from(vec![
Some(0),
Expand Down Expand Up @@ -1906,8 +1911,6 @@ mod tests {
// Tests actual values of hashes, which are different if forcing collisions
#[cfg(not(feature = "force_hash_collisions"))]
fn create_hashes_for_large_list_view_arrays() {
use arrow::buffer::{NullBuffer, ScalarBuffer};

// Create values array: [0, 1, 2, 3, null, 5]
let values = Arc::new(Int32Array::from(vec![
Some(0),
Expand Down
Loading