-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Make Aggregations an abstract class rather than an interface #24184
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,83 +27,26 @@ | |
|
|
||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.Iterator; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import static java.util.Collections.emptyMap; | ||
| import static java.util.Collections.unmodifiableMap; | ||
| /** | ||
| * An internal implementation of {@link Aggregations}. | ||
| */ | ||
| public class InternalAggregations implements Aggregations, ToXContent, Streamable { | ||
| public final class InternalAggregations extends Aggregations implements ToXContent, Streamable { | ||
|
|
||
| public static final InternalAggregations EMPTY = new InternalAggregations(); | ||
|
|
||
| private List<InternalAggregation> aggregations = Collections.emptyList(); | ||
|
|
||
| private Map<String, Aggregation> aggregationsAsMap; | ||
|
|
||
| private InternalAggregations() { | ||
| } | ||
|
|
||
| /** | ||
| * Constructs a new addAggregation. | ||
| */ | ||
| public InternalAggregations(List<InternalAggregation> aggregations) { | ||
| this.aggregations = aggregations; | ||
| } | ||
|
|
||
| /** | ||
| * Iterates over the {@link Aggregation}s. | ||
| */ | ||
| @Override | ||
| public Iterator<Aggregation> iterator() { | ||
| return aggregations.stream().map((p) -> (Aggregation) p).iterator(); | ||
| } | ||
|
|
||
| /** | ||
| * The list of {@link Aggregation}s. | ||
| */ | ||
| @Override | ||
| public List<Aggregation> asList() { | ||
| return aggregations.stream().map((p) -> (Aggregation) p).collect(Collectors.toList()); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the {@link Aggregation}s keyed by map. | ||
| */ | ||
| @Override | ||
| public Map<String, Aggregation> asMap() { | ||
| return getAsMap(); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the {@link Aggregation}s keyed by map. | ||
| */ | ||
| @Override | ||
| public Map<String, Aggregation> getAsMap() { | ||
| if (aggregationsAsMap == null) { | ||
| Map<String, InternalAggregation> newAggregationsAsMap = new HashMap<>(); | ||
| for (InternalAggregation aggregation : aggregations) { | ||
| newAggregationsAsMap.put(aggregation.getName(), aggregation); | ||
| } | ||
| this.aggregationsAsMap = unmodifiableMap(newAggregationsAsMap); | ||
| } | ||
| return aggregationsAsMap; | ||
| } | ||
|
|
||
| /** | ||
| * @return the aggregation of the specified name. | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| @Override | ||
| public <A extends Aggregation> A get(String name) { | ||
| return (A) asMap().get(name); | ||
| super(aggregations); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -118,21 +61,16 @@ public static InternalAggregations reduce(List<InternalAggregations> aggregation | |
| } | ||
|
|
||
| // first we collect all aggregations of the same type and list them together | ||
|
|
||
| Map<String, List<InternalAggregation>> aggByName = new HashMap<>(); | ||
| for (InternalAggregations aggregations : aggregationsList) { | ||
| for (InternalAggregation aggregation : aggregations.aggregations) { | ||
| List<InternalAggregation> aggs = aggByName.get(aggregation.getName()); | ||
| if (aggs == null) { | ||
| aggs = new ArrayList<>(aggregationsList.size()); | ||
| aggByName.put(aggregation.getName(), aggs); | ||
| } | ||
| aggs.add(aggregation); | ||
| for (Aggregation aggregation : aggregations.aggregations) { | ||
| List<InternalAggregation> aggs = aggByName.computeIfAbsent( | ||
| aggregation.getName(), k -> new ArrayList<>(aggregationsList.size())); | ||
| aggs.add((InternalAggregation)aggregation); | ||
| } | ||
| } | ||
|
|
||
| // now we can use the first aggregation of each list to handle the reduce of its list | ||
|
|
||
| List<InternalAggregation> reducedAggregations = new ArrayList<>(); | ||
| for (Map.Entry<String, List<InternalAggregation>> entry : aggByName.entrySet()) { | ||
| List<InternalAggregation> aggregations = entry.getValue(); | ||
|
|
@@ -142,41 +80,33 @@ public static InternalAggregations reduce(List<InternalAggregations> aggregation | |
| return new InternalAggregations(reducedAggregations); | ||
| } | ||
|
|
||
| /** The fields required to write this addAggregation to xcontent */ | ||
| static class Fields { | ||
| public static final String AGGREGATIONS = "aggregations"; | ||
| } | ||
|
|
||
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I was just wondering |
||
| if (aggregations.isEmpty()) { | ||
| return builder; | ||
| } | ||
| builder.startObject(Fields.AGGREGATIONS); | ||
| builder.startObject("aggregations"); | ||
| toXContentInternal(builder, params); | ||
| return builder.endObject(); | ||
| } | ||
|
|
||
| /** | ||
| * Directly write all the addAggregation without their bounding object. Used by sub-addAggregation (non top level addAggregation) | ||
| * Directly write all the aggregations without their bounding object. Used by sub-aggregations (non top level aggs) | ||
| */ | ||
| public XContentBuilder toXContentInternal(XContentBuilder builder, Params params) throws IOException { | ||
| for (Aggregation aggregation : aggregations) { | ||
| ((InternalAggregation) aggregation).toXContent(builder, params); | ||
| ((InternalAggregation)aggregation).toXContent(builder, params); | ||
| } | ||
| return builder; | ||
| } | ||
|
|
||
|
|
||
| public static InternalAggregations readAggregations(StreamInput in) throws IOException { | ||
| InternalAggregations result = new InternalAggregations(); | ||
| result.readFrom(in); | ||
| return result; | ||
| } | ||
|
|
||
| public static InternalAggregations readOptionalAggregations(StreamInput in) throws IOException { | ||
| return in.readOptionalStreamable(InternalAggregations::new); | ||
| } | ||
|
|
||
| @Override | ||
| public void readFrom(StreamInput in) throws IOException { | ||
| aggregations = in.readList(stream -> in.readNamedWriteable(InternalAggregation.class)); | ||
|
|
@@ -186,20 +116,8 @@ public void readFrom(StreamInput in) throws IOException { | |
| } | ||
|
|
||
| @Override | ||
| @SuppressWarnings("unchecked") | ||
| public void writeTo(StreamOutput out) throws IOException { | ||
| out.writeNamedWriteableList(aggregations); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (obj == null || getClass() != obj.getClass()) { | ||
| return false; | ||
| } | ||
| return aggregations.equals(((InternalAggregations) obj).aggregations); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(getClass(), aggregations); | ||
| out.writeNamedWriteableList((List<InternalAggregation>)aggregations); | ||
| } | ||
| } | ||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right that is a good point. seems like it could hurt then, I will move it back.