-
Notifications
You must be signed in to change notification settings - Fork 25.2k
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
base: main
Are you sure you want to change the base?
Inline redundant SingleBucketAggregation interface #127093
Conversation
This interface is just implemented once, inline it into the abstract parent of all other implementations.
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
||
import java.io.IOException; | ||
import java.util.Map; | ||
|
||
/** | ||
* Results of the {@link ParentToChildrenAggregator}. | ||
*/ | ||
public class InternalChildren extends InternalSingleBucketAggregation { |
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.
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 { |
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.
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.
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.