From 137df274ab313b678495408e789b9c0913450d1e Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 11 May 2020 19:48:59 -0400 Subject: [PATCH] Add support for numeric range keys (#56452) (#56552) This adds support for parsing numbers as range keys. They get converted into a string, but we allow numbers. While I was there I replaced the parser for `Range` with a `ConstructingObjectParser` which will automatically add support for "did you mean" style corrections on errors. Closes #56402 --- .../range/DateRangeAggregationBuilder.java | 8 +- .../bucket/range/RangeAggregationBuilder.java | 7 +- .../bucket/range/RangeAggregator.java | 104 ++++++++------- .../range/RangeAggregationBuilderTests.java | 119 ++++++++++++++++++ 4 files changed, 172 insertions(+), 66 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilderTests.java diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java index 476ad5cda21..a1ce8a1c44e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java @@ -21,7 +21,6 @@ package org.elasticsearch.search.aggregations.bucket.range; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.xcontent.ObjectParser; -import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.AggregationBuilder; @@ -50,12 +49,7 @@ public class DateRangeAggregationBuilder extends AbstractRangeBuilder DateRangeAggregationBuilder.parseRange(p), RangeAggregator.RANGES_FIELD); - } - - - private static RangeAggregator.Range parseRange(XContentParser parser) throws IOException { - return RangeAggregator.Range.fromXContent(parser); + }, (p, c) -> RangeAggregator.Range.PARSER.parse(p, null), RangeAggregator.RANGES_FIELD); } public static void registerAggregators(ValuesSourceRegistry.Builder builder) { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java index c9bfb9dc000..e891525c071 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java @@ -21,7 +21,6 @@ package org.elasticsearch.search.aggregations.bucket.range; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.xcontent.ObjectParser; -import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.AggregationBuilder; @@ -48,11 +47,7 @@ public class RangeAggregationBuilder extends AbstractRangeBuilder RangeAggregationBuilder.parseRange(p), RangeAggregator.RANGES_FIELD); - } - - private static Range parseRange(XContentParser parser) throws IOException { - return Range.fromXContent(parser); + }, (p, c) -> RangeAggregator.Range.PARSER.parse(p, null), RangeAggregator.RANGES_FIELD); } public static void registerAggregators(ValuesSourceRegistry.Builder builder) { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java index 86b75f54556..58ce08fd80d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java @@ -24,10 +24,12 @@ import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.ContextParser; +import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.common.xcontent.XContentParserUtils; import org.elasticsearch.index.fielddata.SortedNumericDoubleValues; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.Aggregator; @@ -47,6 +49,8 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg; + public class RangeAggregator extends BucketsAggregator { public static final ParseField RANGES_FIELD = new ParseField("ranges"); @@ -63,6 +67,23 @@ public class RangeAggregator extends BucketsAggregator { protected final double to; protected final String toAsStr; + /** + * Build the range. Generally callers should prefer + * {@link Range#Range(String, Double, Double)} or + * {@link Range#Range(String, String, String)}. If you + * must call this know that consumers prefer + * {@code from} and {@code to} parameters if they are non-null + * and finite. Otherwise they parse from {@code fromrStr} and + * {@code toStr}. + */ + public Range(String key, Double from, String fromAsStr, Double to, String toAsStr) { + this.key = key; + this.from = from == null ? Double.NEGATIVE_INFINITY : from; + this.fromAsStr = fromAsStr; + this.to = to == null ? Double.POSITIVE_INFINITY : to; + this.toAsStr = toAsStr; + } + public Range(String key, Double from, Double to) { this(key, from, null, to, null); } @@ -111,14 +132,6 @@ public class RangeAggregator extends BucketsAggregator { return this.key; } - public Range(String key, Double from, String fromAsStr, Double to, String toAsStr) { - this.key = key; - this.from = from == null ? Double.NEGATIVE_INFINITY : from; - this.fromAsStr = fromAsStr; - this.to = to == null ? Double.POSITIVE_INFINITY : to; - this.toAsStr = toAsStr; - } - boolean matches(double value) { return value >= from && value < to; } @@ -128,50 +141,6 @@ public class RangeAggregator extends BucketsAggregator { return "[" + from + " to " + to + ")"; } - public static Range fromXContent(XContentParser parser) throws IOException { - XContentParser.Token token; - String currentFieldName = null; - double from = Double.NEGATIVE_INFINITY; - String fromAsStr = null; - double to = Double.POSITIVE_INFINITY; - String toAsStr = null; - String key = null; - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentFieldName = parser.currentName(); - } else if (token == XContentParser.Token.VALUE_NUMBER) { - if (FROM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - from = parser.doubleValue(); - } else if (TO_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - to = parser.doubleValue(); - } else { - XContentParserUtils.throwUnknownField(currentFieldName, parser.getTokenLocation()); - } - } else if (token == XContentParser.Token.VALUE_STRING) { - if (FROM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - fromAsStr = parser.text(); - } else if (TO_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - toAsStr = parser.text(); - } else if (KEY_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - key = parser.text(); - } else { - XContentParserUtils.throwUnknownField(currentFieldName, parser.getTokenLocation()); - } - } else if (token == XContentParser.Token.VALUE_NULL) { - if (FROM_FIELD.match(currentFieldName, parser.getDeprecationHandler()) - || TO_FIELD.match(currentFieldName, parser.getDeprecationHandler()) - || KEY_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - // ignore null value - } else { - XContentParserUtils.throwUnknownField(currentFieldName, parser.getTokenLocation()); - } - } else { - XContentParserUtils.throwUnknownToken(token, parser.getTokenLocation()); - } - } - return new Range(key, from, fromAsStr, to, toAsStr); - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); @@ -194,6 +163,35 @@ public class RangeAggregator extends BucketsAggregator { return builder; } + public static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>("range", arg -> { + String key = (String) arg[0]; + Object from = arg[1]; + Object to = arg[2]; + Double fromDouble = from instanceof Number ? ((Number) from).doubleValue() : null; + Double toDouble = to instanceof Number ? ((Number) to).doubleValue() : null; + String fromStr = from instanceof String ? (String) from : null; + String toStr = to instanceof String ? (String) to : null; + return new Range(key, fromDouble, fromStr, toDouble, toStr); + }); + + static { + PARSER.declareField(optionalConstructorArg(), + (p, c) -> p.text(), + KEY_FIELD, ValueType.DOUBLE); // DOUBLE supports string and number + ContextParser fromToParser = (p, c) -> { + if (p.currentToken() == XContentParser.Token.VALUE_STRING) { + return p.text(); + } + if (p.currentToken() == XContentParser.Token.VALUE_NUMBER) { + return p.doubleValue(); + } + return null; + }; + // DOUBLE_OR_NULL accepts String, Number, and null + PARSER.declareField(optionalConstructorArg(), fromToParser, FROM_FIELD, ValueType.DOUBLE_OR_NULL); + PARSER.declareField(optionalConstructorArg(), fromToParser, TO_FIELD, ValueType.DOUBLE_OR_NULL); + } + @Override public int hashCode() { return Objects.hash(key, from, fromAsStr, to, toAsStr); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilderTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilderTests.java new file mode 100644 index 00000000000..9fcf3b00cbb --- /dev/null +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilderTests.java @@ -0,0 +1,119 @@ +/* + * 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.range; + +import org.elasticsearch.common.io.stream.Writeable.Reader; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.test.AbstractSerializingTestCase; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; + +import static org.hamcrest.Matchers.equalTo; + +public class RangeAggregationBuilderTests extends AbstractSerializingTestCase { + @Override + protected RangeAggregationBuilder doParseInstance(XContentParser parser) throws IOException { + assertThat(parser.nextToken(), equalTo(XContentParser.Token.START_OBJECT)); + assertThat(parser.nextToken(), equalTo(XContentParser.Token.FIELD_NAME)); + String name = parser.currentName(); + assertThat(parser.nextToken(), equalTo(XContentParser.Token.START_OBJECT)); + assertThat(parser.nextToken(), equalTo(XContentParser.Token.FIELD_NAME)); + assertThat(parser.currentName(), equalTo("range")); + RangeAggregationBuilder parsed = RangeAggregationBuilder.PARSER.apply(parser, name); + assertThat(parser.nextToken(), equalTo(XContentParser.Token.END_OBJECT)); + assertThat(parser.nextToken(), equalTo(XContentParser.Token.END_OBJECT)); + return parsed; + } + + @Override + protected Reader instanceReader() { + return RangeAggregationBuilder::new; + } + + @Override + protected RangeAggregationBuilder createTestInstance() { + RangeAggregationBuilder builder = new RangeAggregationBuilder(randomAlphaOfLength(5)); + builder.keyed(randomBoolean()); + builder.field(randomAlphaOfLength(4)); + int rangeCount = between(1, 10); + double r = 0; + for (int i = 0; i < rangeCount; i++) { + switch (between(0, 2)) { + case 0: + builder.addUnboundedFrom(randomAlphaOfLength(2), r); + break; + case 1: + builder.addUnboundedTo(randomAlphaOfLength(2), r); + break; + case 2: + double from = r; + r += randomDouble(); // less than 1 + double to = r; + builder.addRange(randomAlphaOfLength(2), from, to); + break; + default: + fail(); + } + r += randomDouble(); // less than 1 + } + return builder; + } + + @Override + protected RangeAggregationBuilder mutateInstance(RangeAggregationBuilder builder) throws IOException { + String name = builder.getName(); + boolean keyed = builder.keyed(); + String field = builder.field(); + List ranges = builder.ranges(); + switch (between(0, 3)) { + case 0: + name += randomAlphaOfLength(1); + break; + case 1: + keyed = !keyed; + break; + case 2: + field += randomAlphaOfLength(1); + break; + case 3: + ranges = new ArrayList<>(ranges); + double from = ranges.get(ranges.size() - 1).from; + double to = from + randomDouble(); + ranges.add(new RangeAggregator.Range(randomAlphaOfLength(2), from, to)); + break; + default: + fail(); + } + RangeAggregationBuilder mutant = new RangeAggregationBuilder(name).keyed(keyed).field(field); + ranges.stream().forEach(mutant::addRange); + return mutant; + } + + public void testNumericKeys() throws IOException { + RangeAggregationBuilder builder = doParseInstance(createParser(JsonXContent.jsonXContent, + "{\"test\":{\"range\":{\"field\":\"f\",\"ranges\":[{\"key\":1,\"to\":0}]}}}")); + assertThat(builder.getName(), equalTo("test")); + assertThat(builder.field(), equalTo("f")); + assertThat(builder.ranges, equalTo(org.elasticsearch.common.collect.List.of(new RangeAggregator.Range("1", null, 0d)))); + } +}