From a06db772a82b1beab9bcbd90251fd3afb4f2e0c0 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 16 May 2017 12:03:54 +0200 Subject: [PATCH 1/3] Add parsing to Binary Range aggregation --- .../bucket/range/InternalBinaryRange.java | 1 + .../bucket/range/ParsedBinaryRange.java | 170 ++++++++++++++++++ .../aggregations/AggregationsTests.java | 2 + .../range/InternalBinaryRangeTests.java | 65 +++++-- .../bucket/range/InternalRangeTestCase.java | 17 +- .../bucket/range/InternalRangeTests.java | 11 ++ .../range/date/InternalDateRangeTests.java | 11 ++ .../geodistance/InternalGeoDistanceTests.java | 11 ++ .../test/InternalAggregationTestCase.java | 6 +- 9 files changed, 270 insertions(+), 24 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java index 640b1cfb467f4..60fb5b99fb9fb 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java @@ -44,6 +44,7 @@ public final class InternalBinaryRange extends InternalMultiBucketAggregation implements Range { + public static class Bucket extends InternalMultiBucketAggregation.InternalBucket implements Range.Bucket { private final transient DocValueFormat format; diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java new file mode 100644 index 0000000000000..7a1c6decd9720 --- /dev/null +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java @@ -0,0 +1,170 @@ +/* + * 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.apache.lucene.util.BytesRef; +import org.elasticsearch.common.xcontent.ObjectParser; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentParserUtils; +import org.elasticsearch.search.aggregations.Aggregation; +import org.elasticsearch.search.aggregations.Aggregations; +import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation; +import org.elasticsearch.search.aggregations.bucket.range.ip.IpRangeAggregationBuilder; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; + +import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken; + +public class ParsedBinaryRange extends ParsedMultiBucketAggregation implements Range { + + @Override + public String getType() { + return IpRangeAggregationBuilder.NAME; + } + + @Override + public List getBuckets() { + return buckets; + } + + private static ObjectParser PARSER = + new ObjectParser<>(ParsedBinaryRange.class.getSimpleName(), true, ParsedBinaryRange::new); + static { + declareMultiBucketAggregationFields(PARSER, + parser -> ParsedBucket.fromXContent(parser, false), + parser -> ParsedBucket.fromXContent(parser, true)); + } + + public static ParsedBinaryRange fromXContent(XContentParser parser, String name) throws IOException { + ParsedBinaryRange aggregation = PARSER.parse(parser, null); + aggregation.setName(name); + return aggregation; + } + + public static class ParsedBucket extends ParsedMultiBucketAggregation.ParsedBucket implements Range.Bucket { + + private String key; + private BytesRef from; + private BytesRef to; + + @Override + public Object getKey() { + return key; + } + + @Override + public String getKeyAsString() { + return key; + } + + @Override + public Object getFrom() { + return getFromAsString(); + } + + @Override + public String getFromAsString() { + return bytesRefToString(from); + } + + @Override + public Object getTo() { + return getToAsString(); + } + + @Override + public String getToAsString() { + return bytesRefToString(to); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + String key = this.key; + if (isKeyed()) { + if (key == null) { + StringBuilder keyBuilder = new StringBuilder(); + keyBuilder.append(from == null ? "*" : getFromAsString()); + keyBuilder.append('-'); + keyBuilder.append(to == null ? "*" : getToAsString()); + key = keyBuilder.toString(); + } + builder.startObject(key); + } else { + builder.startObject(); + if (key != null) { + builder.field(CommonFields.KEY.getPreferredName(), key); + } + } + if (from != null) { + builder.field(CommonFields.FROM.getPreferredName(), getFrom()); + } + if (to != null) { + builder.field(CommonFields.TO.getPreferredName(), getTo()); + } + builder.field(CommonFields.DOC_COUNT.getPreferredName(), getDocCount()); + getAggregations().toXContentInternal(builder, params); + builder.endObject(); + return builder; + } + + private static String bytesRefToString(final BytesRef bytesRef) { + if (bytesRef == null) { + return null; + } + return bytesRef.utf8ToString(); + } + + static ParsedBucket fromXContent(final XContentParser parser, final boolean keyed) throws IOException { + final ParsedBucket bucket = new ParsedBucket(); + bucket.setKeyed(keyed); + XContentParser.Token token = parser.currentToken(); + String currentFieldName = parser.currentName(); + if (keyed) { + ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation); + bucket.key = currentFieldName; + ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation); + } + + List aggregations = new ArrayList<>(); + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + if (token == XContentParser.Token.FIELD_NAME) { + currentFieldName = parser.currentName(); + } else if (token.isValue()) { + if (CommonFields.KEY.getPreferredName().equals(currentFieldName)) { + bucket.key = parser.text(); + } else if (CommonFields.DOC_COUNT.getPreferredName().equals(currentFieldName)) { + bucket.setDocCount(parser.longValue()); + } else if (CommonFields.FROM.getPreferredName().equals(currentFieldName)) { + bucket.from = parser.utf8BytesOrNull(); + } else if (CommonFields.TO.getPreferredName().equals(currentFieldName)) { + bucket.to = parser.utf8BytesOrNull(); + } + } else if (token == XContentParser.Token.START_OBJECT) { + aggregations.add(XContentParserUtils.parseTypedKeysObject(parser, Aggregation.TYPED_KEYS_DELIMITER, Aggregation.class)); + } + } + bucket.setAggregations(new Aggregations(aggregations)); + return bucket; + } + } +} diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/AggregationsTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/AggregationsTests.java index 1206088538656..7a2ae44733669 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/AggregationsTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/AggregationsTests.java @@ -36,6 +36,7 @@ import org.elasticsearch.search.aggregations.bucket.missing.InternalMissingTests; import org.elasticsearch.search.aggregations.bucket.nested.InternalNestedTests; import org.elasticsearch.search.aggregations.bucket.nested.InternalReverseNestedTests; +import org.elasticsearch.search.aggregations.bucket.range.InternalBinaryRangeTests; import org.elasticsearch.search.aggregations.bucket.range.InternalRangeTests; import org.elasticsearch.search.aggregations.bucket.range.date.InternalDateRangeTests; import org.elasticsearch.search.aggregations.bucket.range.geodistance.InternalGeoDistanceTests; @@ -132,6 +133,7 @@ private static List getAggsTests() { aggsTests.add(new SignificantLongTermsTests()); aggsTests.add(new SignificantStringTermsTests()); aggsTests.add(new InternalScriptedMetricTests()); + aggsTests.add(new InternalBinaryRangeTests()); return Collections.unmodifiableList(aggsTests); } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java index 6d83b74c828bb..5d90a895dc9a6 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java @@ -23,47 +23,59 @@ import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.InternalAggregations; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; +import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; -import org.elasticsearch.test.InternalAggregationTestCase; -import org.junit.Before; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.List; import java.util.Map; -public class InternalBinaryRangeTests extends InternalAggregationTestCase { - private Tuple[] RANGES; +public class InternalBinaryRangeTests extends InternalRangeTestCase { + + private List> ranges; + + @Override + public void setUp() throws Exception { + super.setUp(); + + final int numRanges = randomIntBetween(1, 10); + ranges = new ArrayList<>(numRanges); - @Before - public void randomSortedRanges() { - int numRanges = randomIntBetween(1, 10); - Tuple[] ranges = new Tuple[numRanges]; for (int i = 0; i < numRanges; i++) { BytesRef[] values = new BytesRef[2]; values[0] = new BytesRef(randomAlphaOfLength(15)); values[1] = new BytesRef(randomAlphaOfLength(15)); Arrays.sort(values); - ranges[i] = new Tuple(values[0], values[1]); + ranges.add(Tuple.tuple(values[0], values[1])); } - Arrays.sort(ranges, (t1, t2) -> t1.v1().compareTo(t2.v1())); - RANGES = ranges; - } + if (randomBoolean()) { + ranges.add(Tuple.tuple(null, new BytesRef(randomAlphaOfLength(15)))); + } + if (randomBoolean()) { + ranges.add(Tuple.tuple(new BytesRef(randomAlphaOfLength(15)), null)); + } + if (randomBoolean()) { + ranges.add(Tuple.tuple(null, null)); + } + } @Override - protected InternalBinaryRange createTestInstance(String name, List pipelineAggregators, - Map metaData) { - boolean keyed = randomBoolean(); + protected InternalBinaryRange createTestInstance(String name, + List pipelineAggregators, + Map metaData, + InternalAggregations aggregations, + boolean keyed) { DocValueFormat format = DocValueFormat.RAW; List buckets = new ArrayList<>(); - for (int i = 0; i < RANGES.length; ++i) { + for (int i = 0; i < ranges.size(); ++i) { final int docCount = randomIntBetween(1, 100); buckets.add(new InternalBinaryRange.Bucket(format, keyed, randomAlphaOfLength(10), - RANGES[i].v1(), RANGES[i].v2(), docCount, InternalAggregations.EMPTY)); + ranges.get(i).v1(), ranges.get(i).v2(), docCount, aggregations)); } - return new InternalBinaryRange(name, format, keyed, buckets, pipelineAggregators, Collections.emptyMap()); + return new InternalBinaryRange(name, format, keyed, buckets, pipelineAggregators, metaData); } @Override @@ -71,6 +83,11 @@ protected Writeable.Reader instanceReader() { return InternalBinaryRange::new; } + @Override + protected Class implementationClass() { + return ParsedBinaryRange.class; + } + @Override protected void assertReduced(InternalBinaryRange reduced, List inputs) { int pos = 0; @@ -86,4 +103,14 @@ protected void assertReduced(InternalBinaryRange reduced, List internalRangeBucketClass() { + return InternalBinaryRange.Bucket.class; + } + + @Override + protected Class parsedRangeBucketClass() { + return ParsedBinaryRange.ParsedBucket.class; + } } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalRangeTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalRangeTestCase.java index 1901f4800e0df..38cdb003178d3 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalRangeTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalRangeTestCase.java @@ -21,7 +21,9 @@ import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.InternalAggregations; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; import org.elasticsearch.search.aggregations.InternalMultiBucketAggregationTestCase; +import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation; import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.junit.Before; @@ -73,11 +75,16 @@ protected void assertReduced(T reduced, List inputs) { } @Override - protected void assertBucket(MultiBucketsAggregation.Bucket expected, MultiBucketsAggregation.Bucket actual, boolean checkOrder) { + protected final void assertBucket(MultiBucketsAggregation.Bucket expected, MultiBucketsAggregation.Bucket actual, boolean checkOrder) { super.assertBucket(expected, actual, checkOrder); - assertTrue(expected instanceof InternalRange.Bucket); - assertTrue(actual instanceof ParsedRange.ParsedBucket); + Class internalBucketClass = internalRangeBucketClass(); + assertNotNull("Internal bucket class must not be null", internalBucketClass); + assertTrue(internalBucketClass.isInstance(expected)); + + Class parsedBucketClass = parsedRangeBucketClass(); + assertNotNull("Parsed bucket class must not be null", parsedBucketClass); + assertTrue(parsedBucketClass.isInstance(actual)); Range.Bucket expectedRange = (Range.Bucket) expected; Range.Bucket actualRange = (Range.Bucket) actual; @@ -87,4 +94,8 @@ protected void assertBucket(MultiBucketsAggregation.Bucket expected, MultiBucket assertEquals(expectedRange.getTo(), actualRange.getTo()); assertEquals(expectedRange.getToAsString(), actualRange.getToAsString()); } + + protected abstract Class internalRangeBucketClass(); + + protected abstract Class parsedRangeBucketClass(); } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalRangeTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalRangeTests.java index a235471852608..5427d6c5d2999 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalRangeTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalRangeTests.java @@ -23,6 +23,7 @@ import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.InternalAggregations; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.junit.Before; @@ -97,4 +98,14 @@ protected Writeable.Reader instanceReader() { protected Class implementationClass() { return ParsedRange.class; } + + @Override + protected Class internalRangeBucketClass() { + return InternalRange.Bucket.class; + } + + @Override + protected Class parsedRangeBucketClass() { + return ParsedRange.ParsedBucket.class; + } } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/date/InternalDateRangeTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/date/InternalDateRangeTests.java index 318e5f6b5ad6f..f8cb8b3a1d841 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/date/InternalDateRangeTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/date/InternalDateRangeTests.java @@ -23,6 +23,7 @@ import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.InternalAggregations; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation; import org.elasticsearch.search.aggregations.bucket.range.InternalRangeTestCase; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; @@ -101,4 +102,14 @@ protected Writeable.Reader instanceReader() { protected Class implementationClass() { return ParsedDateRange.class; } + + @Override + protected Class internalRangeBucketClass() { + return InternalDateRange.Bucket.class; + } + + @Override + protected Class parsedRangeBucketClass() { + return ParsedDateRange.ParsedBucket.class; + } } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/InternalGeoDistanceTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/InternalGeoDistanceTests.java index 25c0a9ae3e103..e726530f79fb5 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/InternalGeoDistanceTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/InternalGeoDistanceTests.java @@ -22,6 +22,7 @@ import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.search.aggregations.InternalAggregations; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation; import org.elasticsearch.search.aggregations.bucket.range.InternalRangeTestCase; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; @@ -86,4 +87,14 @@ protected InternalGeoDistance createTestInstance(String name, protected Class implementationClass() { return ParsedGeoDistance.class; } + + @Override + protected Class internalRangeBucketClass() { + return InternalGeoDistance.Bucket.class; + } + + @Override + protected Class parsedRangeBucketClass() { + return ParsedGeoDistance.ParsedBucket.class; + } } diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java index da3b96b559dd7..835f7292a32ce 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java @@ -58,12 +58,14 @@ import org.elasticsearch.search.aggregations.bucket.nested.ParsedNested; import org.elasticsearch.search.aggregations.bucket.nested.ParsedReverseNested; import org.elasticsearch.search.aggregations.bucket.nested.ReverseNestedAggregationBuilder; +import org.elasticsearch.search.aggregations.bucket.range.ParsedBinaryRange; import org.elasticsearch.search.aggregations.bucket.range.ParsedRange; import org.elasticsearch.search.aggregations.bucket.range.RangeAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.range.date.DateRangeAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.range.date.ParsedDateRange; import org.elasticsearch.search.aggregations.bucket.range.geodistance.GeoDistanceAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.range.geodistance.ParsedGeoDistance; +import org.elasticsearch.search.aggregations.bucket.range.ip.IpRangeAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.sampler.InternalSampler; import org.elasticsearch.search.aggregations.bucket.sampler.ParsedSampler; import org.elasticsearch.search.aggregations.bucket.significant.ParsedSignificantLongTerms; @@ -161,8 +163,7 @@ public abstract class InternalAggregationTestCase map.put(StatsAggregationBuilder.NAME, (p, c) -> ParsedStats.fromXContent(p, (String) c)); map.put(StatsBucketPipelineAggregationBuilder.NAME, (p, c) -> ParsedStatsBucket.fromXContent(p, (String) c)); map.put(ExtendedStatsAggregationBuilder.NAME, (p, c) -> ParsedExtendedStats.fromXContent(p, (String) c)); - map.put(ExtendedStatsBucketPipelineAggregationBuilder.NAME, - (p, c) -> ParsedExtendedStatsBucket.fromXContent(p, (String) c)); + map.put(ExtendedStatsBucketPipelineAggregationBuilder.NAME, (p, c) -> ParsedExtendedStatsBucket.fromXContent(p, (String) c)); map.put(GeoBoundsAggregationBuilder.NAME, (p, c) -> ParsedGeoBounds.fromXContent(p, (String) c)); map.put(GeoCentroidAggregationBuilder.NAME, (p, c) -> ParsedGeoCentroid.fromXContent(p, (String) c)); map.put(HistogramAggregationBuilder.NAME, (p, c) -> ParsedHistogram.fromXContent(p, (String) c)); @@ -185,6 +186,7 @@ public abstract class InternalAggregationTestCase map.put(SignificantLongTerms.NAME, (p, c) -> ParsedSignificantLongTerms.fromXContent(p, (String) c)); map.put(SignificantStringTerms.NAME, (p, c) -> ParsedSignificantStringTerms.fromXContent(p, (String) c)); map.put(ScriptedMetricAggregationBuilder.NAME, (p, c) -> ParsedScriptedMetric.fromXContent(p, (String) c)); + map.put(IpRangeAggregationBuilder.NAME, (p, c) -> ParsedBinaryRange.fromXContent(p, (String) c)); namedXContents = map.entrySet().stream() .map(entry -> new NamedXContentRegistry.Entry(Aggregation.class, new ParseField(entry.getKey()), entry.getValue())) From b6b54d319f30ab573ee059ab9b11614c0fdfa7f1 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 16 May 2017 17:02:24 +0200 Subject: [PATCH 2/3] Handle null keys --- .../bucket/range/ParsedBinaryRange.java | 50 +++++++++---------- ...nternalMultiBucketAggregationTestCase.java | 5 +- .../range/InternalBinaryRangeTests.java | 4 +- 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java index 7a1c6decd9720..760bd23c092e7 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java @@ -19,7 +19,6 @@ package org.elasticsearch.search.aggregations.bucket.range; -import org.apache.lucene.util.BytesRef; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; @@ -64,8 +63,8 @@ public static ParsedBinaryRange fromXContent(XContentParser parser, String name) public static class ParsedBucket extends ParsedMultiBucketAggregation.ParsedBucket implements Range.Bucket { private String key; - private BytesRef from; - private BytesRef to; + private String from; + private String to; @Override public Object getKey() { @@ -79,36 +78,28 @@ public String getKeyAsString() { @Override public Object getFrom() { - return getFromAsString(); + return from; } @Override public String getFromAsString() { - return bytesRefToString(from); + return from; } @Override public Object getTo() { - return getToAsString(); + return to; } @Override public String getToAsString() { - return bytesRefToString(to); + return to; } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - String key = this.key; if (isKeyed()) { - if (key == null) { - StringBuilder keyBuilder = new StringBuilder(); - keyBuilder.append(from == null ? "*" : getFromAsString()); - keyBuilder.append('-'); - keyBuilder.append(to == null ? "*" : getToAsString()); - key = keyBuilder.toString(); - } - builder.startObject(key); + builder.startObject(key != null ? key : rangeKey(from, to)); } else { builder.startObject(); if (key != null) { @@ -127,21 +118,16 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder; } - private static String bytesRefToString(final BytesRef bytesRef) { - if (bytesRef == null) { - return null; - } - return bytesRef.utf8ToString(); - } - static ParsedBucket fromXContent(final XContentParser parser, final boolean keyed) throws IOException { final ParsedBucket bucket = new ParsedBucket(); bucket.setKeyed(keyed); XContentParser.Token token = parser.currentToken(); String currentFieldName = parser.currentName(); + + String rangeKey = null; if (keyed) { ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation); - bucket.key = currentFieldName; + rangeKey = currentFieldName; ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation); } @@ -155,16 +141,28 @@ static ParsedBucket fromXContent(final XContentParser parser, final boolean keye } else if (CommonFields.DOC_COUNT.getPreferredName().equals(currentFieldName)) { bucket.setDocCount(parser.longValue()); } else if (CommonFields.FROM.getPreferredName().equals(currentFieldName)) { - bucket.from = parser.utf8BytesOrNull(); + bucket.from = parser.text(); } else if (CommonFields.TO.getPreferredName().equals(currentFieldName)) { - bucket.to = parser.utf8BytesOrNull(); + bucket.to = parser.text(); } } else if (token == XContentParser.Token.START_OBJECT) { aggregations.add(XContentParserUtils.parseTypedKeysObject(parser, Aggregation.TYPED_KEYS_DELIMITER, Aggregation.class)); } } bucket.setAggregations(new Aggregations(aggregations)); + + if (keyed) { + if (rangeKey(bucket.from, bucket.to).equals(rangeKey)) { + bucket.key = null; + } else { + bucket.key = rangeKey; + } + } return bucket; } + + private static String rangeKey(String from, String to) { + return (from == null ? "*" : from) + '-' + (to == null ? "*" : to); + } } } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregationTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregationTestCase.java index 38e8624eeb9db..a4e61d56ca210 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregationTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregationTestCase.java @@ -98,9 +98,12 @@ private void assertMultiBucketsAggregations(Aggregation expected, Aggregation ac } } else { for (MultiBucketsAggregation.Bucket expectedBucket : expectedBuckets) { + final Object expectedKey = expectedBucket.getKey(); boolean found = false; + for (MultiBucketsAggregation.Bucket actualBucket : actualBuckets) { - if (actualBucket.getKey().equals(expectedBucket.getKey())) { + final Object actualKey = actualBucket.getKey(); + if ((actualKey != null && actualKey.equals(expectedKey)) || (actualKey == null && expectedKey == null)) { found = true; assertBucket(expectedBucket, actualBucket, false); break; diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java index 5d90a895dc9a6..d63300e12b39b 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java @@ -72,8 +72,8 @@ protected InternalBinaryRange createTestInstance(String name, List buckets = new ArrayList<>(); for (int i = 0; i < ranges.size(); ++i) { final int docCount = randomIntBetween(1, 100); - buckets.add(new InternalBinaryRange.Bucket(format, keyed, randomAlphaOfLength(10), - ranges.get(i).v1(), ranges.get(i).v2(), docCount, aggregations)); + final String key = i > 0 ? randomAlphaOfLength(10) : null; + buckets.add(new InternalBinaryRange.Bucket(format, keyed, key, ranges.get(i).v1(), ranges.get(i).v2(), docCount, aggregations)); } return new InternalBinaryRange(name, format, keyed, buckets, pipelineAggregators, metaData); } From 1ee7eb19ea69027bca22eaf8ca1c11300d8aa32c Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 16 May 2017 17:09:30 +0200 Subject: [PATCH 3/3] boo --- .../aggregations/bucket/range/InternalBinaryRangeTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java index d63300e12b39b..f88300a45028f 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java @@ -70,9 +70,11 @@ protected InternalBinaryRange createTestInstance(String name, boolean keyed) { DocValueFormat format = DocValueFormat.RAW; List buckets = new ArrayList<>(); + + int nullKey = randomBoolean() ? randomIntBetween(0, ranges.size() -1) : -1; for (int i = 0; i < ranges.size(); ++i) { final int docCount = randomIntBetween(1, 100); - final String key = i > 0 ? randomAlphaOfLength(10) : null; + final String key = (i == nullKey) ? null: randomAlphaOfLength(10); buckets.add(new InternalBinaryRange.Bucket(format, keyed, key, ranges.get(i).v1(), ranges.get(i).v2(), docCount, aggregations)); } return new InternalBinaryRange(name, format, keyed, buckets, pipelineAggregators, metaData);