From 06bd896ce04c68e69ddc722b76589dbd2ca9af0c Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 11 Jul 2016 16:26:54 -0400 Subject: [PATCH] Migrate geohash_grid and geo_bounds to NamedWriteable Just another small step in removing Aggregation's custom streams implementation in favor of NamedWriteable. --- .../resources/checkstyle_suppressions.xml | 1 - .../elasticsearch/search/SearchModule.java | 8 +- .../geogrid/GeoGridAggregationBuilder.java | 8 +- .../bucket/geogrid/InternalGeoHashGrid.java | 111 +++++++----------- .../GeoBoundsAggregationBuilder.java | 8 +- .../metrics/geobounds/InternalGeoBounds.java | 87 ++++++-------- .../aggregations/metrics/GeoBoundsIT.java | 3 - 7 files changed, 90 insertions(+), 136 deletions(-) diff --git a/buildSrc/src/main/resources/checkstyle_suppressions.xml b/buildSrc/src/main/resources/checkstyle_suppressions.xml index 0a83640c701..c61d86abd9d 100644 --- a/buildSrc/src/main/resources/checkstyle_suppressions.xml +++ b/buildSrc/src/main/resources/checkstyle_suppressions.xml @@ -604,7 +604,6 @@ - diff --git a/core/src/main/java/org/elasticsearch/search/SearchModule.java b/core/src/main/java/org/elasticsearch/search/SearchModule.java index 9f729aa77d5..b43aaef974d 100644 --- a/core/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/core/src/main/java/org/elasticsearch/search/SearchModule.java @@ -542,14 +542,16 @@ public class SearchModule extends AbstractModule { DateHistogramAggregationBuilder.AGGREGATION_NAME_FIELD); registerAggregation(new AggregationSpec(GeoDistanceAggregationBuilder::new, new GeoDistanceParser(), GeoDistanceAggregationBuilder.AGGREGATION_NAME_FIELD).addResultReader(InternalGeoDistance::new)); - registerAggregation(GeoGridAggregationBuilder::new, new GeoHashGridParser(), GeoGridAggregationBuilder.AGGREGATION_NAME_FIELD); + registerAggregation(new AggregationSpec(GeoGridAggregationBuilder::new, new GeoHashGridParser(), + GeoGridAggregationBuilder.AGGREGATION_NAME_FIELD).addResultReader(InternalGeoHashGrid::new)); registerAggregation(NestedAggregationBuilder::new, NestedAggregationBuilder::parse, NestedAggregationBuilder.AGGREGATION_FIELD_NAME); registerAggregation(ReverseNestedAggregationBuilder::new, ReverseNestedAggregationBuilder::parse, ReverseNestedAggregationBuilder.AGGREGATION_NAME_FIELD); registerAggregation(TopHitsAggregationBuilder::new, TopHitsAggregationBuilder::parse, TopHitsAggregationBuilder.AGGREGATION_NAME_FIELD); - registerAggregation(GeoBoundsAggregationBuilder::new, new GeoBoundsParser(), GeoBoundsAggregationBuilder.AGGREGATION_NAME_FIED); + registerAggregation(new AggregationSpec(GeoBoundsAggregationBuilder::new, new GeoBoundsParser(), + GeoBoundsAggregationBuilder.AGGREGATION_NAME_FIED).addResultReader(InternalGeoBounds::new)); registerAggregation(new AggregationSpec(GeoCentroidAggregationBuilder::new, new GeoCentroidParser(), GeoCentroidAggregationBuilder.AGGREGATION_NAME_FIELD).addResultReader(InternalGeoCentroid::new)); registerAggregation(new AggregationSpec(ScriptedMetricAggregationBuilder::new, ScriptedMetricAggregationBuilder::parse, @@ -820,13 +822,11 @@ public class SearchModule extends AbstractModule { static { // buckets - InternalGeoHashGrid.registerStreams(); InternalBinaryRange.registerStream(); InternalHistogram.registerStream(); InternalNested.registerStream(); InternalReverseNested.registerStream(); InternalTopHits.registerStreams(); - InternalGeoBounds.registerStream(); InternalChildren.registerStream(); // Pipeline Aggregations diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java index 4f00a49a1d9..8aa4f6ab598 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java @@ -33,6 +33,7 @@ import org.elasticsearch.index.fielddata.SortedBinaryDocValues; import org.elasticsearch.index.fielddata.SortedNumericDoubleValues; import org.elasticsearch.index.fielddata.SortingNumericDocValues; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; +import org.elasticsearch.search.aggregations.InternalAggregation.Type; import org.elasticsearch.search.aggregations.AggregatorFactory; import org.elasticsearch.search.aggregations.bucket.BucketUtils; import org.elasticsearch.search.aggregations.support.AggregationContext; @@ -47,7 +48,8 @@ import java.io.IOException; import java.util.Objects; public class GeoGridAggregationBuilder extends ValuesSourceAggregationBuilder { - public static final String NAME = InternalGeoHashGrid.TYPE.name(); + public static final String NAME = "geohash_grid"; + private static final Type TYPE = new Type(NAME); public static final ParseField AGGREGATION_NAME_FIELD = new ParseField(NAME); private int precision = GeoHashGridParser.DEFAULT_PRECISION; @@ -55,14 +57,14 @@ public class GeoGridAggregationBuilder extends ValuesSourceAggregationBuilder implements GeoHashGrid { - - public static final Type TYPE = new Type("geohash_grid", "ghcells"); - - public static final AggregationStreams.Stream STREAM = new AggregationStreams.Stream() { - @Override - public InternalGeoHashGrid readResult(StreamInput in) throws IOException { - InternalGeoHashGrid buckets = new InternalGeoHashGrid(); - buckets.readFrom(in); - return buckets; - } - }; - - public static void registerStreams() { - AggregationStreams.registerStream(STREAM, TYPE.stream()); - } - - static class Bucket extends InternalMultiBucketAggregation.InternalBucket implements GeoHashGrid.Bucket, Comparable { protected long geohashAsLong; protected long docCount; protected InternalAggregations aggregations; - public Bucket() { - // For Serialization only - } - public Bucket(long geohashAsLong, long docCount, InternalAggregations aggregations) { this.docCount = docCount; this.aggregations = aggregations; this.geohashAsLong = geohashAsLong; } + /** + * Read from a stream. + */ + private Bucket(StreamInput in) throws IOException { + geohashAsLong = in.readLong(); + docCount = in.readVLong(); + aggregations = InternalAggregations.readAggregations(in); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeLong(geohashAsLong); + out.writeVLong(docCount); + aggregations.writeTo(out); + } + + @Override public String getKeyAsString() { return GeoHashUtils.stringEncode(geohashAsLong); @@ -121,20 +117,6 @@ public class InternalGeoHashGrid extends InternalMultiBucketAggregation buckets; - protected Map bucketMap; - InternalGeoHashGrid() { - } // for serialization + private final int requiredSize; + private final List buckets; - public InternalGeoHashGrid(String name, int requiredSize, Collection buckets, List pipelineAggregators, + public InternalGeoHashGrid(String name, int requiredSize, List buckets, List pipelineAggregators, Map metaData) { super(name, pipelineAggregators, metaData); this.requiredSize = requiredSize; this.buckets = buckets; } + /** + * Read from a stream. + */ + public InternalGeoHashGrid(StreamInput in) throws IOException { + super(in); + requiredSize = readSize(in); + buckets = in.readList(Bucket::new); + } + @Override - public Type type() { - return TYPE; + protected void doWriteTo(StreamOutput out) throws IOException { + writeSize(requiredSize, out); + out.writeList(buckets); + } + + @Override + public String getWriteableName() { + return GeoGridAggregationBuilder.NAME; } @Override @@ -174,11 +168,9 @@ public class InternalGeoHashGrid extends InternalMultiBucketAggregation getBuckets() { - Object o = buckets; - return (List) o; + return unmodifiableList(buckets); } @Override @@ -214,29 +206,6 @@ public class InternalGeoHashGrid extends InternalMultiBucketAggregation buckets = new ArrayList<>(size); - for (int i = 0; i < size; i++) { - Bucket bucket = new Bucket(); - bucket.readFrom(in); - buckets.add(bucket); - } - this.buckets = buckets; - this.bucketMap = null; - } - - @Override - protected void doWriteTo(StreamOutput out) throws IOException { - writeSize(requiredSize, out); - out.writeVInt(buckets.size()); - for (Bucket bucket : buckets) { - bucket.writeTo(out); - } - } - @Override public XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { builder.startArray(CommonFields.BUCKETS); diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/geobounds/GeoBoundsAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/geobounds/GeoBoundsAggregationBuilder.java index eff020ec610..833787738e5 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/geobounds/GeoBoundsAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/geobounds/GeoBoundsAggregationBuilder.java @@ -24,6 +24,7 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; +import org.elasticsearch.search.aggregations.InternalAggregation.Type; import org.elasticsearch.search.aggregations.AggregatorFactory; import org.elasticsearch.search.aggregations.support.AggregationContext; import org.elasticsearch.search.aggregations.support.ValueType; @@ -36,20 +37,21 @@ import java.io.IOException; import java.util.Objects; public class GeoBoundsAggregationBuilder extends ValuesSourceAggregationBuilder { - public static final String NAME = InternalGeoBounds.TYPE.name(); + public static final String NAME = "geo_bounds"; + private static final Type TYPE = new Type(NAME); public static final ParseField AGGREGATION_NAME_FIED = new ParseField(NAME); private boolean wrapLongitude = true; public GeoBoundsAggregationBuilder(String name) { - super(name, InternalGeoBounds.TYPE, ValuesSourceType.GEOPOINT, ValueType.GEOPOINT); + super(name, TYPE, ValuesSourceType.GEOPOINT, ValueType.GEOPOINT); } /** * Read from a stream. */ public GeoBoundsAggregationBuilder(StreamInput in) throws IOException { - super(in, InternalGeoBounds.TYPE, ValuesSourceType.GEOPOINT, ValueType.GEOPOINT); + super(in, TYPE, ValuesSourceType.GEOPOINT, ValueType.GEOPOINT); wrapLongitude = in.readBoolean(); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/geobounds/InternalGeoBounds.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/geobounds/InternalGeoBounds.java index 0b41d5d4939..b999936693d 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/geobounds/InternalGeoBounds.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/geobounds/InternalGeoBounds.java @@ -23,7 +23,6 @@ import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.search.aggregations.AggregationStreams; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.metrics.InternalMetricsAggregation; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; @@ -33,28 +32,14 @@ import java.util.List; import java.util.Map; public class InternalGeoBounds extends InternalMetricsAggregation implements GeoBounds { + private final double top; + private final double bottom; + private final double posLeft; + private final double posRight; + private final double negLeft; + private final double negRight; + private final boolean wrapLongitude; - public static final Type TYPE = new Type("geo_bounds"); - public static final AggregationStreams.Stream STREAM = new AggregationStreams.Stream() { - @Override - public InternalGeoBounds readResult(StreamInput in) throws IOException { - InternalGeoBounds result = new InternalGeoBounds(); - result.readFrom(in); - return result; - } - }; - - private double top; - private double bottom; - private double posLeft; - private double posRight; - private double negLeft; - private double negRight; - private boolean wrapLongitude; - - InternalGeoBounds() { - } - InternalGeoBounds(String name, double top, double bottom, double posLeft, double posRight, double negLeft, double negRight, boolean wrapLongitude, List pipelineAggregators, Map metaData) { @@ -68,9 +53,34 @@ public class InternalGeoBounds extends InternalMetricsAggregation implements Geo this.wrapLongitude = wrapLongitude; } + /** + * Read from a stream. + */ + public InternalGeoBounds(StreamInput in) throws IOException { + super(in); + top = in.readDouble(); + bottom = in.readDouble(); + posLeft = in.readDouble(); + posRight = in.readDouble(); + negLeft = in.readDouble(); + negRight = in.readDouble(); + wrapLongitude = in.readBoolean(); + } + @Override - public Type type() { - return TYPE; + protected void doWriteTo(StreamOutput out) throws IOException { + out.writeDouble(top); + out.writeDouble(bottom); + out.writeDouble(posLeft); + out.writeDouble(posRight); + out.writeDouble(negLeft); + out.writeDouble(negRight); + out.writeBoolean(wrapLongitude); + } + + @Override + public String getWriteableName() { + return GeoBoundsAggregationBuilder.NAME; } @Override @@ -104,7 +114,8 @@ public class InternalGeoBounds extends InternalMetricsAggregation implements Geo negRight = bounds.negRight; } } - return new InternalGeoBounds(name, top, bottom, posLeft, posRight, negLeft, negRight, wrapLongitude, pipelineAggregators(), getMetaData()); + return new InternalGeoBounds(name, top, bottom, posLeft, posRight, negLeft, negRight, wrapLongitude, pipelineAggregators(), + getMetaData()); } @Override @@ -173,32 +184,6 @@ public class InternalGeoBounds extends InternalMetricsAggregation implements Geo return builder; } - @Override - protected void doReadFrom(StreamInput in) throws IOException { - top = in.readDouble(); - bottom = in.readDouble(); - posLeft = in.readDouble(); - posRight = in.readDouble(); - negLeft = in.readDouble(); - negRight = in.readDouble(); - wrapLongitude = in.readBoolean(); - } - - @Override - protected void doWriteTo(StreamOutput out) throws IOException { - out.writeDouble(top); - out.writeDouble(bottom); - out.writeDouble(posLeft); - out.writeDouble(posRight); - out.writeDouble(negLeft); - out.writeDouble(negRight); - out.writeBoolean(wrapLongitude); - } - - public static void registerStream() { - AggregationStreams.registerStream(STREAM, TYPE.stream()); - } - private static class BoundingBox { private final GeoPoint topLeft; private final GeoPoint bottomRight; diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoBoundsIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoBoundsIT.java index d97bc824602..3afba951b4a 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoBoundsIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoBoundsIT.java @@ -44,9 +44,6 @@ import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.sameInstance; -/** - * - */ @ESIntegTestCase.SuiteScopeTestCase public class GeoBoundsIT extends AbstractGeoTestCase { private static final String aggName = "geoBounds";