Skip to content

Remove hppc from terms agg#86151

Merged
rjernst merged 3 commits intoelastic:masterfrom
rjernst:hppc/termsagg
Apr 26, 2022
Merged

Remove hppc from terms agg#86151
rjernst merged 3 commits intoelastic:masterfrom
rjernst:hppc/termsagg

Conversation

@rjernst
Copy link
Copy Markdown
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

@rjernst
Remove hppc from terms agg
b298356 
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
Copy Markdown
Collaborator

elasticmachine commented Apr 25, 2022

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

@rjernst rjernst mentioned this pull request Apr 25, 2022
Closed
43 tasks
nik9000
nik9000 reviewed Apr 26, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Reviewers

@nik9000 nik9000 nik9000 approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rjernst @elasticmachine @nik9000