Skip to content
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

Jump to bottom

Check hashes first during probing the aggr hash table #11718

Merged
merged 2 commits into from
Jul 31, 2024
+5 −4
Merged

Check hashes first during probing the aggr hash table #11718

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0b8009
check hashes first during probing.
Rachelint Jul 30, 2024
9458a24
fix style.
ShiKaiWi Jul 30, 2024
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
9 changes: 5 additions & 4 deletions datafusion/physical-plan/src/aggregates/group_values/row.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,13 @@ impl GroupValues for GroupValuesRows {
batch_hashes.resize(n_rows, 0);
create_hashes(cols, &self.random_state, batch_hashes)?;

for (row, &hash) in batch_hashes.iter().enumerate() {
let entry = self.map.get_mut(hash, |(_hash, group_idx)| {
for (row, &target_hash) in batch_hashes.iter().enumerate() {
let entry = self.map.get_mut(target_hash, |(exist_hash, group_idx)| {
// verify that a group that we are inserting with hash is
// actually the same key value as the group in
// existing_idx (aka group_values @ row)
group_rows.row(row) == group_values.row(*group_idx)
target_hash == *exist_hash
Copy link
Contributor

Choose a reason for hiding this comment

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

I am somewhat confused about how this makes things faster -- I thought that the check for equal hash was done as part of self.map.get_mut (aka the closure is only called when the hashes are equal, so I would expct this comparison to always be true)

However, if the benchmark results show it is an improvement wonderful.

I'll run the numbers as well to confirm.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get it either.
I thought the closure (row values equal check) is triggered only if the hash is matched 😕

Copy link
Contributor Author

@Rachelint Rachelint Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am somewhat confused about how this makes things faster -- I thought that the check for equal hash was done as part of self.map.get_mut (aka the closure is only called when the hashes are equal, so I would expct this comparison to always be true)

However, if the benchmark results show it is an improvement wonderful.

I'll run the numbers as well to confirm.

This is my thought about why faster.

I read the source code in hashbrown, and the get_mut procedure is like this:

  • Use the hash value to find the first bucket.
  • If the bucket is filled, it will use the eq function passed by us to check if it is the target, for example, the prev eq function.
|(exist_hash, group_idx)| {
                // verify that a group that we are inserting with hash is
                // actually the same key value as the group in
                // existing_idx  (aka group_values @ row)
                group_rows.row(row) == group_values.row(*group_idx)
            })
  • If eq return true, it is the target, otherwise we need to prob next and check.

In the high cardinality aggr scenario, the entry often actually not exist in the hash table.

And after the hash table grow too large(many buckets are filled), the prob will perform many times and finally find nothing...

In this sitution, check the hash first can reduce the random memory accesses compared to directy check the group value through group index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @jayzhan211 , can see the guess above.

Copy link
Contributor

@Dandandan Dandandan Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@Rachelint
My understanding is the similar, collision will happen quite often in get_mut, equality of u64 hashes will be faster than retrieving / comparing rows.
Hash collisions are usually very low, even for high cardinality, but RawTable::get_mut doesn't check for equality itself, just finds a first match without guaranteeing hash values are the same (equality check should be in the provided equality function). In other implementations we also check for hash values to be equal first.

Copy link
Contributor Author

@Rachelint Rachelint Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was curious -- I think the code @Rachelint is referring to in hashbrown is here

https://github.com/rust-lang/hashbrown/blob/ac00a0bbef46f02f555e235f57ce263aefa361e0/src/raw/mod.rs#L2183-L2199

It does appear that collisions could happen (it is doing some sort of abbreviated check by condensing the actual hash value to a byte or something), though I don't fully understand how it works.

I wonder if there are other places we could use this observation 🤔

Good idea! I am trying to find it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea to check hash before expensive arrow row comparison makes sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was curious -- I think the code @Rachelint is referring to in hashbrown is here

https://github.com/rust-lang/hashbrown/blob/ac00a0bbef46f02f555e235f57ce263aefa361e0/src/raw/mod.rs#L2183-L2199

It does appear that collisions could happen (it is doing some sort of abbreviated check by condensing the actual hash value to a byte or something), though I don't fully understand how it works.

I wonder if there are other places we could use this observation 🤔

This talk explains the details of this one-byte abbreviation trick https://www.youtube.com/watch?v=ncHmEUmJZf4 , I vaguely remember they said when this 1 byte check is done, it's very likely to find the correct slot.
Looks like it's not working well when the hash table size grows over some threshold?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was curious -- I think the code @Rachelint is referring to in hashbrown is here
https://github.com/rust-lang/hashbrown/blob/ac00a0bbef46f02f555e235f57ce263aefa361e0/src/raw/mod.rs#L2183-L2199
It does appear that collisions could happen (it is doing some sort of abbreviated check by condensing the actual hash value to a byte or something), though I don't fully understand how it works.
I wonder if there are other places we could use this observation 🤔

This talk explains the details of this one-byte abbreviation trick https://www.youtube.com/watch?v=ncHmEUmJZf4 , I vaguely remember they said when this 1 byte check is done, it's very likely to find the correct slot. Looks like it's not working well when the hash table size grows over some threshold?

Seems make sense, maybe we should only add this check when found the hash table size is larger than a threshold 🤔 .

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking this rationale is very non obvious, so I proposed adding some comments on th rationale here #11750

&& group_rows.row(row) == group_values.row(*group_idx)
});

let group_idx = match entry {
Expand All @@ -139,7 +140,7 @@ impl GroupValues for GroupValuesRows {

// for hasher function, use precomputed hash value
self.map.insert_accounted(
(hash, group_idx),
(target_hash, group_idx),
|(hash, _group_index)| *hash,
&mut self.map_size,
);
Expand Down