-
-
Notifications
You must be signed in to change notification settings - Fork 817
Add composite aggregation #2714
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
|
@rdettai-sk I refactored aggregations in this PR: #2715 |
|
e083a69 to
e7e2277
Compare
|
Impact of the refacto:
|
|
@PSeitz this PR is finally pretty much ready. Histogram benchmarks are a bit disappointing, I'll try to figure out why.
|
|
I found the issue with histograms. Fx Hash behaves very poorly with small round floats because the lower bits are usually constant (0), and that triggers a lot of collisions.
|
| get_all_ff_readers(reader, &source.field, Some(&allowed_column_types))?; | ||
|
|
||
| columns_and_types | ||
| .sort_by_key(|(_, col_type)| col_type_order_key(col_type, source.order)); |
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.
can we just sort them and if the order is reversed add .reverse?
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 admit that this part is over-complicating things a little bit. This sort works hand in hand with find_skip_idx() / skip_for_key() which assume an order where types other than bool/str/numeric are always at the end. Just reverting wouldn't work, but we can maybe find something simpler.
See https://www.elastic.co/docs/reference/aggregations/search-aggregations-bucket-composite-aggregation
Composite aggregations are usefull to export pre-aggregated data at medium cardinailities (10k to millions).