From 2c0754403c07aaf7dee34a0bc839afdb575a0955 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Wed, 6 Jun 2018 19:19:22 -0400 Subject: [PATCH] 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 {