Skip to content

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Apr 25, 2022

The terms aggs take include/excludes for terms, and for numeric fields
use an hppc set of longs. This commit converts to using a HashSet.

relates #84735

The terms aggs take include/excludes for terms, and for numeric fields
use an hppc set of longs. This commit converts to using a HashSet.

relates elastic#84735
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 25, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@rjernst rjernst mentioned this pull request Apr 25, 2022
43 tasks
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Do we know if the contains test allocates? This is deep in a high performance path where we really can't allocate. The size of the objects themselves doesn't matter - it's the accept method that calls contains.

Another option that we know doesn't allocate is LongHash - it's a bit silly here, but if we're allocating then it's probably worth it.

If we're not allocating we probably should have a comments in accept about it.

@rjernst
Copy link
Member Author

rjernst commented Apr 26, 2022

contains doesn't directly allocate, but the autoboxing that will occur could, if the value is outside the range of pre-cached boxed values.

I don't have any strong opinions, other than the overall goal of moving away from hppc. I'll switch this to LongHash.

@rjernst
Copy link
Member Author

rjernst commented Apr 26, 2022

LongHash would have been difficult to use because it requires BigArrays. Instead, I've switched the value type of the set to be our own private boxed type that allows modification. This way the accept method can use a spare instance as we do in other methods here (eg spare BytesRef).

@rjernst
Copy link
Member Author

rjernst commented Apr 26, 2022

@elasticmachine run elasticsearch-ci/part-1

@rjernst rjernst merged commit fc535ea into elastic:master Apr 26, 2022
@rjernst rjernst deleted the hppc/termsagg branch April 26, 2022 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Aggregations Aggregations >refactoring Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants