Skip to content

Inline redundant SingleBucketAggregation interface #127093

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

original-brownbear
Copy link
Member

This interface is just implemented once, inline it into the abstract parent of all other implementations.

Sorry that the diff is a little hard to follow for the SingleBucketAggregation file because of the rename. I still figured it was cleaner to do the rename and inline in one go but happy to change that if it's hard to follow.
Hopefully it's ok because this is just a mechanical PR, but happy to do without the rename of the abstract class to the interface for a shorter PR.

This interface is just implemented once, inline it into the abstract parent of
all other implementations.
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 19, 2025

import java.io.IOException;
import java.util.Map;

/**
* Results of the {@link ParentToChildrenAggregator}.
*/
public class InternalChildren extends InternalSingleBucketAggregation {
Copy link
Member Author

Choose a reason for hiding this comment

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

The motivation in cleaning this up shoud become visible here, all but one implementation of SingleBucketAggregation are noop overrides. We can (with a bit of BwC code for the transport layer) fold all of these into a single class and save both a lot of code but also make serialization a lot more compact (and faster!).

*/
public interface SingleBucketAggregation extends Aggregation {
public class SingleBucketAggregation extends InternalAggregation {
Copy link
Member Author

Choose a reason for hiding this comment

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

Making this a concrete class. For now, that only cleans up some testing and allows us to replace the mocks with actual instances. But going forward, we should be able to remove the various noop subclasses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants