Make Aggregations an abstract class rather than an interface #24184
Conversation
Some of the base methods that don't have to do with reduce phase and serialization can be moved to the base class which is no longer an interface. This will be reusable by the high level REST client further on the road. Also it simplify things as having an interface with a single implementor is not that helpful.
javanna
added
:Analytics/Aggregations
| protected Map<String, Aggregation> aggregationsAsMap; | ||
|
|
||
| protected Aggregations() { | ||
|
|
| /** | ||
| * Iterates over the {@link Aggregation}s. | ||
| */ | ||
|
|
| */ | ||
| List<Aggregation> asList(); | ||
| public final List<Aggregation> asList() { | ||
| return aggregations.stream().map((p) -> p).collect(Collectors.toList()); |
There was a problem hiding this comment.
return new ArrayList<>(aggregations) ?
There was a problem hiding this comment.
sounds like it. I tried to move things without touching them, but now you are pushing me to fix all this :)
| Map<String, Aggregation> getAsMap(); | ||
| public final Map<String, Aggregation> getAsMap() { | ||
| if (aggregationsAsMap == null) { | ||
| Map<String, Aggregation> newAggregationsAsMap = new HashMap<>(); |
There was a problem hiding this comment.
I think you can initialize the capacity.
| if (obj == null || getClass() != obj.getClass()) { | ||
| return false; | ||
| } | ||
| return aggregations.equals(((InternalAggregations) obj).aggregations); |
There was a problem hiding this comment.
Why the cast in InternalAggregations?
| } | ||
|
|
||
| @Override | ||
| public final boolean equals(Object obj) { |
There was a problem hiding this comment.
Since we don't plan to implement equals/hashCode() for the ParsedAggregations at this point, wouldn't it make sense to leave this in InternalAggregations?
There was a problem hiding this comment.
I don't know, does it hurt here? it is based on members that have been moved to the base class after all.
There was a problem hiding this comment.
Doesn't hurt, I guess, but it might obfuscate the fact that all the parsed aggregations don't implement equals/hashCode. At a quick glance people might think all subclasses properly implement equals()...
There was a problem hiding this comment.
right that is a good point. seems like it could hurt then, I will move it back.
| } | ||
|
|
||
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { |
There was a problem hiding this comment.
Since we are planning to implement toXContent equivalents for the ParsedAggregations, would it make sense to pull this (and maybe toXContentInternal()) to the abstract class?
There was a problem hiding this comment.
I am working on this, I was doing it but it's controversial. That's why I took it off this PR for now, I prefer doing it in small steps.
There was a problem hiding this comment.
Specifically, moving the ToXContent part requires to adjust the Aggregation (singular) part too. The short solution is making Aggregation extend ToXContent which is not the cleanest, yet I am not sure any of the other solutions are feasible as they involve generics and complicate things quite a bit.
Some of the base methods that don't have to do with reduce phase and serialization can be moved to the base class which is no longer an interface. This will be reusable by the high level REST client further on the road. Also it simplify things as having an interface with a single implementor is not that helpful.
* master: (61 commits) Build: Move plugin cli and tests to distribution tool (elastic#24220) Peer Recovery: remove maxUnsafeAutoIdTimestamp hand off (elastic#24243) Adds version 5.3.2 and backwards compatibility indices for 5.3.1 Add utility method to parse named XContent objects with typed prefix (elastic#24240) MultiBucketsAggregation.Bucket should not extend Writeable (elastic#24216) Don't expose cleaned-up tasks as pending in PrioritizedEsThreadPoolExecutor (elastic#24237) Adds declareNamedObjects methods to ConstructingObjectParser (elastic#24219) ESIntegTestCase.indexRandom should not introduce types. (elastic#24202) Tests: Extend InternalStatsTests (elastic#24212) IndicesQueryCache should delegate the scorerSupplier method. (elastic#24209) Speed up parsing of large `terms` queries. (elastic#24210) [TEST] make sure that the random query_string query generator defines a default_field or a list of fields token_count type : add an option to count tokens (fix elastic#23227) (elastic#24175) Query string default field (elastic#24214) Make Aggregations an abstract class rather than an interface (elastic#24184) [TEST] ensure expected sequence no and version are set when index/delete engine operation has a document failure Extract batch executor out of cluster service (elastic#24102) Add 5.3.1 to bwc versions Added "release-state" support to plugin docs Added examples to cross cluster search of using cluster settings ...
3 participants
Some of the base methods that don't have to do with reduce phase and serialization can be moved to the base class which is no longer an interface. This will be reusable by the high level REST client further on the road. Also it simplify things as having an interface with a single implementor is not that helpful.