Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Aggregator result equality easy to break #111757

Open
nik9000 opened this issue Aug 9, 2024 · 1 comment
Open

Aggregator result equality easy to break #111757

nik9000 opened this issue Aug 9, 2024 · 1 comment
Labels
:Analytics/Aggregations Aggregations >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)

Comments

@nik9000
Copy link
Member

nik9000 commented Aug 9, 2024

Description

We'd like to make some optimizations that are gated behind the equals method on subclasses of InternalAggregation working perfectly. But at the moment it's quite easy implement equals incorrectly - our default equals implementation checks a few things but doesn't force subclasses to override it with their extra data. Many do, but not all. We should make not overriding equals here a compile time failure. Generally we do this by adding an abstract boolean doEquals method and calling it in equals. That's a bit tricky here because of all the layers of subclasses, but we can do it.
@nik9000 nik9000 added >enhancement :Analytics/Aggregations Aggregations needs:triage Requires assignment of a team area label labels Aug 9, 2024
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) and removed needs:triage Requires assignment of a team area label labels Aug 9, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

nik9000 added a commit to nik9000/elasticsearch that referenced this issue Aug 9, 2024
@nik9000
Revert "Avoid bucket copies in Aggs (elastic#110261)"
ddcb503 
This reverts elastic#110261 which we can't land until elastic#111757 - we need to be
sure that the `equals` implementations on subclasses of
`InternalAggregations` is correct before this optimization is safe.
This was referenced Aug 9, 2024
Merged
Closed
elasticsearchmachine pushed a commit that referenced this issue Aug 9, 2024
@nik9000
Revert "Avoid bucket copies in Aggs (#110261)" (#111758)
6b88065 
This reverts #110261 which we can't land until #111757 - we need to be
sure that the `equals` implementations on subclasses of
`InternalAggregations` is correct before this optimization is safe.

Closes #111679
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Aug 9, 2024
@nik9000
Revert "Avoid bucket copies in Aggs (elastic#110261)" (elastic#111758)
234b429 
This reverts elastic#110261 which we can't land until elastic#111757 - we need to be
sure that the `equals` implementations on subclasses of
`InternalAggregations` is correct before this optimization is safe.

Closes elastic#111679
elasticsearchmachine pushed a commit that referenced this issue Aug 9, 2024
@nik9000
Revert "Avoid bucket copies in Aggs (#110261)" (#111758) (#111761)
801e1b9 
This reverts #110261 which we can't land until #111757 - we need to be
sure that the `equals` implementations on subclasses of
`InternalAggregations` is correct before this optimization is safe.

Closes #111679
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this issue Sep 4, 2024
@nik9000 @cbuescher
Revert "Avoid bucket copies in Aggs (elastic#110261)" (elastic#111758)
2e13976 
This reverts elastic#110261 which we can't land until elastic#111757 - we need to be
sure that the `equals` implementations on subclasses of
`InternalAggregations` is correct before this optimization is safe.

Closes elastic#111679
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this issue Sep 5, 2024
@nik9000 @davidkyle
Revert "Avoid bucket copies in Aggs (elastic#110261)" (elastic#111758)
f27439b 
This reverts elastic#110261 which we can't land until elastic#111757 - we need to be
sure that the `equals` implementations on subclasses of
`InternalAggregations` is correct before this optimization is safe.

Closes elastic#111679
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
:Analytics/Aggregations Aggregations >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@nik9000 @elasticsearchmachine