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..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 @@ -198,3 +198,46 @@ 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"}' + - 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/upgraded_cluster/10_basic.yml b/qa/rolling-upgrade/src/test/resources/rest-api-spec/test/upgraded_cluster/10_basic.yml index 3e293f91ce12a..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 @@ -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: + hash_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/common/geo/GeoUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index 2f3443639cdb7..6887cd35ad0b8 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,26 @@ 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)); + // 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); 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/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 2f66531834d38..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 @@ -22,21 +22,19 @@ 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.ConstructingObjectParser; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParseException; 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,39 +51,72 @@ import java.io.IOException; import java.util.Map; import java.util.Objects; +import java.util.stream.Collectors; +import java.util.stream.Stream; -import static org.elasticsearch.common.geo.GeoUtils.parsePrecision; +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_PRECISION = 5; 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.declareField((parser, builder, context) -> builder.precision(parsePrecision(parser)), GeoHashGridParams.FIELD_PRECISION, - org.elasticsearch.common.xcontent.ObjectParser.ValueType.INT); + PARSER = new ConstructingObjectParser<>(GeoGridAggregationBuilder.NAME, false, + (a, name) -> new GeoGridAggregationBuilder(name, (GeoHashType) a[0])); + + PARSER.declareField(optionalConstructorArg(), + GeoGridAggregationBuilder::parseType, + GeoHashGridParams.FIELD_TYPE, + ObjectParser.ValueType.STRING); + 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); } - public static GeoGridAggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { - return PARSER.parse(parser, new GeoGridAggregationBuilder(aggregationName), null); + 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 + 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) { + return PARSER.apply(parser, aggregationName); } - private int precision = DEFAULT_PRECISION; + 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, GeoHashType type) { super(name, ValuesSourceType.GEOPOINT, ValueType.GEOPOINT); + this.type = type == null ? GeoHashType.DEFAULT : type; + this.precision = this.type.getHandler().getDefaultPrecision(); } 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,6 +132,11 @@ protected AggregationBuilder shallowCopy(Builder factoriesBuilder, Map0 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, precision, requiredSize, shardSize, context, parent, - subFactoriesBuilder, metaData); + 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,6 +259,9 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params) @Override protected boolean innerEquals(Object obj) { GeoGridAggregationBuilder other = (GeoGridAggregationBuilder) obj; + if (type != other.type) { + return false; + } if (precision != other.precision) { return false; } @@ -201,7 +276,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 +286,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; } @@ -222,10 +299,12 @@ protected CellValues(MultiGeoPointValues geoValues, int precision) { 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(); - values[i] = GeoHashUtils.longEncode(target.getLon(), target.getLat(), - precision); + values[i] = typeHandler.calculateHash(target.getLon(), target.getLat(), precision); } sort(); return true; @@ -237,14 +316,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 +341,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..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,6 +26,7 @@ final class GeoHashGridParams { /* recognized field names in JSON */ + 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"); 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..36c5b01f633e7 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java @@ -0,0 +1,59 @@ +/* + * 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; +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() { + 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 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 new file mode 100644 index 0000000000000..ddc3bdd3ca7ea --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java @@ -0,0 +1,84 @@ +/* + * 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.Version; +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(new GeoHashHandler()); + + public static final GeoHashType DEFAULT = GEOHASH; + + private GeoHashTypeProvider handler; + + GeoHashType(GeoHashTypeProvider handler) { + this.handler = handler; + } + + /** + * 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 { + + // 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 { + + // 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 [" + toString() + + "] 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..36964e50c5f19 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java @@ -0,0 +1,64 @@ +/* + * 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; + +/** + * Instances implement different hashing algorithms for geo-grid aggregations + */ +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" into 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 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 bc60f5945eb9f..80ff4c696e2fa 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; @@ -49,11 +48,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 +64,10 @@ static class Bucket extends InternalMultiBucketAggregation.InternalBucket implem * Read from a stream. */ private Bucket(StreamInput in) throws IOException { + // type will not be deserialized for the older stream versions + // We do not do backward compatibility here because it is needed + // in multiple places, and could get out of sync + type = GeoHashType.readFromStream(in); geohashAsLong = in.readLong(); docCount = in.readVLong(); aggregations = InternalAggregations.readAggregations(in); @@ -70,6 +75,10 @@ private Bucket(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { + // type will not be serialized for the older stream versions + // We do not do backward compatibility here because it is needed + // in multiple places, and could get out of sync + type.writeTo(out); out.writeLong(geohashAsLong); out.writeVLong(docCount); aggregations.writeTo(out); @@ -77,12 +86,12 @@ public void writeTo(StreamOutput out) throws IOException { @Override public String getKeyAsString() { - return GeoHashUtils.stringEncode(geohashAsLong); + return type.getHandler().hashAsString(geohashAsLong); } @Override public GeoPoint getKey() { - return GeoPoint.fromGeohash(geohashAsLong); + return type.getHandler().hashAsGeoPoint(geohashAsLong); } @Override @@ -114,7 +123,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 +142,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 +191,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/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 [")); + } + + } + } +} 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..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 @@ -21,16 +21,48 @@ 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 { + /** + * 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); + GeoHashType type = randomBoolean() ? randomType() : null; + GeoGridAggregationBuilder factory = new GeoGridAggregationBuilder(name, type); if (randomBoolean()) { - int precision = randomIntBetween(1, 12); - factory.precision(precision); + 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 45b6b64cddc35..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 @@ -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; @@ -31,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; @@ -42,40 +44,46 @@ import java.util.Set; import java.util.function.Consumer; -import static org.elasticsearch.common.geo.GeoHashUtils.stringEncode; - public class GeoHashGridAggregatorTests extends AggregatorTestCase { private static final String FIELD_NAME = "location"; public void testNoDocs() throws IOException { - testCase(new MatchAllDocsQuery(), FIELD_NAME, 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", 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 testWithSeveralDocs() throws IOException { - int precision = randomIntBetween(1, 12); + public void testHashcodeWithSeveralDocs() throws IOException { + final GeoHashType type = GeoHashGridTests.randomType(); + testWithSeveralDocs(type, GeoHashGridTests.randomPrecision(type)); + } + + private void testWithSeveralDocs(GeoHashType type, int precision) + 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; + + // 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 = stringEncode(lng, lat, precision); + String hash = type.getHandler().hashAsString(type.getHandler().calculateHash(lng, lat, precision)); if (distinctHashesPerDoc.contains(hash) == false) { expectedCountPerGeoHash.put(hash, expectedCountPerGeoHash.getOrDefault(hash, 0) + 1); } @@ -97,7 +105,8 @@ public void testWithSeveralDocs() throws IOException { }); } - private void testCase(Query query, String field, int precision, CheckedConsumer buildIndex, + 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); @@ -107,7 +116,7 @@ private void testCase(Query query, String field, int precision, CheckedConsumer< IndexReader indexReader = DirectoryReader.open(directory); IndexSearcher indexSearcher = newSearcher(indexReader, true, true); - GeoGridAggregationBuilder aggregationBuilder = new GeoGridAggregationBuilder("_name").field(field); + 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 e431bf19ff3de..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 @@ -18,11 +18,11 @@ */ 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; 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 +32,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\", \"hash_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 +44,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\", \"hash_type\":\"" + type + "\", \"precision\":\"" + precision + + "\", \"size\": \"500\", \"shard_size\": \"550\"}"); XContentParser.Token token = stParser.nextToken(); assertSame(XContentParser.Token.START_OBJECT, token); // can create a factory @@ -76,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 { @@ -88,23 +98,38 @@ 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 { - XContentParser stParser = createParser(JsonXContent.jsonXContent, "{\"field\":\"my_loc\", \"precision\":false}"); + GeoHashType type = GeoHashGridTests.randomType(); + XContentParser stParser = createParser(JsonXContent.jsonXContent, + "{\"field\":\"my_loc\", \"hash_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 { - 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\", \"hash_type\":\"" + type + "\", \"precision\":\""+ precision +"\"}"); XContentParser.Token token = stParser.nextToken(); assertSame(XContentParser.Token.START_OBJECT, token); try { @@ -112,7 +137,15 @@ 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()); + 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()); } } } 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);