From 7b9e650c8a7704c4f5e8389b374c223573f2d5bf Mon Sep 17 00:00:00 2001 From: javanna Date: Wed, 19 Apr 2017 14:37:17 +0200 Subject: [PATCH 1/4] Make Aggregations an abstract class rather than an interface 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. --- .../search/aggregations/Aggregations.java | 69 +++++++++++- .../aggregations/InternalAggregations.java | 106 ++---------------- 2 files changed, 75 insertions(+), 100 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java b/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java index f7fe3e49f0a18..76dbbb57247e2 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java @@ -18,31 +18,88 @@ */ package org.elasticsearch.search.aggregations; +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.unmodifiableMap; /** - * Represents a set of computed addAggregation. + * Represents a set of {@link Aggregation}s */ -public interface Aggregations extends Iterable { +public abstract class Aggregations implements Iterable { + + protected List aggregations = Collections.emptyList(); + protected Map aggregationsAsMap; + + protected Aggregations() { + + } + + protected Aggregations(List aggregations) { + this.aggregations = aggregations; + } + + /** + * Iterates over the {@link Aggregation}s. + */ + + @Override + public final Iterator iterator() { + return aggregations.stream().map((p) -> (Aggregation) p).iterator(); + } /** * The list of {@link Aggregation}s. */ - List asList(); + public final List asList() { + return aggregations.stream().map((p) -> p).collect(Collectors.toList()); + } /** * Returns the {@link Aggregation}s keyed by aggregation name. */ - Map asMap(); + public final Map asMap() { + return getAsMap(); + } /** * Returns the {@link Aggregation}s keyed by aggregation name. */ - Map getAsMap(); + public final Map getAsMap() { + if (aggregationsAsMap == null) { + Map newAggregationsAsMap = new HashMap<>(); + for (Aggregation aggregation : aggregations) { + newAggregationsAsMap.put(aggregation.getName(), aggregation); + } + this.aggregationsAsMap = unmodifiableMap(newAggregationsAsMap); + } + return aggregationsAsMap; + } /** * Returns the aggregation that is associated with the specified name. */ - A get(String name); + @SuppressWarnings("unchecked") + public final A get(String name) { + return (A) asMap().get(name); + } + + @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); + } + } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java b/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java index c835c47c148b9..af0f0ed6b731d 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java @@ -27,27 +27,18 @@ 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 class InternalAggregations extends Aggregations implements ToXContent, Streamable { public static final InternalAggregations EMPTY = new InternalAggregations(); - private List aggregations = Collections.emptyList(); - - private Map aggregationsAsMap; - private InternalAggregations() { } @@ -55,55 +46,7 @@ private InternalAggregations() { * Constructs a new addAggregation. */ public InternalAggregations(List aggregations) { - this.aggregations = aggregations; - } - - /** - * Iterates over the {@link Aggregation}s. - */ - @Override - public Iterator iterator() { - return aggregations.stream().map((p) -> (Aggregation) p).iterator(); - } - - /** - * The list of {@link Aggregation}s. - */ - @Override - public List asList() { - return aggregations.stream().map((p) -> (Aggregation) p).collect(Collectors.toList()); - } - - /** - * Returns the {@link Aggregation}s keyed by map. - */ - @Override - public Map asMap() { - return getAsMap(); - } - - /** - * Returns the {@link Aggregation}s keyed by map. - */ - @Override - public Map getAsMap() { - if (aggregationsAsMap == null) { - Map 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 get(String name) { - return (A) asMap().get(name); + super(aggregations); } /** @@ -118,21 +61,16 @@ public static InternalAggregations reduce(List aggregation } // first we collect all aggregations of the same type and list them together - Map> aggByName = new HashMap<>(); for (InternalAggregations aggregations : aggregationsList) { - for (InternalAggregation aggregation : aggregations.aggregations) { - List 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 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 reducedAggregations = new ArrayList<>(); for (Map.Entry> entry : aggByName.entrySet()) { List aggregations = entry.getValue(); @@ -142,41 +80,33 @@ public static InternalAggregations reduce(List 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 { 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)aggregations); } } From 3da1d6eb889d2fb2bd4d77b8514a5453c6a205ad Mon Sep 17 00:00:00 2001 From: javanna Date: Wed, 19 Apr 2017 14:43:59 +0200 Subject: [PATCH 2/4] make some methods final --- .../org/elasticsearch/search/aggregations/Aggregations.java | 5 ++--- .../search/aggregations/InternalAggregations.java | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java b/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java index 76dbbb57247e2..88fe320cce04e 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java @@ -90,7 +90,7 @@ public final A get(String name) { } @Override - public boolean equals(Object obj) { + public final boolean equals(Object obj) { if (obj == null || getClass() != obj.getClass()) { return false; } @@ -98,8 +98,7 @@ public boolean equals(Object obj) { } @Override - public int hashCode() { + public final int hashCode() { return Objects.hash(getClass(), aggregations); } - } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java b/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java index af0f0ed6b731d..64c9e4794c338 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java @@ -35,7 +35,7 @@ /** * An internal implementation of {@link Aggregations}. */ -public class InternalAggregations extends Aggregations implements ToXContent, Streamable { +public final class InternalAggregations extends Aggregations implements ToXContent, Streamable { public static final InternalAggregations EMPTY = new InternalAggregations(); From 9629995f5303520e0c27fdb8c94c728b7c09c644 Mon Sep 17 00:00:00 2001 From: javanna Date: Wed, 19 Apr 2017 15:00:03 +0200 Subject: [PATCH 3/4] address review comments --- .../search/aggregations/Aggregations.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java b/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java index 88fe320cce04e..98aa8bd39c7be 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java @@ -18,13 +18,13 @@ */ package org.elasticsearch.search.aggregations; +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.unmodifiableMap; @@ -37,7 +37,6 @@ public abstract class Aggregations implements Iterable { protected Map aggregationsAsMap; protected Aggregations() { - } protected Aggregations(List aggregations) { @@ -47,7 +46,6 @@ protected Aggregations(List aggregations) { /** * Iterates over the {@link Aggregation}s. */ - @Override public final Iterator iterator() { return aggregations.stream().map((p) -> (Aggregation) p).iterator(); @@ -57,7 +55,7 @@ public final Iterator iterator() { * The list of {@link Aggregation}s. */ public final List asList() { - return aggregations.stream().map((p) -> p).collect(Collectors.toList()); + return new ArrayList<>(aggregations); } /** @@ -72,7 +70,7 @@ public final Map asMap() { */ public final Map getAsMap() { if (aggregationsAsMap == null) { - Map newAggregationsAsMap = new HashMap<>(); + Map newAggregationsAsMap = new HashMap<>(aggregations.size()); for (Aggregation aggregation : aggregations) { newAggregationsAsMap.put(aggregation.getName(), aggregation); } @@ -94,7 +92,7 @@ public final boolean equals(Object obj) { if (obj == null || getClass() != obj.getClass()) { return false; } - return aggregations.equals(((InternalAggregations) obj).aggregations); + return aggregations.equals(((Aggregations) obj).aggregations); } @Override From ea579936674027287a5fd8d4f3e679fdc767fddb Mon Sep 17 00:00:00 2001 From: javanna Date: Wed, 19 Apr 2017 15:14:43 +0200 Subject: [PATCH 4/4] address review comments --- .../org/elasticsearch/search/aggregations/Aggregations.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java b/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java index 98aa8bd39c7be..ef84edfb72957 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java @@ -18,7 +18,6 @@ */ package org.elasticsearch.search.aggregations; -import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.Iterator; @@ -55,7 +54,7 @@ public final Iterator iterator() { * The list of {@link Aggregation}s. */ public final List asList() { - return new ArrayList<>(aggregations); + return Collections.unmodifiableList(aggregations); } /**