From 8795e95d647e956296185901a8f57d1940582ef5 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Wed, 2 May 2018 04:37:51 +0300 Subject: [PATCH 01/18] Refactored geogrid to support multiple hash types This patch should not introduce any changes in the existing ES behavior. Its only goal is to allow subsequent addition of various hashing algorithms, such as quadkey, pluscode, hex, ... --- .../elasticsearch/common/geo/GeoUtils.java | 35 +++--- .../geogrid/GeoGridAggregationBuilder.java | 112 ++++++++++++++---- .../bucket/geogrid/GeoHashGridAggregator.java | 10 +- .../geogrid/GeoHashGridAggregatorFactory.java | 8 +- .../bucket/geogrid/GeoHashGridParams.java | 1 + .../bucket/geogrid/GeoHashType.java | 55 +++++++++ .../bucket/geogrid/InternalGeoHashGrid.java | 29 +++-- .../geogrid/GeoHashGridAggregatorTests.java | 27 +++-- .../geogrid/GeoHashGridParserTests.java | 17 +-- .../geogrid/InternalGeoHashGridTests.java | 7 +- 10 files changed, 220 insertions(+), 81 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index 2f3443639cdb7..64398be4c2e8d 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -32,7 +32,6 @@ import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentParser.Token; import org.elasticsearch.common.xcontent.json.JsonXContent; -import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.index.fielddata.FieldData; import org.elasticsearch.index.fielddata.GeoPointValues; import org.elasticsearch.index.fielddata.MultiGeoPointValues; @@ -548,23 +547,25 @@ private static GeoPoint parseGeoHash(GeoPoint point, String geohash, EffectivePo * @return int representing precision */ public static int parsePrecision(XContentParser parser) throws IOException, ElasticsearchParseException { - XContentParser.Token token = parser.currentToken(); - if (token.equals(XContentParser.Token.VALUE_NUMBER)) { - return XContentMapValues.nodeIntegerValue(parser.intValue()); - } else { - String precision = parser.text(); + return parser.currentToken() == Token.VALUE_NUMBER ? parser.intValue() : parsePrecisionString(parser.text()); + } + + /** + * Attempt to parse geohash precision string into an integer value + */ + public static int parsePrecisionString(String precision) { + try { + // we want to treat simple integer strings as precision levels, not distances + return checkPrecisionRange(Integer.parseInt(precision)); + // Do not catch IllegalArgumentException here + } catch (NumberFormatException e) { + // try to parse as a distance value + final int parsedPrecision = GeoUtils.geoHashLevelsForPrecision(precision); try { - // we want to treat simple integer strings as precision levels, not distances - return XContentMapValues.nodeIntegerValue(precision); - } catch (NumberFormatException e) { - // try to parse as a distance value - final int parsedPrecision = GeoUtils.geoHashLevelsForPrecision(precision); - try { - return checkPrecisionRange(parsedPrecision); - } catch (IllegalArgumentException e2) { - // this happens when distance too small, so precision > 12. We'd like to see the original string - throw new IllegalArgumentException("precision too high [" + precision + "]", e2); - } + return checkPrecisionRange(parsedPrecision); + } catch (IllegalArgumentException e2) { + // this happens when distance too small, so precision > 12. We'd like to see the original string + throw new IllegalArgumentException("precision too high [" + precision + "]", e2); } } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java index 2f66531834d38..aeb1723be011d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java @@ -30,13 +30,11 @@ import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.index.fielddata.AbstractSortingNumericDocValues; import org.elasticsearch.index.fielddata.MultiGeoPointValues; import org.elasticsearch.index.fielddata.SortedBinaryDocValues; import org.elasticsearch.index.fielddata.SortedNumericDoubleValues; import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactories; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; import org.elasticsearch.search.aggregations.AggregatorFactory; import org.elasticsearch.search.aggregations.bucket.BucketUtils; @@ -53,30 +51,43 @@ import java.io.IOException; import java.util.Map; import java.util.Objects; - -import static org.elasticsearch.common.geo.GeoUtils.parsePrecision; +import java.util.stream.Collectors; +import java.util.stream.Stream; public class GeoGridAggregationBuilder extends ValuesSourceAggregationBuilder implements MultiBucketAggregationBuilder { public static final String NAME = "geohash_grid"; - public static final int DEFAULT_PRECISION = 5; + public static final GeoHashType DEFAULT_TYPE = GeoHashType.GEOHASH; public static final int DEFAULT_MAX_NUM_CELLS = 10000; private static final ObjectParser PARSER; static { PARSER = new ObjectParser<>(GeoGridAggregationBuilder.NAME); ValuesSourceParserHelper.declareGeoFields(PARSER, false, false); - PARSER.declareField((parser, builder, context) -> builder.precision(parsePrecision(parser)), GeoHashGridParams.FIELD_PRECISION, - org.elasticsearch.common.xcontent.ObjectParser.ValueType.INT); + PARSER.declareString(GeoGridAggregationBuilder::type, GeoHashGridParams.FIELD_TYPE); + // ValueType.INT supports both numbers and strings + PARSER.declareField(GeoGridAggregationBuilder::parsePrecision, GeoHashGridParams.FIELD_PRECISION, ObjectParser.ValueType.INT); PARSER.declareInt(GeoGridAggregationBuilder::size, GeoHashGridParams.FIELD_SIZE); PARSER.declareInt(GeoGridAggregationBuilder::shardSize, GeoHashGridParams.FIELD_SHARD_SIZE); } + private static void parsePrecision(XContentParser parser, GeoGridAggregationBuilder builder, Void context) + throws IOException { + // In some cases, this value cannot be fully parsed until after we know the type + // Declare precision as an ObjectParser.ValueType.STRING, and convert it to String if needed + if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { + builder.precision(Integer.toString(parser.intValue())); + } else { + builder.precision(parser.text()); + } + } + public static GeoGridAggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { return PARSER.parse(parser, new GeoGridAggregationBuilder(aggregationName), null); } - private int precision = DEFAULT_PRECISION; + private GeoHashType type = DEFAULT_TYPE; + private String precision = null; private int requiredSize = DEFAULT_MAX_NUM_CELLS; private int shardSize = -1; @@ -86,6 +97,7 @@ public GeoGridAggregationBuilder(String name) { protected GeoGridAggregationBuilder(GeoGridAggregationBuilder clone, Builder factoriesBuilder, Map metaData) { super(clone, factoriesBuilder, metaData); + this.type = clone.type; this.precision = clone.precision; this.requiredSize = clone.requiredSize; this.shardSize = clone.shardSize; @@ -101,24 +113,45 @@ protected AggregationBuilder shallowCopy(Builder factoriesBuilder, Map innerBuild(SearchContext context, ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { + + // delayed precision validation and defaults + int precision; + switch (type) { + case GEOHASH: + if (this.precision != null) { + precision = GeoUtils.parsePrecisionString(this.precision); + } else { + precision = 5; + } + break; + default: + throw new IllegalArgumentException("Unknown type " + type.toString()); + } + int shardSize = this.shardSize; int requiredSize = this.requiredSize; @@ -170,12 +218,13 @@ public int shardSize() { if (shardSize < requiredSize) { shardSize = requiredSize; } - return new GeoHashGridAggregatorFactory(name, config, precision, requiredSize, shardSize, context, parent, + return new GeoHashGridAggregatorFactory(name, config, type, precision, requiredSize, shardSize, context, parent, subFactoriesBuilder, metaData); } @Override protected XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { + builder.field(GeoHashGridParams.FIELD_TYPE.getPreferredName(), type); builder.field(GeoHashGridParams.FIELD_PRECISION.getPreferredName(), precision); builder.field(GeoHashGridParams.FIELD_SIZE.getPreferredName(), requiredSize); if (shardSize > -1) { @@ -187,7 +236,10 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params) @Override protected boolean innerEquals(Object obj) { GeoGridAggregationBuilder other = (GeoGridAggregationBuilder) obj; - if (precision != other.precision) { + if (type != other.type) { + return false; + } + if (!Objects.equals(precision, other.precision)) { return false; } if (requiredSize != other.requiredSize) { @@ -201,7 +253,7 @@ protected boolean innerEquals(Object obj) { @Override protected int innerHashCode() { - return Objects.hash(precision, requiredSize, shardSize); + return Objects.hash(type, precision, requiredSize, shardSize); } @Override @@ -211,10 +263,12 @@ public String getType() { private static class CellValues extends AbstractSortingNumericDocValues { private MultiGeoPointValues geoValues; + private GeoHashType type; private int precision; - protected CellValues(MultiGeoPointValues geoValues, int precision) { + protected CellValues(MultiGeoPointValues geoValues, GeoHashType type, int precision) { this.geoValues = geoValues; + this.type = type; this.precision = precision; } @@ -224,8 +278,14 @@ public boolean advanceExact(int docId) throws IOException { resize(geoValues.docValueCount()); for (int i = 0; i < docValueCount(); ++i) { GeoPoint target = geoValues.nextValue(); - values[i] = GeoHashUtils.longEncode(target.getLon(), target.getLat(), - precision); + + switch (type) { + case GEOHASH: + values[i] = GeoHashUtils.longEncode(target.getLon(), target.getLat(), precision); + break; + default: + throw new IllegalArgumentException(); + } } sort(); return true; @@ -237,14 +297,20 @@ public boolean advanceExact(int docId) throws IOException { static class CellIdSource extends ValuesSource.Numeric { private final ValuesSource.GeoPoint valuesSource; + private final GeoHashType type; private final int precision; - CellIdSource(ValuesSource.GeoPoint valuesSource, int precision) { + CellIdSource(ValuesSource.GeoPoint valuesSource, GeoHashType type, int precision) { this.valuesSource = valuesSource; //different GeoPoints could map to the same or different geohash cells. + this.type = type; this.precision = precision; } + public GeoHashType type() { + return type; + } + public int precision() { return precision; } @@ -256,7 +322,7 @@ public boolean isFloatingPoint() { @Override public SortedNumericDocValues longValues(LeafReaderContext ctx) { - return new CellValues(valuesSource.geoPointValues(ctx), precision); + return new CellValues(valuesSource.geoPointValues(ctx), type, precision); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregator.java index ec54abb334056..25e0f9d53a157 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregator.java @@ -45,14 +45,16 @@ public class GeoHashGridAggregator extends BucketsAggregator { private final int shardSize; private final GeoGridAggregationBuilder.CellIdSource valuesSource; private final LongHash bucketOrds; + private final GeoHashType type; GeoHashGridAggregator(String name, AggregatorFactories factories, GeoGridAggregationBuilder.CellIdSource valuesSource, int requiredSize, int shardSize, SearchContext aggregationContext, Aggregator parent, List pipelineAggregators, - Map metaData) throws IOException { + Map metaData, GeoHashType type) throws IOException { super(name, factories, aggregationContext, parent, pipelineAggregators, metaData); this.valuesSource = valuesSource; this.requiredSize = requiredSize; this.shardSize = shardSize; + this.type = type; bucketOrds = new LongHash(1, aggregationContext.bigArrays()); } @@ -96,8 +98,8 @@ static class OrdinalBucket extends InternalGeoHashGrid.Bucket { long bucketOrd; - OrdinalBucket() { - super(0, 0, null); + OrdinalBucket(GeoHashType type) { + super(type, 0, 0, null); } } @@ -112,7 +114,7 @@ public InternalGeoHashGrid buildAggregation(long owningBucketOrdinal) throws IOE OrdinalBucket spare = null; for (long i = 0; i < bucketOrds.size(); i++) { if (spare == null) { - spare = new OrdinalBucket(); + spare = new OrdinalBucket(type); } spare.geohashAsLong = bucketOrds.get(i); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java index 13b4850156483..4d20338b080fd 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java @@ -39,14 +39,16 @@ public class GeoHashGridAggregatorFactory extends ValuesSourceAggregatorFactory { + private final GeoHashType type; private final int precision; private final int requiredSize; private final int shardSize; - GeoHashGridAggregatorFactory(String name, ValuesSourceConfig config, int precision, int requiredSize, + GeoHashGridAggregatorFactory(String name, ValuesSourceConfig config, GeoHashType type, int precision, int requiredSize, int shardSize, SearchContext context, AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { super(name, config, context, parent, subFactoriesBuilder, metaData); + this.type = type; this.precision = precision; this.requiredSize = requiredSize; this.shardSize = shardSize; @@ -71,9 +73,9 @@ protected Aggregator doCreateInternal(final ValuesSource.GeoPoint valuesSource, if (collectsFromSingleBucket == false) { return asMultiBucketAggregator(this, context, parent); } - CellIdSource cellIdSource = new CellIdSource(valuesSource, precision); + CellIdSource cellIdSource = new CellIdSource(valuesSource, type, precision); return new GeoHashGridAggregator(name, factories, cellIdSource, requiredSize, shardSize, context, parent, - pipelineAggregators, metaData); + pipelineAggregators, metaData, type); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParams.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParams.java index ff3b21a3a7bae..b16487e4236ab 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParams.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParams.java @@ -26,6 +26,7 @@ final class GeoHashGridParams { /* recognized field names in JSON */ + static final ParseField FIELD_TYPE = new ParseField("type"); static final ParseField FIELD_PRECISION = new ParseField("precision"); static final ParseField FIELD_SIZE = new ParseField("size"); static final ParseField FIELD_SHARD_SIZE = new ParseField("shard_size"); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java new file mode 100644 index 0000000000000..2f49eb7b94e88 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java @@ -0,0 +1,55 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.search.aggregations.bucket.geogrid; + +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.io.stream.Writeable; + +import java.io.IOException; +import java.util.Locale; + +public enum GeoHashType implements Writeable { + GEOHASH; + + /** + * Case-insensitive from string method. + * + * @param value String representation + * @return The hash type + */ + public static GeoHashType forString(String value) { + return valueOf(value.toUpperCase(Locale.ROOT)); + } + + public static GeoHashType readFromStream(StreamInput in) throws IOException { + return in.readEnum(GeoHashType.class); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeEnum(this); + } + + @Override + public String toString() { + return name().toLowerCase(Locale.ROOT); + } +} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java index bc60f5945eb9f..30d7b56fd3b0e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java @@ -49,11 +49,13 @@ public class InternalGeoHashGrid extends InternalMultiBucketAggregation { + protected GeoHashType type; protected long geohashAsLong; protected long docCount; protected InternalAggregations aggregations; - Bucket(long geohashAsLong, long docCount, InternalAggregations aggregations) { + Bucket(GeoHashType type, long geohashAsLong, long docCount, InternalAggregations aggregations) { + this.type = type; this.docCount = docCount; this.aggregations = aggregations; this.geohashAsLong = geohashAsLong; @@ -63,6 +65,7 @@ static class Bucket extends InternalMultiBucketAggregation.InternalBucket implem * Read from a stream. */ private Bucket(StreamInput in) throws IOException { + type = GeoHashType.readFromStream(in); geohashAsLong = in.readLong(); docCount = in.readVLong(); aggregations = InternalAggregations.readAggregations(in); @@ -70,6 +73,7 @@ private Bucket(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { + type.writeTo(out); out.writeLong(geohashAsLong); out.writeVLong(docCount); aggregations.writeTo(out); @@ -77,12 +81,22 @@ public void writeTo(StreamOutput out) throws IOException { @Override public String getKeyAsString() { - return GeoHashUtils.stringEncode(geohashAsLong); + switch(type) { + case GEOHASH: + return GeoHashUtils.stringEncode(geohashAsLong); + default: + throw new IllegalArgumentException(); + } } @Override - public GeoPoint getKey() { - return GeoPoint.fromGeohash(geohashAsLong); + public Object getKey() { + switch(type) { + case GEOHASH: + return GeoPoint.fromGeohash(geohashAsLong); + default: + throw new IllegalArgumentException(); + } } @Override @@ -114,7 +128,7 @@ public Bucket reduce(List buckets, ReduceContext context) { aggregationsList.add(bucket.aggregations); } final InternalAggregations aggs = InternalAggregations.reduce(aggregationsList, context); - return new Bucket(geohashAsLong, docCount, aggs); + return new Bucket(type, geohashAsLong, docCount, aggs); } @Override @@ -133,13 +147,14 @@ public boolean equals(Object o) { if (o == null || getClass() != o.getClass()) return false; Bucket bucket = (Bucket) o; return geohashAsLong == bucket.geohashAsLong && + type == bucket.type && docCount == bucket.docCount && Objects.equals(aggregations, bucket.aggregations); } @Override public int hashCode() { - return Objects.hash(geohashAsLong, docCount, aggregations); + return Objects.hash(type, geohashAsLong, docCount, aggregations); } } @@ -181,7 +196,7 @@ public InternalGeoHashGrid create(List buckets) { @Override public Bucket createBucket(InternalAggregations aggregations, Bucket prototype) { - return new Bucket(prototype.geohashAsLong, prototype.docCount, aggregations); + return new Bucket(prototype.type, prototype.geohashAsLong, prototype.docCount, aggregations); } @Override diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java index 45b6b64cddc35..93d0885b092a5 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java @@ -40,16 +40,17 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.BiFunction; import java.util.function.Consumer; -import static org.elasticsearch.common.geo.GeoHashUtils.stringEncode; +import org.elasticsearch.common.geo.GeoHashUtils; public class GeoHashGridAggregatorTests extends AggregatorTestCase { private static final String FIELD_NAME = "location"; public void testNoDocs() throws IOException { - testCase(new MatchAllDocsQuery(), FIELD_NAME, 1, iw -> { + testCase(new MatchAllDocsQuery(), FIELD_NAME, GeoHashType.GEOHASH, 1, iw -> { // Intentionally not writing any docs }, geoHashGrid -> { assertEquals(0, geoHashGrid.getBuckets().size()); @@ -57,25 +58,32 @@ public void testNoDocs() throws IOException { } public void testFieldMissing() throws IOException { - testCase(new MatchAllDocsQuery(), "wrong_field", 1, iw -> { + testCase(new MatchAllDocsQuery(), "wrong_field", GeoHashType.GEOHASH, 1, iw -> { iw.addDocument(Collections.singleton(new LatLonDocValuesField(FIELD_NAME, 10D, 10D))); }, geoHashGrid -> { assertEquals(0, geoHashGrid.getBuckets().size()); }); } - public void testWithSeveralDocs() throws IOException { + public void testHashcodeWithSeveralDocs() throws IOException { int precision = randomIntBetween(1, 12); + testWithSeveralDocs(GeoHashType.GEOHASH, precision, (lng, lat) -> { + return GeoHashUtils.stringEncode(lng, lat, precision); + }); + } + + private void testWithSeveralDocs(GeoHashType type, int precision, BiFunction hasher) + throws IOException { int numPoints = randomIntBetween(8, 128); Map expectedCountPerGeoHash = new HashMap<>(); - testCase(new MatchAllDocsQuery(), FIELD_NAME, precision, iw -> { + testCase(new MatchAllDocsQuery(), FIELD_NAME, type, precision, iw -> { List points = new ArrayList<>(); Set distinctHashesPerDoc = new HashSet<>(); for (int pointId = 0; pointId < numPoints; pointId++) { double lat = (180d * randomDouble()) - 90d; double lng = (360d * randomDouble()) - 180d; points.add(new LatLonDocValuesField(FIELD_NAME, lat, lng)); - String hash = stringEncode(lng, lat, precision); + String hash = hasher.apply(lng, lat); if (distinctHashesPerDoc.contains(hash) == false) { expectedCountPerGeoHash.put(hash, expectedCountPerGeoHash.getOrDefault(hash, 0) + 1); } @@ -97,8 +105,8 @@ public void testWithSeveralDocs() throws IOException { }); } - private void testCase(Query query, String field, int precision, CheckedConsumer buildIndex, - Consumer verify) throws IOException { + private void testCase(Query query, String field, GeoHashType type, int precision, CheckedConsumer buildIndex, Consumer verify) throws IOException { Directory directory = newDirectory(); RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory); buildIndex.accept(indexWriter); @@ -108,7 +116,8 @@ private void testCase(Query query, String field, int precision, CheckedConsumer< IndexSearcher indexSearcher = newSearcher(indexReader, true, true); GeoGridAggregationBuilder aggregationBuilder = new GeoGridAggregationBuilder("_name").field(field); - aggregationBuilder.precision(precision); + aggregationBuilder.type(type.name()); + aggregationBuilder.precision(Integer.toString(precision)); MappedFieldType fieldType = new GeoPointFieldMapper.GeoPointFieldType(); fieldType.setHasDocValues(true); fieldType.setName(FIELD_NAME); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java index e431bf19ff3de..c938636831733 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java @@ -65,8 +65,8 @@ public void testParseDistanceUnitPrecision() throws Exception { // can create a factory GeoGridAggregationBuilder builder = GeoGridAggregationBuilder.parse("geohash_grid", stParser); assertNotNull(builder); - assertThat(builder.precision(), greaterThanOrEqualTo(0)); - assertThat(builder.precision(), lessThanOrEqualTo(12)); + assertThat(Integer.parseInt(builder.precision()), greaterThanOrEqualTo(0)); + assertThat(Integer.parseInt(builder.precision()), lessThanOrEqualTo(12)); } public void testParseInvalidUnitPrecision() throws Exception { @@ -102,17 +102,4 @@ public void testParseErrorOnBooleanPrecision() throws Exception { assertThat(ExceptionsHelper.detailedMessage(e), containsString("[geohash_grid] precision doesn't support values of type: VALUE_BOOLEAN")); } - - public void testParseErrorOnPrecisionOutOfRange() throws Exception { - XContentParser stParser = createParser(JsonXContent.jsonXContent, "{\"field\":\"my_loc\", \"precision\":\"13\"}"); - XContentParser.Token token = stParser.nextToken(); - assertSame(XContentParser.Token.START_OBJECT, token); - try { - GeoGridAggregationBuilder.parse("geohash_grid", stParser); - fail(); - } catch (XContentParseException ex) { - assertThat(ex.getCause(), instanceOf(IllegalArgumentException.class)); - assertEquals("Invalid geohash aggregation precision of 13. Must be between 1 and 12.", ex.getCause().getMessage()); - } - } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGridTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGridTests.java index 822e05ffa6582..d9f5acf39afb1 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGridTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGridTests.java @@ -56,7 +56,7 @@ protected InternalGeoHashGrid createTestInstance(String name, double longitude = randomDoubleBetween(-180.0, 180.0, false); long geoHashAsLong = GeoHashUtils.longEncode(longitude, latitude, 4); - buckets.add(new InternalGeoHashGrid.Bucket(geoHashAsLong, randomInt(IndexWriter.MAX_DOCS), aggregations)); + buckets.add(new InternalGeoHashGrid.Bucket(GeoHashType.GEOHASH, geoHashAsLong, randomInt(IndexWriter.MAX_DOCS), aggregations)); } return new InternalGeoHashGrid(name, size, buckets, pipelineAggregators, metaData); } @@ -85,7 +85,7 @@ protected void assertReduced(InternalGeoHashGrid reduced, List { int cmp = Long.compare(second.docCount, first.docCount); @@ -124,7 +124,8 @@ protected InternalGeoHashGrid mutateInstance(InternalGeoHashGrid instance) { case 1: buckets = new ArrayList<>(buckets); buckets.add( - new InternalGeoHashGrid.Bucket(randomNonNegativeLong(), randomInt(IndexWriter.MAX_DOCS), InternalAggregations.EMPTY)); + new InternalGeoHashGrid.Bucket(GeoHashType.GEOHASH, randomNonNegativeLong(), + randomInt(IndexWriter.MAX_DOCS), InternalAggregations.EMPTY)); break; case 2: size = size + between(1, 10); From d8245b78fd77d91e0b32b023d2417e21dac7e376 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Wed, 2 May 2018 07:38:02 +0300 Subject: [PATCH 02/18] allow int precision settings --- .../bucket/geogrid/GeoGridAggregationBuilder.java | 8 +++++++- .../bucket/geogrid/GeoHashGridAggregatorTests.java | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java index aeb1723be011d..fa6094543774e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java @@ -76,7 +76,7 @@ private static void parsePrecision(XContentParser parser, GeoGridAggregationBuil // In some cases, this value cannot be fully parsed until after we know the type // Declare precision as an ObjectParser.ValueType.STRING, and convert it to String if needed if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { - builder.precision(Integer.toString(parser.intValue())); + builder.precision(parser.intValue()); } else { builder.precision(parser.text()); } @@ -145,6 +145,12 @@ public GeoHashType type() { return type; } + public GeoGridAggregationBuilder precision(int precision) { + // validation depends on the type, will be checked in innerBuild + this.precision = Integer.toString(precision); + return this; + } + public GeoGridAggregationBuilder precision(String precision) { // validation depends on the type, will be checked in innerBuild this.precision = precision; diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java index 93d0885b092a5..74e3bfed1170b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java @@ -117,7 +117,7 @@ private void testCase(Query query, String field, GeoHashType type, int precision GeoGridAggregationBuilder aggregationBuilder = new GeoGridAggregationBuilder("_name").field(field); aggregationBuilder.type(type.name()); - aggregationBuilder.precision(Integer.toString(precision)); + aggregationBuilder.precision(precision); MappedFieldType fieldType = new GeoPointFieldMapper.GeoPointFieldType(); fieldType.setHasDocValues(true); fieldType.setName(FIELD_NAME); From 5cbad008a9449ab2e201a68da16eb3eda5b296ad Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Wed, 2 May 2018 08:09:07 +0300 Subject: [PATCH 03/18] restored parser test, still broken --- .../bucket/geogrid/GeoHashGridParserTests.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java index c938636831733..d8e4ab1056b63 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java @@ -102,4 +102,17 @@ public void testParseErrorOnBooleanPrecision() throws Exception { assertThat(ExceptionsHelper.detailedMessage(e), containsString("[geohash_grid] precision doesn't support values of type: VALUE_BOOLEAN")); } + + public void testParseErrorOnPrecisionOutOfRange() throws Exception { + XContentParser stParser = createParser(JsonXContent.jsonXContent, "{\"field\":\"my_loc\", \"precision\":\"13\"}"); + XContentParser.Token token = stParser.nextToken(); + assertSame(XContentParser.Token.START_OBJECT, token); + try { + GeoGridAggregationBuilder.parse("geohash_grid", stParser); + fail(); + } catch (XContentParseException ex) { + assertThat(ex.getCause(), instanceOf(IllegalArgumentException.class)); + assertEquals("Invalid geohash aggregation precision of 13. Must be between 1 and 12.", ex.getCause().getMessage()); + } + } } From 2fa03c3882db3ab1845136aa2440a9cb60c4414d Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Thu, 3 May 2018 05:27:33 +0300 Subject: [PATCH 04/18] Interface-based hash handling, bin-compatibility --- .../test/mixed_cluster/10_basic.yml | 16 ++++ .../test/old_cluster/10_basic.yml | 31 +++++++ .../test/upgraded_cluster/10_basic.yml | 16 ++++ .../geogrid/GeoGridAggregationBuilder.java | 83 +++++++++---------- .../bucket/geogrid/GeoHashHandler.java | 37 +++++++++ .../bucket/geogrid/GeoHashType.java | 35 +++++++- .../bucket/geogrid/GeoHashTypeProvider.java | 36 ++++++++ .../bucket/geogrid/InternalGeoHashGrid.java | 17 +--- .../geogrid/GeoHashGridAggregatorTests.java | 16 ++-- .../geogrid/GeoHashGridParserTests.java | 4 +- 10 files changed, 218 insertions(+), 73 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java diff --git a/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/10_basic.yml b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/10_basic.yml index 0cce81c8985cd..818f5fd8eedcd 100644 --- a/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/10_basic.yml +++ b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/10_basic.yml @@ -63,3 +63,19 @@ field3: value - match: { hits.total: 1 } - match: { hits.hits.0._id: q3 } + + +--- +"Verify geo aggregations work during upgrade": + - do: + search: + index: geo_agg_index + body: + aggregations: + mygrid: + geohash_grid: + field : location + precision : 1 + - match: { hits.total: 6 } + - match: { aggregations.mygrid.buckets.0.key: u } + - match: { aggregations.mygrid.buckets.0.doc_count: 6 } diff --git a/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/10_basic.yml b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/10_basic.yml index 04d85eb607835..ea44092d5a9e3 100644 --- a/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/10_basic.yml +++ b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/10_basic.yml @@ -198,3 +198,34 @@ tasks.get: wait_for_completion: true task_id: $task + +--- +"Create geo data records in the old cluster": + - do: + indices.create: + index: geo_agg_index + body: + settings: + index: + number_of_replicas: 0 + mappings: + doc: + properties: + location: + type: geo_point + - do: + bulk: + refresh: true + body: + - '{"index": {"_index": "geo_agg_index", "_type": "doc"}}' + - '{"location": "52.374081,4.912350", "name": "NEMO Science Museum"}' + - '{"index": {"_index": "geo_agg_index", "_type": "doc"}}' + - '{"location": "52.369219,4.901618", "name": "Museum Het Rembrandthuis"}' + - '{"index": {"_index": "geo_agg_index", "_type": "doc"}}' + - '{"location": "52.371667,4.914722", "name": "Nederlands Scheepvaartmuseum"}' + - '{"index": {"_index": "geo_agg_index", "_type": "doc"}}' + - '{"location": "51.222900,4.405200", "name": "Letterenhuis"}' + - '{"index": {"_index": "geo_agg_index", "_type": "doc"}}' + - '{"location": "48.861111,2.336389", "name": "Musée du Louvre"}' + - '{"index": {"_index": "geo_agg_index", "_type": "doc"}}' + - '{"location": "48.860000,2.327000", "name": "Musée Orsay"}' diff --git a/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/10_basic.yml b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/10_basic.yml index 3e293f91ce12a..ee7c29834ff91 100644 --- a/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/10_basic.yml +++ b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/10_basic.yml @@ -117,3 +117,19 @@ wait_for_completion: true task_id: $task_id - match: { task.headers.X-Opaque-Id: "Reindexing Again" } + +--- +"Verify geo aggregations work after upgrade with new types": + - do: + search: + index: geo_agg_index + body: + aggregations: + mygrid: + geohash_grid: + type: geohash + field : location + precision : 1 + - match: { hits.total: 6 } + - match: { aggregations.mygrid.buckets.0.key: u } + - match: { aggregations.mygrid.buckets.0.doc_count: 6 } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java index fa6094543774e..2f06a5d842264 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java @@ -22,9 +22,7 @@ import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.index.SortedNumericDocValues; import org.elasticsearch.ElasticsearchException; -import org.elasticsearch.common.geo.GeoHashUtils; import org.elasticsearch.common.geo.GeoPoint; -import org.elasticsearch.common.geo.GeoUtils; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ObjectParser; @@ -57,7 +55,6 @@ public class GeoGridAggregationBuilder extends ValuesSourceAggregationBuilder implements MultiBucketAggregationBuilder { public static final String NAME = "geohash_grid"; - public static final GeoHashType DEFAULT_TYPE = GeoHashType.GEOHASH; public static final int DEFAULT_MAX_NUM_CELLS = 10000; private static final ObjectParser PARSER; @@ -65,7 +62,6 @@ public class GeoGridAggregationBuilder extends ValuesSourceAggregationBuilder(GeoGridAggregationBuilder.NAME); ValuesSourceParserHelper.declareGeoFields(PARSER, false, false); PARSER.declareString(GeoGridAggregationBuilder::type, GeoHashGridParams.FIELD_TYPE); - // ValueType.INT supports both numbers and strings PARSER.declareField(GeoGridAggregationBuilder::parsePrecision, GeoHashGridParams.FIELD_PRECISION, ObjectParser.ValueType.INT); PARSER.declareInt(GeoGridAggregationBuilder::size, GeoHashGridParams.FIELD_SIZE); PARSER.declareInt(GeoGridAggregationBuilder::shardSize, GeoHashGridParams.FIELD_SHARD_SIZE); @@ -74,20 +70,43 @@ public class GeoGridAggregationBuilder extends ValuesSourceAggregationBuilder innerBuild(SearchContext context, ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { - - // delayed precision validation and defaults - int precision; - switch (type) { - case GEOHASH: - if (this.precision != null) { - precision = GeoUtils.parsePrecisionString(this.precision); - } else { - precision = 5; - } - break; - default: - throw new IllegalArgumentException("Unknown type " + type.toString()); - } - int shardSize = this.shardSize; int requiredSize = this.requiredSize; @@ -245,7 +242,7 @@ protected boolean innerEquals(Object obj) { if (type != other.type) { return false; } - if (!Objects.equals(precision, other.precision)) { + if (precision != other.precision) { return false; } if (requiredSize != other.requiredSize) { @@ -282,16 +279,12 @@ protected CellValues(MultiGeoPointValues geoValues, GeoHashType type, int precis public boolean advanceExact(int docId) throws IOException { if (geoValues.advanceExact(docId)) { resize(geoValues.docValueCount()); + + final GeoHashTypeProvider typeHandler = type.getHandler(); + for (int i = 0; i < docValueCount(); ++i) { GeoPoint target = geoValues.nextValue(); - - switch (type) { - case GEOHASH: - values[i] = GeoHashUtils.longEncode(target.getLon(), target.getLat(), precision); - break; - default: - throw new IllegalArgumentException(); - } + values[i] = typeHandler.calculateHash(target.getLon(), target.getLat(), precision); } sort(); return true; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java new file mode 100644 index 0000000000000..9b32450ce517f --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java @@ -0,0 +1,37 @@ +package org.elasticsearch.search.aggregations.bucket.geogrid; + +import org.elasticsearch.common.geo.GeoHashUtils; +import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.common.geo.GeoUtils; + +public class GeoHashHandler implements GeoHashTypeProvider { + @Override + public int getDefaultPrecision() { + return 5; + } + + @Override + public int parsePrecisionString(String precision) { + return GeoUtils.parsePrecisionString(precision); + } + + @Override + public int validatePrecision(int precision) { + return GeoUtils.checkPrecisionRange(precision); + } + + @Override + public long calculateHash(double longitude, double latitude, int precision) { + return GeoHashUtils.longEncode(longitude, latitude, precision); + } + + @Override + public String hashAsString(long hash) { + return GeoHashUtils.stringEncode(hash); + } + + @Override + public GeoPoint hashAsObject(long hash) { + return GeoPoint.fromGeohash(hash); + } +} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java index 2f49eb7b94e88..6a3782ea7b6d1 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket.geogrid; +import org.elasticsearch.Version; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; @@ -27,7 +28,15 @@ import java.util.Locale; public enum GeoHashType implements Writeable { - GEOHASH; + GEOHASH(new GeoHashHandler()); + + public static final GeoHashType DEFAULT = GEOHASH; + + private GeoHashTypeProvider handler; + + GeoHashType(GeoHashTypeProvider handler) { + this.handler = handler; + } /** * Case-insensitive from string method. @@ -40,16 +49,36 @@ public static GeoHashType forString(String value) { } public static GeoHashType readFromStream(StreamInput in) throws IOException { - return in.readEnum(GeoHashType.class); + + // FIXME: update version after backport + + if(in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { + return in.readEnum(GeoHashType.class); + } else { + return DEFAULT; + } } @Override public void writeTo(StreamOutput out) throws IOException { - out.writeEnum(this); + + // FIXME: update version after backport + + // To maintain binary compatibility, only include type after the given version + if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { + out.writeEnum(this); + } else if (this != DEFAULT) { + throw new UnsupportedOperationException("Geo aggregation type [" + this + + "] is not supported by the node version " + out.getVersion().toString()); + } } @Override public String toString() { return name().toLowerCase(Locale.ROOT); } + + public GeoHashTypeProvider getHandler() { + return handler; + } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java new file mode 100644 index 0000000000000..f4eca26179bcd --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java @@ -0,0 +1,36 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.search.aggregations.bucket.geogrid; + +import org.elasticsearch.common.geo.GeoPoint; + +public interface GeoHashTypeProvider { + int getDefaultPrecision(); + + int parsePrecisionString(String precision); + + int validatePrecision(int precision); + + long calculateHash(double longitude, double latitude, int precision); + + String hashAsString(long hash); + + GeoPoint hashAsObject(long hash); +} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java index 30d7b56fd3b0e..2ae54f9362500 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java @@ -81,22 +81,13 @@ public void writeTo(StreamOutput out) throws IOException { @Override public String getKeyAsString() { - switch(type) { - case GEOHASH: - return GeoHashUtils.stringEncode(geohashAsLong); - default: - throw new IllegalArgumentException(); - } + return type.getHandler().hashAsString(geohashAsLong); } @Override - public Object getKey() { - switch(type) { - case GEOHASH: - return GeoPoint.fromGeohash(geohashAsLong); - default: - throw new IllegalArgumentException(); - } + public GeoPoint getKey() { + // TODO/FIXME: is it ok to change from GeoPoint to Object, and return different types? + return type.getHandler().hashAsObject(geohashAsLong); } @Override diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java index 74e3bfed1170b..141ef0dd314b8 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java @@ -40,11 +40,8 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.function.BiFunction; import java.util.function.Consumer; -import org.elasticsearch.common.geo.GeoHashUtils; - public class GeoHashGridAggregatorTests extends AggregatorTestCase { private static final String FIELD_NAME = "location"; @@ -67,12 +64,10 @@ public void testFieldMissing() throws IOException { public void testHashcodeWithSeveralDocs() throws IOException { int precision = randomIntBetween(1, 12); - testWithSeveralDocs(GeoHashType.GEOHASH, precision, (lng, lat) -> { - return GeoHashUtils.stringEncode(lng, lat, precision); - }); + testWithSeveralDocs(GeoHashType.GEOHASH, precision); } - private void testWithSeveralDocs(GeoHashType type, int precision, BiFunction hasher) + private void testWithSeveralDocs(GeoHashType type, int precision) throws IOException { int numPoints = randomIntBetween(8, 128); Map expectedCountPerGeoHash = new HashMap<>(); @@ -83,7 +78,7 @@ private void testWithSeveralDocs(GeoHashType type, int precision, BiFunction buildIndex, Consumer verify) throws IOException { + private void testCase(Query query, String field, GeoHashType type, int precision, + CheckedConsumer buildIndex, + Consumer verify) throws IOException { Directory directory = newDirectory(); RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory); buildIndex.accept(indexWriter); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java index d8e4ab1056b63..e431bf19ff3de 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java @@ -65,8 +65,8 @@ public void testParseDistanceUnitPrecision() throws Exception { // can create a factory GeoGridAggregationBuilder builder = GeoGridAggregationBuilder.parse("geohash_grid", stParser); assertNotNull(builder); - assertThat(Integer.parseInt(builder.precision()), greaterThanOrEqualTo(0)); - assertThat(Integer.parseInt(builder.precision()), lessThanOrEqualTo(12)); + assertThat(builder.precision(), greaterThanOrEqualTo(0)); + assertThat(builder.precision(), lessThanOrEqualTo(12)); } public void testParseInvalidUnitPrecision() throws Exception { From cf724e9d1c73e7aadfa75c6d31abc728484a536c Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Thu, 3 May 2018 19:35:41 +0300 Subject: [PATCH 05/18] missing license --- .../bucket/geogrid/GeoHashHandler.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java index 9b32450ce517f..7caa0fe39821e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java @@ -1,3 +1,22 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + package org.elasticsearch.search.aggregations.bucket.geogrid; import org.elasticsearch.common.geo.GeoHashUtils; From 8115690947076817827beef0711e1a338dab2ddc Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Thu, 3 May 2018 21:34:36 +0300 Subject: [PATCH 06/18] wrap precision parsing in proper exception --- .../geogrid/GeoGridAggregationBuilder.java | 42 ++++++++++++------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java index 2f06a5d842264..fbe6e52257187 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java @@ -27,6 +27,8 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentLocation; +import org.elasticsearch.common.xcontent.XContentParseException; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.fielddata.AbstractSortingNumericDocValues; import org.elasticsearch.index.fielddata.MultiGeoPointValues; @@ -72,10 +74,11 @@ private static void parsePrecision(XContentParser parser, GeoGridAggregationBuil // In some cases, this value cannot be fully parsed until after we know the type // Store precision as is until the end of parsing. // ValueType.INT could be either a number or a string + builder.precisionLocation = parser.getTokenLocation(); if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { - builder.tempPrecision = parser.intValue(); + builder.precisionRaw = parser.intValue(); } else { - builder.tempPrecision = parser.text(); + builder.precisionRaw = parser.text(); } } @@ -83,27 +86,38 @@ public static GeoGridAggregationBuilder parse(String aggregationName, XContentPa GeoGridAggregationBuilder builder = PARSER.parse(parser, new GeoGridAggregationBuilder(aggregationName), null); - // delayed precision validation - final GeoHashTypeProvider typeHandler = builder.type.getHandler(); + try { + // delayed precision validation + final GeoHashTypeProvider typeHandler = builder.type.getHandler(); - if (builder.tempPrecision == null) { - builder.precision(typeHandler.getDefaultPrecision()); - } else if (builder.tempPrecision instanceof String) { - builder.precision(typeHandler.parsePrecisionString((String) builder.tempPrecision)); - } else { - builder.precision((int) builder.tempPrecision); - } + if (builder.precisionRaw == null) { + builder.precision(typeHandler.getDefaultPrecision()); + } else if (builder.precisionRaw instanceof String) { + builder.precision(typeHandler.parsePrecisionString((String) builder.precisionRaw)); + } else { + builder.precision((int) builder.precisionRaw); + } - return builder; + return builder; + } catch (Exception e) { + throw new XContentParseException(builder.precisionLocation, + "[geohash_grid] failed to parse field [precision]", e); + } } private GeoHashType type = GeoHashType.DEFAULT; /** - * Contains temporary value during the parsing. + * Contains unparsed precision value during the parsing. * This value will be converted into an integer precision at the end of parsing. */ - private Object tempPrecision = null; + private Object precisionRaw = null; + + /** + * Stores the location of the precision parameter, in case precision is + * incorrect and an error has to be reported to the user. Only valid during parsing. + */ + private XContentLocation precisionLocation = null; private int precision = -1; From 82dd82dac7d88c0d5939fc0b098e765c6b8f4231 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Thu, 3 May 2018 23:51:56 +0300 Subject: [PATCH 07/18] test fixes, docs --- .../geogrid/GeoGridAggregationBuilder.java | 2 +- .../bucket/geogrid/GeoHashTypeProvider.java | 25 +++++++++++++++++++ .../aggregations/bucket/GeoHashGridTests.java | 18 ++++++++++++- 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java index fbe6e52257187..2b7985e0c57d0 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java @@ -119,7 +119,7 @@ public static GeoGridAggregationBuilder parse(String aggregationName, XContentPa */ private XContentLocation precisionLocation = null; - private int precision = -1; + private int precision = Integer.MIN_VALUE; private int requiredSize = DEFAULT_MAX_NUM_CELLS; private int shardSize = -1; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java index f4eca26179bcd..1d1511023e1c3 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java @@ -22,15 +22,40 @@ import org.elasticsearch.common.geo.GeoPoint; public interface GeoHashTypeProvider { + /** + * Returns default precision for the type, e.g. 5 for geohash + */ int getDefaultPrecision(); + /** + * Parses precision string into an integer, e.g. 100km -> 4 + */ int parsePrecisionString(String precision); + /** + * Validates precision for the given geo type, and throws an exception on error + * @param precision value to validate + * @return the original value if everything is ok + */ int validatePrecision(int precision); + /** + * Converts longitude/latitude into a bucket identifying hash value with the given precision + * @return hash value + */ long calculateHash(double longitude, double latitude, int precision); + /** + * Decodes hash value into a string returned to the user + * @param hash as generated by the {@link #calculateHash} + * @return bucket ID as a string + */ String hashAsString(long hash); + /** + * Converts hash value into a center point of a grid + * @param hash as generated by the {@link #calculateHash} + * @return center of the grid cell + */ GeoPoint hashAsObject(long hash); } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java index fb7b3984cdf2b..3ef32c14cea7f 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java @@ -21,6 +21,7 @@ import org.elasticsearch.search.aggregations.BaseAggregationTestCase; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoGridAggregationBuilder; +import org.elasticsearch.search.aggregations.bucket.geogrid.GeoHashType; public class GeoHashGridTests extends BaseAggregationTestCase { @@ -29,8 +30,23 @@ protected GeoGridAggregationBuilder createTestAggregatorBuilder() { String name = randomAlphaOfLengthBetween(3, 20); GeoGridAggregationBuilder factory = new GeoGridAggregationBuilder(name); if (randomBoolean()) { - int precision = randomIntBetween(1, 12); + factory.type(randomFrom(GeoHashType.values()).toString()); + } + if (randomBoolean()) { + int precision; + switch (factory.type()) { + case GEOHASH: + precision = randomIntBetween(1, 12); + break; + default: + throw new IllegalArgumentException( + "GeoHashType." + factory.type().name() + " was not added to the test"); + } factory.precision(precision); + } else { + // precision gets initialized during the parse stage, so we have to force it + // to the default value for the chosen hashing type. + factory.precision(factory.type().getHandler().getDefaultPrecision()); } if (randomBoolean()) { factory.size(randomIntBetween(1, Integer.MAX_VALUE)); From 06d87dec858cb34c3b8d95308cdb9c3e969a5242 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Fri, 4 May 2018 01:28:17 +0300 Subject: [PATCH 08/18] doc fix --- .../aggregations/bucket/geogrid/GeoHashTypeProvider.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java index 1d1511023e1c3..2509133b3c4d7 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java @@ -21,6 +21,9 @@ import org.elasticsearch.common.geo.GeoPoint; +/** + * Instances implement different hashing algorithms for geo-grid aggregations + */ public interface GeoHashTypeProvider { /** * Returns default precision for the type, e.g. 5 for geohash @@ -28,7 +31,7 @@ public interface GeoHashTypeProvider { int getDefaultPrecision(); /** - * Parses precision string into an integer, e.g. 100km -> 4 + * Parses precision string into an integer, e.g. "100km" into 4 */ int parsePrecisionString(String precision); From f5f56bae21df16063dde0412175086200cc651b5 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Fri, 4 May 2018 03:51:13 +0300 Subject: [PATCH 09/18] keep default precision updated --- .../bucket/geogrid/GeoGridAggregationBuilder.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java index 2b7985e0c57d0..06ade02336015 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java @@ -119,7 +119,7 @@ public static GeoGridAggregationBuilder parse(String aggregationName, XContentPa */ private XContentLocation precisionLocation = null; - private int precision = Integer.MIN_VALUE; + private int precision = GeoHashType.DEFAULT.getHandler().getDefaultPrecision(); private int requiredSize = DEFAULT_MAX_NUM_CELLS; private int shardSize = -1; @@ -171,6 +171,9 @@ public GeoGridAggregationBuilder type(String type) { .collect(Collectors.joining(", ")) + ". Found [" + type + "] in [" + name + "]"); } + // Some tests bypass parsing steps, so keep the default precision consistent. + // Note that tests must set precision after setting the type + this.precision = this.type.getHandler().getDefaultPrecision(); return this; } From 689dffc5c9faecd303a7d81bcd57c4e22db4b1d8 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sat, 5 May 2018 02:31:52 +0300 Subject: [PATCH 10/18] test fix --- .../bucket/geogrid/GeoHashGridAggregatorTests.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java index 141ef0dd314b8..a77e13e3ff18f 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket.geogrid; import org.apache.lucene.document.LatLonDocValuesField; +import org.apache.lucene.geo.GeoEncodingUtils; import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.IndexReader; import org.apache.lucene.index.RandomIndexWriter; @@ -77,6 +78,11 @@ private void testWithSeveralDocs(GeoHashType type, int precision) for (int pointId = 0; pointId < numPoints; pointId++) { double lat = (180d * randomDouble()) - 90d; double lng = (360d * randomDouble()) - 180d; + + // Precision-adjust longitude/latitude to avoid wrong bucket placement + lng = GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(lng)); + lat = GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(lat)); + points.add(new LatLonDocValuesField(FIELD_NAME, lat, lng)); String hash = type.getHandler().hashAsString(type.getHandler().calculateHash(lng, lat, precision)); if (distinctHashesPerDoc.contains(hash) == false) { From 8effe02513bfc53ffe86c07649d0afdc95f002a4 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sat, 5 May 2018 03:40:10 +0300 Subject: [PATCH 11/18] generalize tests --- .../aggregations/bucket/GeoHashGridTests.java | 48 +++++++++++++------ .../geogrid/GeoHashGridAggregatorTests.java | 19 ++++---- .../geogrid/GeoHashGridParserTests.java | 34 ++++++++++--- 3 files changed, 69 insertions(+), 32 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java index 3ef32c14cea7f..c02922afa01e8 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java @@ -25,28 +25,46 @@ public class GeoHashGridTests extends BaseAggregationTestCase { + /** + * Pick a random hash type + */ + public static GeoHashType randomType() { + return randomFrom(GeoHashType.values()); + } + + /** + * Pick a random precision for the given hash type. + */ + public static int randomPrecision(final GeoHashType type) { + int precision; + switch (type) { + case GEOHASH: + precision = randomIntBetween(1, 12); + break; + default: + throw new IllegalArgumentException("GeoHashType." + type.name() + " was not added to the test"); + } + return precision; + } + + public static int maxPrecision(GeoHashType type) { + switch (type) { + case GEOHASH: + return 12; + default: + throw new IllegalArgumentException("GeoHashType." + type.name() + " was not added to the test"); + } + } + @Override protected GeoGridAggregationBuilder createTestAggregatorBuilder() { String name = randomAlphaOfLengthBetween(3, 20); GeoGridAggregationBuilder factory = new GeoGridAggregationBuilder(name); if (randomBoolean()) { - factory.type(randomFrom(GeoHashType.values()).toString()); + factory.type(randomType().toString()); } if (randomBoolean()) { - int precision; - switch (factory.type()) { - case GEOHASH: - precision = randomIntBetween(1, 12); - break; - default: - throw new IllegalArgumentException( - "GeoHashType." + factory.type().name() + " was not added to the test"); - } - factory.precision(precision); - } else { - // precision gets initialized during the parse stage, so we have to force it - // to the default value for the chosen hashing type. - factory.precision(factory.type().getHandler().getDefaultPrecision()); + factory.precision(randomPrecision(factory.type())); } if (randomBoolean()) { factory.size(randomIntBetween(1, Integer.MAX_VALUE)); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java index a77e13e3ff18f..2632f12d4c7df 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java @@ -32,6 +32,7 @@ import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.search.aggregations.Aggregator; import org.elasticsearch.search.aggregations.AggregatorTestCase; +import org.elasticsearch.search.aggregations.bucket.GeoHashGridTests; import java.io.IOException; import java.util.ArrayList; @@ -48,24 +49,22 @@ public class GeoHashGridAggregatorTests extends AggregatorTestCase { private static final String FIELD_NAME = "location"; public void testNoDocs() throws IOException { - testCase(new MatchAllDocsQuery(), FIELD_NAME, GeoHashType.GEOHASH, 1, iw -> { + final GeoHashType type = GeoHashGridTests.randomType(); + testCase(new MatchAllDocsQuery(), FIELD_NAME, type, GeoHashGridTests.randomPrecision(type), iw -> { // Intentionally not writing any docs - }, geoHashGrid -> { - assertEquals(0, geoHashGrid.getBuckets().size()); - }); + }, geoHashGrid -> assertEquals(0, geoHashGrid.getBuckets().size())); } public void testFieldMissing() throws IOException { - testCase(new MatchAllDocsQuery(), "wrong_field", GeoHashType.GEOHASH, 1, iw -> { + final GeoHashType type = GeoHashGridTests.randomType(); + testCase(new MatchAllDocsQuery(), "wrong_field", type, GeoHashGridTests.randomPrecision(type), iw -> { iw.addDocument(Collections.singleton(new LatLonDocValuesField(FIELD_NAME, 10D, 10D))); - }, geoHashGrid -> { - assertEquals(0, geoHashGrid.getBuckets().size()); - }); + }, geoHashGrid -> assertEquals(0, geoHashGrid.getBuckets().size())); } public void testHashcodeWithSeveralDocs() throws IOException { - int precision = randomIntBetween(1, 12); - testWithSeveralDocs(GeoHashType.GEOHASH, precision); + final GeoHashType type = GeoHashGridTests.randomType(); + testWithSeveralDocs(type, GeoHashGridTests.randomPrecision(type)); } private void testWithSeveralDocs(GeoHashType type, int precision) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java index e431bf19ff3de..ad97de4329aa0 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java @@ -23,6 +23,7 @@ import org.elasticsearch.common.xcontent.XContentParseException; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.search.aggregations.bucket.GeoHashGridTests; import org.elasticsearch.test.ESTestCase; import static org.hamcrest.Matchers.containsString; @@ -32,9 +33,11 @@ public class GeoHashGridParserTests extends ESTestCase { public void testParseValidFromInts() throws Exception { - int precision = randomIntBetween(1, 12); + GeoHashType type = GeoHashGridTests.randomType(); + int precision = GeoHashGridTests.randomPrecision(type); XContentParser stParser = createParser(JsonXContent.jsonXContent, - "{\"field\":\"my_loc\", \"precision\":" + precision + ", \"size\": 500, \"shard_size\": 550}"); + "{\"field\":\"my_loc\", \"type\":\"" + type + "\", \"precision\":" + precision + + ", \"size\": 500, \"shard_size\": 550}"); XContentParser.Token token = stParser.nextToken(); assertSame(XContentParser.Token.START_OBJECT, token); // can create a factory @@ -42,9 +45,11 @@ public void testParseValidFromInts() throws Exception { } public void testParseValidFromStrings() throws Exception { - int precision = randomIntBetween(1, 12); + GeoHashType type = GeoHashGridTests.randomType(); + int precision = GeoHashGridTests.randomPrecision(type); XContentParser stParser = createParser(JsonXContent.jsonXContent, - "{\"field\":\"my_loc\", \"precision\":\"" + precision + "\", \"size\": \"500\", \"shard_size\": \"550\"}"); + "{\"field\":\"my_loc\", \"type\":\"" + type + "\", \"precision\":\"" + precision + + "\", \"size\": \"500\", \"shard_size\": \"550\"}"); XContentParser.Token token = stParser.nextToken(); assertSame(XContentParser.Token.START_OBJECT, token); // can create a factory @@ -94,7 +99,9 @@ public void testParseDistanceUnitPrecisionTooSmall() throws Exception { } public void testParseErrorOnBooleanPrecision() throws Exception { - XContentParser stParser = createParser(JsonXContent.jsonXContent, "{\"field\":\"my_loc\", \"precision\":false}"); + GeoHashType type = GeoHashGridTests.randomType(); + XContentParser stParser = createParser(JsonXContent.jsonXContent, + "{\"field\":\"my_loc\", \"type\":\"" + type + "\", \"precision\":false}"); XContentParser.Token token = stParser.nextToken(); assertSame(XContentParser.Token.START_OBJECT, token); XContentParseException e = expectThrows(XContentParseException.class, @@ -104,7 +111,10 @@ public void testParseErrorOnBooleanPrecision() throws Exception { } public void testParseErrorOnPrecisionOutOfRange() throws Exception { - XContentParser stParser = createParser(JsonXContent.jsonXContent, "{\"field\":\"my_loc\", \"precision\":\"13\"}"); + final GeoHashType type = GeoHashGridTests.randomType(); + final int precision = GeoHashGridTests.maxPrecision(type) + 1; + XContentParser stParser = createParser(JsonXContent.jsonXContent, + "{\"field\":\"my_loc\", \"type\":\"" + type + "\", \"precision\":\""+ precision +"\"}"); XContentParser.Token token = stParser.nextToken(); assertSame(XContentParser.Token.START_OBJECT, token); try { @@ -112,7 +122,17 @@ public void testParseErrorOnPrecisionOutOfRange() throws Exception { fail(); } catch (XContentParseException ex) { assertThat(ex.getCause(), instanceOf(IllegalArgumentException.class)); - assertEquals("Invalid geohash aggregation precision of 13. Must be between 1 and 12.", ex.getCause().getMessage()); + if (type == GeoHashType.GEOHASH) { + String expectedMsg; + switch (type) { + case GEOHASH: + expectedMsg = "Invalid geohash aggregation precision of 13. Must be between 1 and 12."; + break; + default: + throw new IllegalArgumentException("GeoHashType." + type.name() + " was not added to the test"); + } + assertEquals(expectedMsg, ex.getCause().getMessage()); + } } } } From 32a8725ebf32a47645f3abc81d5b8677e9b3b6f8 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sat, 5 May 2018 04:07:43 +0300 Subject: [PATCH 12/18] expand test --- .../bucket/geogrid/GeoHashGridParserTests.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java index ad97de4329aa0..b26635e5ff730 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java @@ -122,17 +122,15 @@ public void testParseErrorOnPrecisionOutOfRange() throws Exception { fail(); } catch (XContentParseException ex) { assertThat(ex.getCause(), instanceOf(IllegalArgumentException.class)); - if (type == GeoHashType.GEOHASH) { - String expectedMsg; - switch (type) { - case GEOHASH: - expectedMsg = "Invalid geohash aggregation precision of 13. Must be between 1 and 12."; - break; - default: - throw new IllegalArgumentException("GeoHashType." + type.name() + " was not added to the test"); - } - assertEquals(expectedMsg, ex.getCause().getMessage()); + String expectedMsg; + switch (type) { + case GEOHASH: + expectedMsg = "Invalid geohash aggregation precision of 13. Must be between 1 and 12."; + break; + default: + throw new IllegalArgumentException("GeoHashType." + type.name() + " was not added to the test"); } + assertEquals(expectedMsg, ex.getCause().getMessage()); } } } From a26a588b2ac1814e041c4faf47ae9fd64bbc0485 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Tue, 5 Jun 2018 01:36:51 -0400 Subject: [PATCH 13/18] Added GeoHashTypeTests --- .../common/geo/GeoHashTypeTests.java | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 server/src/test/java/org/elasticsearch/common/geo/GeoHashTypeTests.java diff --git a/server/src/test/java/org/elasticsearch/common/geo/GeoHashTypeTests.java b/server/src/test/java/org/elasticsearch/common/geo/GeoHashTypeTests.java new file mode 100644 index 0000000000000..e0cb0e0400dc5 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/common/geo/GeoHashTypeTests.java @@ -0,0 +1,111 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.geo; + +import org.elasticsearch.Version; +import org.elasticsearch.common.io.stream.BytesStreamOutput; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.search.aggregations.bucket.geogrid.GeoHashType; +import org.elasticsearch.test.ESTestCase; + +import java.io.IOException; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; + +public class GeoHashTypeTests extends ESTestCase { + + public void testValidOrdinals() { + assertThat(GeoHashType.DEFAULT.ordinal(), equalTo(0)); + assertThat(GeoHashType.GEOHASH.ordinal(), equalTo(0)); + } + + public void testWriteTo() throws Exception { + try (BytesStreamOutput out = new BytesStreamOutput()) { + + // FIXME: update version after backport + + final Version version = Version.V_7_0_0_alpha1; + out.setVersion(version); + GeoHashType.GEOHASH.writeTo(out); + try (StreamInput in = out.bytes().streamInput()) { + assertThat(in.readVInt(), equalTo(0)); + } + } + + try (BytesStreamOutput out = new BytesStreamOutput()) { + + // FIXME: update version after backport to the last unsupported + + out.setVersion(Version.V_6_3_0); + GeoHashType.GEOHASH.writeTo(out); + // Should not have written anything + assertThat(out.size(), equalTo(0)); + } + + // ATTENTION: new hashing types should also assert that writeTo() throws + // an error when values other than GEOHASH are written to older version streams + } + + public void testReadFrom() throws Exception { + try (BytesStreamOutput out = new BytesStreamOutput()) { + out.writeVInt(0); + try (StreamInput in = out.bytes().streamInput()) { + + // FIXME: update version after backport + + in.setVersion(Version.V_7_0_0_alpha1); + assertThat(in.available(), equalTo(1)); + assertThat(GeoHashType.readFromStream(in), equalTo(GeoHashType.GEOHASH)); + assertThat(in.available(), equalTo(0)); + } + } + + try (BytesStreamOutput out = new BytesStreamOutput()) { + // keep the stream empty to cause an exception if a read is attempted + try (StreamInput in = out.bytes().streamInput()) { + + // FIXME: update version after backport to the last unsupported + + in.setVersion(Version.V_6_3_0); + assertThat(in.available(), equalTo(0)); + assertThat(in.available(), equalTo(0)); + assertThat(GeoHashType.readFromStream(in), equalTo(GeoHashType.GEOHASH)); + } + } + } + + public void testInvalidReadFrom() throws Exception { + try (BytesStreamOutput out = new BytesStreamOutput()) { + out.writeVInt(randomIntBetween(1, Integer.MAX_VALUE)); + try (StreamInput in = out.bytes().streamInput()) { + + // FIXME: update version after backport + + in.setVersion(Version.V_7_0_0_alpha1); + GeoHashType.readFromStream(in); + fail("Expected IOException"); + } catch(IOException e) { + assertThat(e.getMessage(), containsString("Unknown GeoHashType ordinal [")); + } + + } + } +} From b54f832ca85abd23470de35ecdb96b7dc0cae81a Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Wed, 6 Jun 2018 19:19:22 -0400 Subject: [PATCH 14/18] fixed some concerns by @colings86 --- .../test/old_cluster/10_basic.yml | 12 ++ .../elasticsearch/common/geo/GeoUtils.java | 3 +- .../aggregations/AggregationBuilders.java | 3 +- .../geogrid/GeoGridAggregationBuilder.java | 128 +++++++++--------- .../bucket/geogrid/GeoHashHandler.java | 5 +- .../bucket/geogrid/GeoHashType.java | 2 +- .../bucket/geogrid/GeoHashTypeProvider.java | 2 +- .../bucket/geogrid/InternalGeoHashGrid.java | 4 +- .../aggregations/bucket/GeoHashGridTests.java | 6 +- .../geogrid/GeoHashGridAggregatorTests.java | 3 +- .../geogrid/GeoHashGridParserTests.java | 35 +++-- 11 files changed, 113 insertions(+), 90 deletions(-) diff --git a/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/10_basic.yml b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/10_basic.yml index ea44092d5a9e3..87de721860807 100644 --- a/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/10_basic.yml +++ b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/10_basic.yml @@ -229,3 +229,15 @@ - '{"location": "48.861111,2.336389", "name": "Musée du Louvre"}' - '{"index": {"_index": "geo_agg_index", "_type": "doc"}}' - '{"location": "48.860000,2.327000", "name": "Musée Orsay"}' + - do: + search: + index: geo_agg_index + body: + aggregations: + mygrid: + geohash_grid: + field : location + precision : 1 + - match: { hits.total: 6 } + - match: { aggregations.mygrid.buckets.0.key: u } + - match: { aggregations.mygrid.buckets.0.doc_count: 6 } diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index 64398be4c2e8d..6887cd35ad0b8 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -557,7 +557,8 @@ public static int parsePrecisionString(String precision) { try { // we want to treat simple integer strings as precision levels, not distances return checkPrecisionRange(Integer.parseInt(precision)); - // Do not catch IllegalArgumentException here + // checkPrecisionRange could also throw IllegalArgumentException, but let it through + // to keep errors somewhat consistent with how they were shown before this change } catch (NumberFormatException e) { // try to parse as a distance value final int parsedPrecision = GeoUtils.geoHashLevelsForPrecision(precision); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java index 26d8bb1a1bdf5..eef01cc0a0bba 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java @@ -30,6 +30,7 @@ import org.elasticsearch.search.aggregations.bucket.filter.FiltersAggregator.KeyedFilter; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoGridAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoHashGrid; +import org.elasticsearch.search.aggregations.bucket.geogrid.GeoHashType; import org.elasticsearch.search.aggregations.bucket.global.Global; import org.elasticsearch.search.aggregations.bucket.global.GlobalAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder; @@ -237,7 +238,7 @@ public static HistogramAggregationBuilder histogram(String name) { * Create a new {@link GeoHashGrid} aggregation with the given name. */ public static GeoGridAggregationBuilder geohashGrid(String name) { - return new GeoGridAggregationBuilder(name); + return new GeoGridAggregationBuilder(name, GeoHashType.DEFAULT); } /** diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java index 06ade02336015..0065f5d075ebc 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java @@ -25,9 +25,9 @@ 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.ConstructingObjectParser; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentLocation; import org.elasticsearch.common.xcontent.XContentParseException; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.fielddata.AbstractSortingNumericDocValues; @@ -54,78 +54,65 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg; + public class GeoGridAggregationBuilder extends ValuesSourceAggregationBuilder - implements MultiBucketAggregationBuilder { + implements MultiBucketAggregationBuilder { public static final String NAME = "geohash_grid"; public static final int DEFAULT_MAX_NUM_CELLS = 10000; - private static final ObjectParser PARSER; + private static final ConstructingObjectParser PARSER; + static { - PARSER = new ObjectParser<>(GeoGridAggregationBuilder.NAME); - ValuesSourceParserHelper.declareGeoFields(PARSER, false, false); - PARSER.declareString(GeoGridAggregationBuilder::type, GeoHashGridParams.FIELD_TYPE); - PARSER.declareField(GeoGridAggregationBuilder::parsePrecision, GeoHashGridParams.FIELD_PRECISION, ObjectParser.ValueType.INT); + PARSER = new ConstructingObjectParser<>(GeoGridAggregationBuilder.NAME, false, + (a, name) -> new GeoGridAggregationBuilder(name, (String) a[0])); + + PARSER.declareString(optionalConstructorArg(), GeoHashGridParams.FIELD_TYPE); + PARSER.declareField( + GeoGridAggregationBuilder::precisionRaw, + GeoGridAggregationBuilder::parsePrecision, + GeoHashGridParams.FIELD_PRECISION, + ObjectParser.ValueType.VALUE); PARSER.declareInt(GeoGridAggregationBuilder::size, GeoHashGridParams.FIELD_SIZE); PARSER.declareInt(GeoGridAggregationBuilder::shardSize, GeoHashGridParams.FIELD_SHARD_SIZE); + + ValuesSourceParserHelper.declareGeoFields(PARSER, false, false); } - private static void parsePrecision(XContentParser parser, GeoGridAggregationBuilder builder, Void context) - throws IOException { + private static Object parsePrecision(XContentParser parser, String name) + throws IOException { + // Delay actual parsing until builder.precision() // In some cases, this value cannot be fully parsed until after we know the type - // Store precision as is until the end of parsing. - // ValueType.INT could be either a number or a string - builder.precisionLocation = parser.getTokenLocation(); - if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { - builder.precisionRaw = parser.intValue(); - } else { - builder.precisionRaw = parser.text(); + final XContentParser.Token token = parser.currentToken(); + switch (token) { + case VALUE_NUMBER: + return parser.intValue(); + case VALUE_STRING: + return parser.text(); + default: + throw new XContentParseException(parser.getTokenLocation(), + "[geohash_grid] failed to parse field [precision] in [" + name + + "]. It must be either an integer or a string"); } } - public static GeoGridAggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { - GeoGridAggregationBuilder builder = PARSER.parse(parser, - new GeoGridAggregationBuilder(aggregationName), null); - - try { - // delayed precision validation - final GeoHashTypeProvider typeHandler = builder.type.getHandler(); - - if (builder.precisionRaw == null) { - builder.precision(typeHandler.getDefaultPrecision()); - } else if (builder.precisionRaw instanceof String) { - builder.precision(typeHandler.parsePrecisionString((String) builder.precisionRaw)); - } else { - builder.precision((int) builder.precisionRaw); - } - - return builder; - } catch (Exception e) { - throw new XContentParseException(builder.precisionLocation, - "[geohash_grid] failed to parse field [precision]", e); - } + public static GeoGridAggregationBuilder parse(String aggregationName, XContentParser parser) { + return PARSER.apply(parser, aggregationName); } - private GeoHashType type = GeoHashType.DEFAULT; - - /** - * Contains unparsed precision value during the parsing. - * This value will be converted into an integer precision at the end of parsing. - */ - private Object precisionRaw = null; - - /** - * Stores the location of the precision parameter, in case precision is - * incorrect and an error has to be reported to the user. Only valid during parsing. - */ - private XContentLocation precisionLocation = null; - - private int precision = GeoHashType.DEFAULT.getHandler().getDefaultPrecision(); - + private final GeoHashType type; + private int precision; private int requiredSize = DEFAULT_MAX_NUM_CELLS; private int shardSize = -1; - public GeoGridAggregationBuilder(String name) { + public GeoGridAggregationBuilder(String name, String type) { + this(name, parseType(type, name)); + } + + public GeoGridAggregationBuilder(String name, GeoHashType type) { super(name, ValuesSourceType.GEOPOINT, ValueType.GEOPOINT); + this.type = type; + this.precision = type.getHandler().getDefaultPrecision(); } protected GeoGridAggregationBuilder(GeoGridAggregationBuilder clone, Builder factoriesBuilder, Map metaData) { @@ -160,9 +147,9 @@ protected void innerWriteTo(StreamOutput out) throws IOException { out.writeVInt(shardSize); } - public GeoGridAggregationBuilder type(String type) { + private static GeoHashType parseType(String type, String name) { try { - this.type = GeoHashType.forString(type); + return type == null ? GeoHashType.DEFAULT : GeoHashType.forString(type); } catch (IllegalArgumentException e) { throw new IllegalArgumentException( "[type] is not valid. Allowed values: " + @@ -171,16 +158,25 @@ public GeoGridAggregationBuilder type(String type) { .collect(Collectors.joining(", ")) + ". Found [" + type + "] in [" + name + "]"); } - // Some tests bypass parsing steps, so keep the default precision consistent. - // Note that tests must set precision after setting the type - this.precision = this.type.getHandler().getDefaultPrecision(); - return this; } public GeoHashType type() { return type; } + private GeoGridAggregationBuilder precisionRaw(Object precision) { + final GeoHashTypeProvider typeHandler = this.type.getHandler(); + + if (precision == null) { + this.precision(typeHandler.getDefaultPrecision()); + } else if (precision instanceof String) { + this.precision(typeHandler.parsePrecisionString((String) precision)); + } else { + this.precision((int) precision); + } + return this; + } + public GeoGridAggregationBuilder precision(int precision) { this.precision = type.getHandler().validatePrecision(precision); return this; @@ -193,7 +189,7 @@ public int precision() { public GeoGridAggregationBuilder size(int size) { if (size <= 0) { throw new IllegalArgumentException( - "[size] must be greater than 0. Found [" + size + "] in [" + name + "]"); + "[size] must be greater than 0. Found [" + size + "] in [" + name + "]"); } this.requiredSize = size; return this; @@ -206,7 +202,7 @@ public int size() { public GeoGridAggregationBuilder shardSize(int shardSize) { if (shardSize <= 0) { throw new IllegalArgumentException( - "[shardSize] must be greater than 0. Found [" + shardSize + "] in [" + name + "]"); + "[shardSize] must be greater than 0. Found [" + shardSize + "] in [" + name + "]"); } this.shardSize = shardSize; return this; @@ -218,8 +214,8 @@ public int shardSize() { @Override protected ValuesSourceAggregatorFactory innerBuild(SearchContext context, - ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) - throws IOException { + ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) + throws IOException { int shardSize = this.shardSize; int requiredSize = this.requiredSize; @@ -232,14 +228,14 @@ public int shardSize() { if (requiredSize <= 0 || shardSize <= 0) { throw new ElasticsearchException( - "parameters [required_size] and [shard_size] must be >0 in geohash_grid aggregation [" + name + "]."); + "parameters [required_size] and [shard_size] must be >0 in geohash_grid aggregation [" + name + "]."); } if (shardSize < requiredSize) { shardSize = requiredSize; } return new GeoHashGridAggregatorFactory(name, config, type, precision, requiredSize, shardSize, context, parent, - subFactoriesBuilder, metaData); + subFactoriesBuilder, metaData); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java index 7caa0fe39821e..36c5b01f633e7 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java @@ -23,6 +23,9 @@ import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoUtils; +/** + * A simple wrapper for GeoUtils handling of the geohash hashing algorithm + */ public class GeoHashHandler implements GeoHashTypeProvider { @Override public int getDefaultPrecision() { @@ -50,7 +53,7 @@ public String hashAsString(long hash) { } @Override - public GeoPoint hashAsObject(long hash) { + public GeoPoint hashAsGeoPoint(long hash) { return GeoPoint.fromGeohash(hash); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java index 6a3782ea7b6d1..ddc3bdd3ca7ea 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java @@ -68,7 +68,7 @@ public void writeTo(StreamOutput out) throws IOException { if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) { out.writeEnum(this); } else if (this != DEFAULT) { - throw new UnsupportedOperationException("Geo aggregation type [" + this + + throw new UnsupportedOperationException("Geo aggregation type [" + toString() + "] is not supported by the node version " + out.getVersion().toString()); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java index 2509133b3c4d7..36964e50c5f19 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java @@ -60,5 +60,5 @@ public interface GeoHashTypeProvider { * @param hash as generated by the {@link #calculateHash} * @return center of the grid cell */ - GeoPoint hashAsObject(long hash); + GeoPoint hashAsGeoPoint(long hash); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java index 2ae54f9362500..4217521b70ff5 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java @@ -19,7 +19,6 @@ package org.elasticsearch.search.aggregations.bucket.geogrid; import org.apache.lucene.util.PriorityQueue; -import org.elasticsearch.common.geo.GeoHashUtils; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -86,8 +85,7 @@ public String getKeyAsString() { @Override public GeoPoint getKey() { - // TODO/FIXME: is it ok to change from GeoPoint to Object, and return different types? - return type.getHandler().hashAsObject(geohashAsLong); + return type.getHandler().hashAsGeoPoint(geohashAsLong); } @Override diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java index c02922afa01e8..5f344a8b7fb25 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java @@ -59,10 +59,8 @@ public static int maxPrecision(GeoHashType type) { @Override protected GeoGridAggregationBuilder createTestAggregatorBuilder() { String name = randomAlphaOfLengthBetween(3, 20); - GeoGridAggregationBuilder factory = new GeoGridAggregationBuilder(name); - if (randomBoolean()) { - factory.type(randomType().toString()); - } + String type = randomBoolean() ? randomType().toString() : null; + GeoGridAggregationBuilder factory = new GeoGridAggregationBuilder(name, type); if (randomBoolean()) { factory.precision(randomPrecision(factory.type())); } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java index 2632f12d4c7df..33808b2102a16 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java @@ -116,8 +116,7 @@ private void testCase(Query query, String field, GeoHashType type, int precision IndexReader indexReader = DirectoryReader.open(directory); IndexSearcher indexSearcher = newSearcher(indexReader, true, true); - GeoGridAggregationBuilder aggregationBuilder = new GeoGridAggregationBuilder("_name").field(field); - aggregationBuilder.type(type.name()); + GeoGridAggregationBuilder aggregationBuilder = new GeoGridAggregationBuilder("_name", type).field(field); aggregationBuilder.precision(precision); MappedFieldType fieldType = new GeoPointFieldMapper.GeoPointFieldType(); fieldType.setHasDocValues(true); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java index b26635e5ff730..7e1942481a9b4 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java @@ -18,7 +18,6 @@ */ package org.elasticsearch.search.aggregations.bucket.geogrid; -import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.xcontent.XContentParseException; import org.elasticsearch.common.xcontent.XContentParser; @@ -81,9 +80,15 @@ public void testParseInvalidUnitPrecision() throws Exception { assertSame(XContentParser.Token.START_OBJECT, token); XContentParseException ex = expectThrows(XContentParseException.class, () -> GeoGridAggregationBuilder.parse("geohash_grid", stParser)); - assertThat(ex.getMessage(), containsString("[geohash_grid] failed to parse field [precision]")); - assertThat(ex.getCause(), instanceOf(NumberFormatException.class)); - assertEquals("For input string: \"10kg\"", ex.getCause().getMessage()); + assertThat(ex.getMessage(), containsString("failed to build [geohash_grid] after last required field arrived")); + + Throwable cause = ex.getCause(); + assertThat(cause, instanceOf(XContentParseException.class)); + assertThat(cause.getMessage(), containsString("[geohash_grid] failed to parse field [precision]")); + + cause = cause.getCause(); + assertThat(cause, instanceOf(NumberFormatException.class)); + assertThat(cause.getMessage(), containsString("For input string: \"10kg\"")); } public void testParseDistanceUnitPrecisionTooSmall() throws Exception { @@ -93,9 +98,15 @@ public void testParseDistanceUnitPrecisionTooSmall() throws Exception { assertSame(XContentParser.Token.START_OBJECT, token); XContentParseException ex = expectThrows(XContentParseException.class, () -> GeoGridAggregationBuilder.parse("geohash_grid", stParser)); - assertThat(ex.getMessage(), containsString("[geohash_grid] failed to parse field [precision]")); - assertThat(ex.getCause(), instanceOf(IllegalArgumentException.class)); - assertEquals("precision too high [1cm]", ex.getCause().getMessage()); + assertThat(ex.getMessage(), containsString("failed to build [geohash_grid] after last required field arrived")); + + Throwable cause = ex.getCause(); + assertThat(cause, instanceOf(XContentParseException.class)); + assertThat(cause.getMessage(), containsString("[geohash_grid] failed to parse field [precision]")); + + cause = cause.getCause(); + assertThat(cause, instanceOf(IllegalArgumentException.class)); + assertEquals("precision too high [1cm]", cause.getMessage()); } public void testParseErrorOnBooleanPrecision() throws Exception { @@ -104,10 +115,14 @@ public void testParseErrorOnBooleanPrecision() throws Exception { "{\"field\":\"my_loc\", \"type\":\"" + type + "\", \"precision\":false}"); XContentParser.Token token = stParser.nextToken(); assertSame(XContentParser.Token.START_OBJECT, token); - XContentParseException e = expectThrows(XContentParseException.class, + XContentParseException ex = expectThrows(XContentParseException.class, () -> GeoGridAggregationBuilder.parse("geohash_grid", stParser)); - assertThat(ExceptionsHelper.detailedMessage(e), - containsString("[geohash_grid] precision doesn't support values of type: VALUE_BOOLEAN")); + assertThat(ex.getMessage(), containsString("[geohash_grid] failed to parse field [precision]")); + + Throwable cause = ex.getCause(); + assertThat(cause, instanceOf(XContentParseException.class)); + assertThat(cause.getMessage(), containsString("[geohash_grid] failed to parse field [precision]" + + " in [geohash_grid]. It must be either an integer or a string")); } public void testParseErrorOnPrecisionOutOfRange() throws Exception { From 9b3668d1edafedd248bd8e8e1bc83fe2710b492d Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Thu, 7 Jun 2018 10:30:15 -0400 Subject: [PATCH 15/18] fix style formatting --- .../bucket/geogrid/GeoGridAggregationBuilder.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java index 0065f5d075ebc..ebefc7feda331 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java @@ -214,8 +214,8 @@ public int shardSize() { @Override protected ValuesSourceAggregatorFactory innerBuild(SearchContext context, - ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) - throws IOException { + ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) + throws IOException { int shardSize = this.shardSize; int requiredSize = this.requiredSize; From 040f889cd54c26317d811fb14d6a72af11585474 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Thu, 7 Jun 2018 11:25:13 -0400 Subject: [PATCH 16/18] follow up fixes with sync up --- .../geogrid/GeoGridAggregationBuilder.java | 25 ++++++++++++------- .../bucket/geogrid/InternalGeoHashGrid.java | 6 +++++ 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java index ebefc7feda331..3f98378ff8eec 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java @@ -65,9 +65,12 @@ public class GeoGridAggregationBuilder extends ValuesSourceAggregationBuilder(GeoGridAggregationBuilder.NAME, false, - (a, name) -> new GeoGridAggregationBuilder(name, (String) a[0])); + (a, name) -> new GeoGridAggregationBuilder(name, (GeoHashType) a[0])); - PARSER.declareString(optionalConstructorArg(), GeoHashGridParams.FIELD_TYPE); + PARSER.declareField(optionalConstructorArg(), + GeoGridAggregationBuilder::parseType, + GeoHashGridParams.FIELD_TYPE, + ObjectParser.ValueType.STRING); PARSER.declareField( GeoGridAggregationBuilder::precisionRaw, GeoGridAggregationBuilder::parsePrecision, @@ -105,13 +108,9 @@ public static GeoGridAggregationBuilder parse(String aggregationName, XContentPa private int requiredSize = DEFAULT_MAX_NUM_CELLS; private int shardSize = -1; - public GeoGridAggregationBuilder(String name, String type) { - this(name, parseType(type, name)); - } - public GeoGridAggregationBuilder(String name, GeoHashType type) { super(name, ValuesSourceType.GEOPOINT, ValueType.GEOPOINT); - this.type = type; + this.type = type == null ? GeoHashType.DEFAULT : type; this.precision = type.getHandler().getDefaultPrecision(); } @@ -133,6 +132,10 @@ protected AggregationBuilder shallowCopy(Builder factoriesBuilder, Map Date: Thu, 7 Jun 2018 12:11:24 -0400 Subject: [PATCH 17/18] renamed type to hash_type --- .../search/aggregations/bucket/geogrid/GeoHashGridParams.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParams.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParams.java index b16487e4236ab..736f66b5931e9 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParams.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParams.java @@ -26,7 +26,7 @@ final class GeoHashGridParams { /* recognized field names in JSON */ - static final ParseField FIELD_TYPE = new ParseField("type"); + static final ParseField FIELD_TYPE = new ParseField("hash_type"); static final ParseField FIELD_PRECISION = new ParseField("precision"); static final ParseField FIELD_SIZE = new ParseField("size"); static final ParseField FIELD_SHARD_SIZE = new ParseField("shard_size"); From d95c6a0319449456c7fff55deb4c58b2c157223b Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Thu, 7 Jun 2018 12:24:58 -0400 Subject: [PATCH 18/18] fixed hash_type and a possible nullref --- .../rest-api-spec/test/upgraded_cluster/10_basic.yml | 2 +- .../bucket/geogrid/GeoGridAggregationBuilder.java | 2 +- .../search/aggregations/bucket/GeoHashGridTests.java | 2 +- .../bucket/geogrid/GeoHashGridParserTests.java | 8 ++++---- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/10_basic.yml b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/10_basic.yml index ee7c29834ff91..dce4594804fb0 100644 --- a/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/10_basic.yml +++ b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/10_basic.yml @@ -127,7 +127,7 @@ aggregations: mygrid: geohash_grid: - type: geohash + hash_type: geohash field : location precision : 1 - match: { hits.total: 6 } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java index 3f98378ff8eec..23123542b0530 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java @@ -111,7 +111,7 @@ public static GeoGridAggregationBuilder parse(String aggregationName, XContentPa public GeoGridAggregationBuilder(String name, GeoHashType type) { super(name, ValuesSourceType.GEOPOINT, ValueType.GEOPOINT); this.type = type == null ? GeoHashType.DEFAULT : type; - this.precision = type.getHandler().getDefaultPrecision(); + this.precision = this.type.getHandler().getDefaultPrecision(); } protected GeoGridAggregationBuilder(GeoGridAggregationBuilder clone, Builder factoriesBuilder, Map metaData) { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java index 5f344a8b7fb25..5dd83170c40d4 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java @@ -59,7 +59,7 @@ public static int maxPrecision(GeoHashType type) { @Override protected GeoGridAggregationBuilder createTestAggregatorBuilder() { String name = randomAlphaOfLengthBetween(3, 20); - String type = randomBoolean() ? randomType().toString() : null; + GeoHashType type = randomBoolean() ? randomType() : null; GeoGridAggregationBuilder factory = new GeoGridAggregationBuilder(name, type); if (randomBoolean()) { factory.precision(randomPrecision(factory.type())); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java index 7e1942481a9b4..c77aff717bca7 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java @@ -35,7 +35,7 @@ public void testParseValidFromInts() throws Exception { GeoHashType type = GeoHashGridTests.randomType(); int precision = GeoHashGridTests.randomPrecision(type); XContentParser stParser = createParser(JsonXContent.jsonXContent, - "{\"field\":\"my_loc\", \"type\":\"" + type + "\", \"precision\":" + precision + + "{\"field\":\"my_loc\", \"hash_type\":\"" + type + "\", \"precision\":" + precision + ", \"size\": 500, \"shard_size\": 550}"); XContentParser.Token token = stParser.nextToken(); assertSame(XContentParser.Token.START_OBJECT, token); @@ -47,7 +47,7 @@ public void testParseValidFromStrings() throws Exception { GeoHashType type = GeoHashGridTests.randomType(); int precision = GeoHashGridTests.randomPrecision(type); XContentParser stParser = createParser(JsonXContent.jsonXContent, - "{\"field\":\"my_loc\", \"type\":\"" + type + "\", \"precision\":\"" + precision + + "{\"field\":\"my_loc\", \"hash_type\":\"" + type + "\", \"precision\":\"" + precision + "\", \"size\": \"500\", \"shard_size\": \"550\"}"); XContentParser.Token token = stParser.nextToken(); assertSame(XContentParser.Token.START_OBJECT, token); @@ -112,7 +112,7 @@ public void testParseDistanceUnitPrecisionTooSmall() throws Exception { public void testParseErrorOnBooleanPrecision() throws Exception { GeoHashType type = GeoHashGridTests.randomType(); XContentParser stParser = createParser(JsonXContent.jsonXContent, - "{\"field\":\"my_loc\", \"type\":\"" + type + "\", \"precision\":false}"); + "{\"field\":\"my_loc\", \"hash_type\":\"" + type + "\", \"precision\":false}"); XContentParser.Token token = stParser.nextToken(); assertSame(XContentParser.Token.START_OBJECT, token); XContentParseException ex = expectThrows(XContentParseException.class, @@ -129,7 +129,7 @@ public void testParseErrorOnPrecisionOutOfRange() throws Exception { final GeoHashType type = GeoHashGridTests.randomType(); final int precision = GeoHashGridTests.maxPrecision(type) + 1; XContentParser stParser = createParser(JsonXContent.jsonXContent, - "{\"field\":\"my_loc\", \"type\":\"" + type + "\", \"precision\":\""+ precision +"\"}"); + "{\"field\":\"my_loc\", \"hash_type\":\"" + type + "\", \"precision\":\""+ precision +"\"}"); XContentParser.Token token = stParser.nextToken(); assertSame(XContentParser.Token.START_OBJECT, token); try {