-
Notifications
You must be signed in to change notification settings - Fork 953
Use imporved version of Postgres' simplehash table for aggregation #8006
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.
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
base: main
Are you sure you want to change the base?
Conversation
akuzm
commented
Apr 24, 2025
- No separate status field for better memory efficiency
- Slow path in insert function rewritten to use tail recursion for better inlining of fast path
* No separate status field for better memory efficiency * Slow path in insert function rewritten to use tail recursion for better inlining of fast path
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8006 +/- ##
==========================================
- Coverage 82.21% 81.98% -0.24%
==========================================
Files 256 257 +1
Lines 48184 48303 +119
Branches 12155 12182 +27
==========================================
- Hits 39615 39599 -16
- Misses 3704 3891 +187
+ Partials 4865 4813 -52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Why do we need to vendor Postgres simplehash.h? |
We use it for vectorized aggregation, and this is a version where I made a couple peformance-related changes as described in the PR message. |
Ahh got it ... I've just quickly looked the both code and it looked like the same but actually no (after diff them). |
/* | ||
* This file and its contents are licensed under the Timescale License. | ||
* Please see the included NOTICE for copyright information and | ||
* LICENSE-TIMESCALE for a copy of the license. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that Postgres license is very permissive, but doesn't make sense to vendor this code on Apache code?
* PostgreSQL License. Please see the NOTICE at the top level | ||
* directory for a copy of the PostgreSQL License. | ||
* | ||
* PostgreSQL 17.4 f8554dee417ffc4540c94cf357f7bf7d4b6e5d80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have a brief explanation here about the changes you've made just for reference.
|
||
/* <element> *<prefix>_lookup(<prefix>_hash *tb, <key> key) */ | ||
SH_SCOPE SH_ELEMENT_TYPE *SH_LOOKUP(SH_TYPE * tb, SH_KEY_TYPE key); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed that you removed SH_DELETE_ITEM
and SH_DELETE
. We don't need it? I'm just thinking that maybe this simplehash stuff can be used in other places.