You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/07/31 14:13:04 UTC
[PR] Minor: Add comment explaining rationale for hash check [datafusion]
alamb opened a new pull request, #11750:
URL: https://github.com/apache/datafusion/pull/11750
## Which issue does this PR close?
Follow on to https://github.com/apache/datafusion/pull/11718
Closes #.
## Rationale for this change
In https://github.com/apache/datafusion/pull/11718 @Rachelint found that comparing hash values before group key values improved performance. The reason this could be faster was not obvious at first
## What changes are included in this PR?
Add doc comments explaining the rationale and linking to https://github.com/apache/datafusion/pull/11718
## Are these changes tested?
CI
## Are there any user-facing changes?
No, just comments
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Minor: Add comment explaining rationale for hash check [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11750:
URL: https://github.com/apache/datafusion/pull/11750#issuecomment-2260652641
Seems duckdb has the similar check(but just use the u16 prefix), maybe we can mention?
https://github.com/duckdb/duckdb/blob/f92559f42c118075baaa8daafc437954eb5c85ec/src/execution/aggregate_hashtable.cpp#L379-L386
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Minor: Add comment explaining rationale for hash check [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #11750:
URL: https://github.com/apache/datafusion/pull/11750
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Minor: Add comment explaining rationale for hash check [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11750:
URL: https://github.com/apache/datafusion/pull/11750#issuecomment-2260652679
Seems duckdb has the similar check(but just use the u16 prefix), maybe we can mention?
https://github.com/duckdb/duckdb/blob/f92559f42c118075baaa8daafc437954eb5c85ec/src/execution/aggregate_hashtable.cpp#L379-L386
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Minor: Add comment explaining rationale for hash check [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11750:
URL: https://github.com/apache/datafusion/pull/11750#issuecomment-2260899751
> > Seems duckdb has the similar check(but just use the u16 prefix), maybe we can mention?
> > https://github.com/duckdb/duckdb/blob/f92559f42c118075baaa8daafc437954eb5c85ec/src/execution/aggregate_hashtable.cpp#L379-L386
>
> The linked PR is interesting: [duckdb/duckdb#9575](https://github.com/duckdb/duckdb/pull/9575)
Interesting, it seems the common point with #11718 is that we should consider the increasing collision when the hash table is filling up.
> > maybe we can mention?
>
> I am not sure what we would mention 🤔 Maybe can you propose the wording ?
>
> Or maybe we can leave a comment to the duckdb art in one of the comments on #11718 ?
Maybe better to leave a comment, seems actually hard to make a good conclusion about the discussion in #11718
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Minor: Add comment explaining rationale for hash check [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11750:
URL: https://github.com/apache/datafusion/pull/11750#issuecomment-2260817863
> maybe we can mention?
I am not sure what we would mention 🤔 Maybe can you propose the wording ?
Or maybe we can leave a comment to the duckdb art in one of the comments on https://github.com/apache/datafusion/pull/11718 ?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Minor: Add comment explaining rationale for hash check [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11750:
URL: https://github.com/apache/datafusion/pull/11750#issuecomment-2260933508
> Actually, I am thinking that, maybe exist a threshold (like xx% of the bucket have been filled), and it is just reasonable to check the hash first when exceed it.
It might be worth checking -- one thing that we have to be careful of is that the overhead of the check itself may end up large
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Minor: Add comment explaining rationale for hash check [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11750:
URL: https://github.com/apache/datafusion/pull/11750#issuecomment-2260814818
> Seems duckdb has the similar check(but just use the u16 prefix), maybe we can mention?
>
> https://github.com/duckdb/duckdb/blob/f92559f42c118075baaa8daafc437954eb5c85ec/src/execution/aggregate_hashtable.cpp#L379-L386
The linked PR is interesting: https://github.com/duckdb/duckdb/pull/9575
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Minor: Add comment explaining rationale for hash check [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11750:
URL: https://github.com/apache/datafusion/pull/11750#issuecomment-2260908276
> Interesting, it seems the common point with https://github.com/apache/datafusion/pull/11718 is that we should consider the increasing collision when the hash table is filling up.
In theory I think this is the kind of optimization that we are relying on `hashbrown` to do for us. Maybe there are some tuning knobs we could set but maybe we should just leave the defaults -- it seems to work reasonably well
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Minor: Add comment explaining rationale for hash check [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11750:
URL: https://github.com/apache/datafusion/pull/11750#issuecomment-2260892662
> > Seems duckdb has the similar check(but just use the u16 prefix), maybe we can mention?
> > https://github.com/duckdb/duckdb/blob/f92559f42c118075baaa8daafc437954eb5c85ec/src/execution/aggregate_hashtable.cpp#L379-L386
>
> The linked PR is interesting: [duckdb/duckdb#9575](https://github.com/duckdb/duckdb/pull/9575)
Interesting, it seems the common point with #11718 is that we should consider the increasing collision when the hash table is filling up.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Minor: Add comment explaining rationale for hash check [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11750:
URL: https://github.com/apache/datafusion/pull/11750#issuecomment-2260922679
> > Interesting, it seems the common point with #11718 is that we should consider the increasing collision when the hash table is filling up.
>
> In theory I think this is the kind of optimization that we are relying on `hashbrown` to do for us. Maybe there are some tuning knobs we could set but maybe we should just leave the defaults -- it seems to work reasonably well
Yes, as @2010YOUY01 mentioned [ in #11](https://github.com/apache/datafusion/pull/11718#discussion_r1696934383), hashbrown do a lot of design work about it.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Minor: Add comment explaining rationale for hash check [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11750:
URL: https://github.com/apache/datafusion/pull/11750#issuecomment-2260946831
> > Actually, I am thinking that, maybe exist a threshold (like xx% of the bucket have been filled), and it is just reasonable to check the hash first when exceed it.
>
> It might be worth checking -- one thing that we have to be careful of is that the overhead of the check itself may end up large
Make sense, will make more experiments about this when having free time.
Another thing I found maybe we can optimize is about the `group index`. We use `usize` type now, it seems too big(which lead to the large size of hash table), maybe u32 is enough?
Actually u32 can hold `42 * (sizeof T) G` entries.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org