From 9b2957104a23d5264cdeb5da7788098da8dd059f Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Mon, 15 May 2017 15:06:26 +0200 Subject: [PATCH 1/3] Add parsing to Significant Terms aggregations --- .../ParsedSignificantLongTerms.java | 90 ++++++++++ .../ParsedSignificantStringTerms.java | 90 ++++++++++ .../significant/ParsedSignificantTerms.java | 164 ++++++++++++++++++ .../aggregations/AggregationsTests.java | 4 + .../InternalSignificantTermsTestCase.java | 24 ++- .../SignificantLongTermsTests.java | 17 +- .../SignificantStringTermsTests.java | 14 +- .../test/InternalAggregationTestCase.java | 6 + 8 files changed, 398 insertions(+), 11 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantLongTerms.java create mode 100644 core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantStringTerms.java create mode 100644 core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantTerms.java diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantLongTerms.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantLongTerms.java new file mode 100644 index 0000000000000..e657270ed403b --- /dev/null +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantLongTerms.java @@ -0,0 +1,90 @@ +/* + * 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.significant; + +import org.elasticsearch.common.xcontent.ObjectParser; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParser; + +import java.io.IOException; + +public class ParsedSignificantLongTerms extends ParsedSignificantTerms { + + @Override + public String getType() { + return SignificantLongTerms.NAME; + } + + private static ObjectParser PARSER = + new ObjectParser<>(ParsedSignificantLongTerms.class.getSimpleName(), true, ParsedSignificantLongTerms::new); + static { + declareParsedSignificantTermsFields(PARSER, ParsedBucket::fromXContent); + } + + public static ParsedSignificantLongTerms fromXContent(XContentParser parser, String name) throws IOException { + ParsedSignificantLongTerms aggregation = PARSER.parse(parser, null); + aggregation.setName(name); + return aggregation; + } + + public static class ParsedBucket extends ParsedSignificantTerms.ParsedBucket { + + private Long key; + + @Override + public Object getKey() { + return key; + } + + @Override + public String getKeyAsString() { + String keyAsString = super.getKeyAsString(); + if (keyAsString != null) { + return keyAsString; + } + if (key != null) { + return Long.toString(key); + } + return null; + } + + public Number getKeyAsNumber() { + return key; + } + + @Override + public int compareTerm(SignificantTerms.Bucket other) { + return key.compareTo(((ParsedBucket) other).key); + } + + @Override + protected XContentBuilder keyToXContent(XContentBuilder builder) throws IOException { + builder.field(CommonFields.KEY.getPreferredName(), key); + if (super.getKeyAsString() != null) { + builder.field(CommonFields.KEY_AS_STRING.getPreferredName(), getKeyAsString()); + } + return builder; + } + + static ParsedBucket fromXContent(XContentParser parser) throws IOException { + return parseSignificantTermsBucketXContent(parser, new ParsedBucket(), (p, bucket) -> bucket.key = p.longValue()); + } + } +} diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantStringTerms.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantStringTerms.java new file mode 100644 index 0000000000000..f3a2d6734ed95 --- /dev/null +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantStringTerms.java @@ -0,0 +1,90 @@ +/* + * 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.significant; + +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 java.io.IOException; + +public class ParsedSignificantStringTerms extends ParsedSignificantTerms { + + @Override + public String getType() { + return SignificantStringTerms.NAME; + } + + private static ObjectParser PARSER = + new ObjectParser<>(ParsedSignificantStringTerms.class.getSimpleName(), true, ParsedSignificantStringTerms::new); + static { + declareParsedSignificantTermsFields(PARSER, ParsedBucket::fromXContent); + } + + public static ParsedSignificantStringTerms fromXContent(XContentParser parser, String name) throws IOException { + ParsedSignificantStringTerms aggregation = PARSER.parse(parser, null); + aggregation.setName(name); + return aggregation; + } + + public static class ParsedBucket extends ParsedSignificantTerms.ParsedBucket { + + private BytesRef key; + + @Override + public Object getKey() { + return getKeyAsString(); + } + + @Override + public String getKeyAsString() { + String keyAsString = super.getKeyAsString(); + if (keyAsString != null) { + return keyAsString; + } + if (key != null) { + return key.utf8ToString(); + } + return null; + } + + public Number getKeyAsNumber() { + if (key != null) { + return Double.parseDouble(key.utf8ToString()); + } + return null; + } + + @Override + public int compareTerm(SignificantTerms.Bucket other) { + return key.compareTo(((ParsedBucket) other).key); + } + + @Override + protected XContentBuilder keyToXContent(XContentBuilder builder) throws IOException { + return builder.field(CommonFields.KEY.getPreferredName(), getKey()); + } + + static ParsedBucket fromXContent(XContentParser parser) throws IOException { + return parseSignificantTermsBucketXContent(parser, new ParsedBucket(), (p, bucket) -> bucket.key = p.utf8BytesOrNull()); + } + } +} diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantTerms.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantTerms.java new file mode 100644 index 0000000000000..04466fe30687a --- /dev/null +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantTerms.java @@ -0,0 +1,164 @@ +/* + * 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.significant; + +import org.elasticsearch.common.CheckedBiConsumer; +import org.elasticsearch.common.CheckedFunction; +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 java.io.IOException; +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.stream.Collectors; + +public abstract class ParsedSignificantTerms extends ParsedMultiBucketAggregation + implements SignificantTerms { + + private static final String SCORE = "score"; + private static final String BG_COUNT = "bg_count"; + + protected long subsetSize; + + @Override + public List getBuckets() { + return buckets; + } + + @Override + public SignificantTerms.Bucket getBucketByKey(String term) { + for (SignificantTerms.Bucket bucket : getBuckets()) { + if (bucket.getKeyAsString().equals(term)) { + return bucket; + } + } + return null; + } + + @Override + public Iterator iterator() { + return buckets.stream().map(bucket -> (SignificantTerms.Bucket) bucket).collect(Collectors.toList()).iterator(); + } + + @Override + protected XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { + builder.field(CommonFields.DOC_COUNT.getPreferredName(), subsetSize); + builder.startArray(CommonFields.BUCKETS.getPreferredName()); + for (SignificantTerms.Bucket bucket : buckets) { + bucket.toXContent(builder, params); + } + builder.endArray(); + return builder; + } + + static void declareParsedSignificantTermsFields(final ObjectParser objectParser, + final CheckedFunction bucketParser) { + declareMultiBucketAggregationFields(objectParser, bucketParser::apply, bucketParser::apply); + objectParser.declareLong((parsedTerms, value) -> parsedTerms.subsetSize = value , CommonFields.DOC_COUNT); + } + + public abstract static class ParsedBucket extends ParsedMultiBucketAggregation.ParsedBucket implements SignificantTerms.Bucket { + + protected long subsetDf; + protected long supersetDf; + protected double score; + + @Override + public long getDocCount() { + return getSubsetDf(); + } + + @Override + public long getSubsetDf() { + return subsetDf; + } + + @Override + public long getSupersetDf() { + return supersetDf; + } + + @Override + public double getSignificanceScore() { + return score; + } + + @Override + public long getSupersetSize() { + throw new UnsupportedOperationException(); + } + + @Override + public long getSubsetSize() { + throw new UnsupportedOperationException(); + } + + @Override + public final XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(); + keyToXContent(builder); + builder.field(CommonFields.DOC_COUNT.getPreferredName(), getDocCount()); + builder.field(SCORE, getSignificanceScore()); + builder.field(BG_COUNT, getSupersetDf()); + getAggregations().toXContentInternal(builder, params); + builder.endObject(); + return builder; + } + + protected abstract XContentBuilder keyToXContent(XContentBuilder builder) throws IOException; + + static B parseSignificantTermsBucketXContent(final XContentParser parser, final B bucket, + final CheckedBiConsumer keyConsumer) throws IOException { + + final List aggregations = new ArrayList<>(); + XContentParser.Token token; + String currentFieldName = parser.currentName(); + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + if (token == XContentParser.Token.FIELD_NAME) { + currentFieldName = parser.currentName(); + } else if (token.isValue()) { + if (CommonFields.KEY_AS_STRING.getPreferredName().equals(currentFieldName)) { + bucket.setKeyAsString(parser.text()); + } else if (CommonFields.KEY.getPreferredName().equals(currentFieldName)) { + keyConsumer.accept(parser, bucket); + } else if (CommonFields.DOC_COUNT.getPreferredName().equals(currentFieldName)) { + long value = parser.longValue(); + bucket.subsetDf = value; + bucket.setDocCount(value); + } else if (SCORE.equals(currentFieldName)) { + bucket.score = parser.longValue(); + } else if (BG_COUNT.equals(currentFieldName)) { + bucket.supersetDf = parser.longValue(); + } + } 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 fd4b5ae11c2bb..85deb604a6677 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/AggregationsTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/AggregationsTests.java @@ -40,6 +40,8 @@ import org.elasticsearch.search.aggregations.bucket.range.date.InternalDateRangeTests; import org.elasticsearch.search.aggregations.bucket.range.geodistance.InternalGeoDistanceTests; import org.elasticsearch.search.aggregations.bucket.sampler.InternalSamplerTests; +import org.elasticsearch.search.aggregations.bucket.significant.SignificantLongTermsTests; +import org.elasticsearch.search.aggregations.bucket.significant.SignificantStringTermsTests; import org.elasticsearch.search.aggregations.bucket.terms.DoubleTermsTests; import org.elasticsearch.search.aggregations.bucket.terms.LongTermsTests; import org.elasticsearch.search.aggregations.bucket.terms.StringTermsTests; @@ -126,6 +128,8 @@ private static List getAggsTests() { aggsTests.add(new InternalGeoDistanceTests()); aggsTests.add(new InternalFiltersTests()); aggsTests.add(new InternalAdjacencyMatrixTests()); + aggsTests.add(new SignificantLongTermsTests()); + aggsTests.add(new SignificantStringTermsTests()); return Collections.unmodifiableList(aggsTests); } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTermsTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTermsTestCase.java index f1c54c4b503dc..d53c5fc36ae8b 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTermsTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTermsTestCase.java @@ -19,8 +19,9 @@ package org.elasticsearch.search.aggregations.bucket.significant; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregationTestCase; +import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; -import org.elasticsearch.test.InternalAggregationTestCase; import java.util.Arrays; import java.util.HashMap; @@ -30,7 +31,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; -public abstract class InternalSignificantTermsTestCase extends InternalAggregationTestCase> { +public abstract class InternalSignificantTermsTestCase extends InternalMultiBucketAggregationTestCase> { @Override protected InternalSignificantTerms createUnmappedInstance(String name, @@ -61,6 +62,25 @@ protected void assertReduced(InternalSignificantTerms reduced, List toCounts(Stream buckets, Function fn) { return buckets.collect(Collectors.toMap(SignificantTerms.Bucket::getKey, fn, Long::sum)); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantLongTermsTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantLongTermsTests.java index 7e80cf61608e5..fe20fa558e3af 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantLongTermsTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantLongTermsTests.java @@ -21,6 +21,8 @@ import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.aggregations.InternalAggregations; +import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation; import org.elasticsearch.search.aggregations.bucket.significant.heuristics.ChiSquare; import org.elasticsearch.search.aggregations.bucket.significant.heuristics.GND; import org.elasticsearch.search.aggregations.bucket.significant.heuristics.JLHScore; @@ -35,22 +37,22 @@ import java.util.Map; import java.util.Set; -import static org.elasticsearch.search.aggregations.InternalAggregations.EMPTY; - public class SignificantLongTermsTests extends InternalSignificantTermsTestCase { private SignificanceHeuristic significanceHeuristic; + private DocValueFormat format; @Before public void setUpSignificanceHeuristic() { significanceHeuristic = randomSignificanceHeuristic(); + format = randomNumericDocValueFormat(); } @Override protected InternalSignificantTerms createTestInstance(String name, List pipelineAggregators, - Map metaData) { - DocValueFormat format = DocValueFormat.RAW; + Map metaData, + InternalAggregations aggregations) { int requiredSize = randomIntBetween(1, 5); int shardSize = requiredSize + 2; final int numBuckets = randomInt(shardSize); @@ -70,7 +72,7 @@ protected InternalSignificantTerms createTestInstance(String name, globalSubsetSize += subsetDf; globalSupersetSize += supersetSize; - buckets.add(new SignificantLongTerms.Bucket(subsetDf, subsetDf, supersetDf, supersetSize, term, EMPTY, format)); + buckets.add(new SignificantLongTerms.Bucket(subsetDf, subsetDf, supersetDf, supersetSize, term, aggregations, format)); } return new SignificantLongTerms(name, requiredSize, 1L, pipelineAggregators, metaData, format, globalSubsetSize, globalSupersetSize, significanceHeuristic, buckets); @@ -81,6 +83,11 @@ protected InternalSignificantTerms createTestInstance(String name, return SignificantLongTerms::new; } + @Override + protected Class implementationClass() { + return ParsedSignificantLongTerms.class; + } + private static SignificanceHeuristic randomSignificanceHeuristic() { return randomFrom( new JLHScore(), diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantStringTermsTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantStringTermsTests.java index 82cd21cdf3873..7016a1aca290a 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantStringTermsTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantStringTermsTests.java @@ -22,6 +22,8 @@ import org.apache.lucene.util.BytesRef; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.aggregations.InternalAggregations; +import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation; import org.elasticsearch.search.aggregations.bucket.significant.heuristics.ChiSquare; import org.elasticsearch.search.aggregations.bucket.significant.heuristics.GND; import org.elasticsearch.search.aggregations.bucket.significant.heuristics.JLHScore; @@ -36,8 +38,6 @@ import java.util.Map; import java.util.Set; -import static org.elasticsearch.search.aggregations.InternalAggregations.EMPTY; - public class SignificantStringTermsTests extends InternalSignificantTermsTestCase { private SignificanceHeuristic significanceHeuristic; @@ -50,7 +50,8 @@ public void setUpSignificanceHeuristic() { @Override protected InternalSignificantTerms createTestInstance(String name, List pipelineAggregators, - Map metaData) { + Map metaData, + InternalAggregations aggregations) { DocValueFormat format = DocValueFormat.RAW; int requiredSize = randomIntBetween(1, 5); int shardSize = requiredSize + 2; @@ -71,7 +72,7 @@ protected InternalSignificantTerms createTestInstance(String name, globalSubsetSize += subsetDf; globalSupersetSize += supersetSize; - buckets.add(new SignificantStringTerms.Bucket(term, subsetDf, subsetDf, supersetDf, supersetSize, EMPTY, format)); + buckets.add(new SignificantStringTerms.Bucket(term, subsetDf, subsetDf, supersetDf, supersetSize, aggregations, format)); } return new SignificantStringTerms(name, requiredSize, 1L, pipelineAggregators, metaData, format, globalSubsetSize, globalSupersetSize, significanceHeuristic, buckets); @@ -82,6 +83,11 @@ protected InternalSignificantTerms createTestInstance(String name, return SignificantStringTerms::new; } + @Override + protected Class implementationClass() { + return ParsedSignificantStringTerms.class; + } + private static SignificanceHeuristic randomSignificanceHeuristic() { return randomFrom( new JLHScore(), 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 99b0046a88702..c256275b99482 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java @@ -66,6 +66,10 @@ import org.elasticsearch.search.aggregations.bucket.range.geodistance.ParsedGeoDistance; import org.elasticsearch.search.aggregations.bucket.sampler.InternalSampler; import org.elasticsearch.search.aggregations.bucket.sampler.ParsedSampler; +import org.elasticsearch.search.aggregations.bucket.significant.ParsedSignificantLongTerms; +import org.elasticsearch.search.aggregations.bucket.significant.ParsedSignificantStringTerms; +import org.elasticsearch.search.aggregations.bucket.significant.SignificantLongTerms; +import org.elasticsearch.search.aggregations.bucket.significant.SignificantStringTerms; import org.elasticsearch.search.aggregations.bucket.terms.DoubleTerms; import org.elasticsearch.search.aggregations.bucket.terms.LongTerms; import org.elasticsearch.search.aggregations.bucket.terms.ParsedDoubleTerms; @@ -176,6 +180,8 @@ public abstract class InternalAggregationTestCase map.put(GeoDistanceAggregationBuilder.NAME, (p, c) -> ParsedGeoDistance.fromXContent(p, (String) c)); map.put(FiltersAggregationBuilder.NAME, (p, c) -> ParsedFilters.fromXContent(p, (String) c)); map.put(AdjacencyMatrixAggregationBuilder.NAME, (p, c) -> ParsedAdjacencyMatrix.fromXContent(p, (String) c)); + map.put(SignificantLongTerms.NAME, (p, c) -> ParsedSignificantLongTerms.fromXContent(p, (String) c)); + map.put(SignificantStringTerms.NAME, (p, c) -> ParsedSignificantStringTerms.fromXContent(p, (String) c)); namedXContents = map.entrySet().stream() .map(entry -> new NamedXContentRegistry.Entry(Aggregation.class, new ParseField(entry.getKey()), entry.getValue())) From ec53aa82e564f4822a5610f76b4e0472ce1a6ab0 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 16 May 2017 13:10:41 +0200 Subject: [PATCH 2/3] Apply feedback --- .../bucket/significant/ParsedSignificantLongTerms.java | 5 +---- .../significant/ParsedSignificantStringTerms.java | 10 ++-------- .../significant/InternalSignificantTermsTestCase.java | 1 - .../bucket/significant/SignificantLongTermsTests.java | 4 +++- .../significant/SignificantStringTermsTests.java | 4 +++- 5 files changed, 9 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantLongTerms.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantLongTerms.java index e657270ed403b..79e05f4be9a30 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantLongTerms.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantLongTerms.java @@ -59,10 +59,7 @@ public String getKeyAsString() { if (keyAsString != null) { return keyAsString; } - if (key != null) { - return Long.toString(key); - } - return null; + return Long.toString(key); } public Number getKeyAsNumber() { diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantStringTerms.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantStringTerms.java index f3a2d6734ed95..28d889b15c213 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantStringTerms.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantStringTerms.java @@ -60,17 +60,11 @@ public String getKeyAsString() { if (keyAsString != null) { return keyAsString; } - if (key != null) { - return key.utf8ToString(); - } - return null; + return key.utf8ToString(); } public Number getKeyAsNumber() { - if (key != null) { - return Double.parseDouble(key.utf8ToString()); - } - return null; + return Double.parseDouble(key.utf8ToString()); } @Override diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTermsTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTermsTestCase.java index d53c5fc36ae8b..56e51416d42f8 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTermsTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTermsTestCase.java @@ -66,7 +66,6 @@ protected void assertReduced(InternalSignificantTerms reduced, List Date: Tue, 16 May 2017 13:58:31 +0200 Subject: [PATCH 3/3] Apply feedback --- .../significant/InternalSignificantTerms.java | 4 +-- .../significant/ParsedSignificantTerms.java | 26 +++++++++-------- ...nternalMultiBucketAggregationTestCase.java | 29 +++++++++++-------- .../InternalSignificantTermsTestCase.java | 17 +++++++++++ 4 files changed, 50 insertions(+), 26 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTerms.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTerms.java index acb655b19b462..ac60829c4ca9e 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTerms.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTerms.java @@ -44,8 +44,8 @@ public abstract class InternalSignificantTerms, B extends InternalSignificantTerms.Bucket> extends InternalMultiBucketAggregation implements SignificantTerms, ToXContent { - private static final String SCORE = "score"; - private static final String BG_COUNT = "bg_count"; + public static final String SCORE = "score"; + public static final String BG_COUNT = "bg_count"; @SuppressWarnings("PMD.ConstructorCallsOverridableMethod") public abstract static class Bucket> extends InternalMultiBucketAggregation.InternalBucket diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantTerms.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantTerms.java index 04466fe30687a..56be0aa60711f 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantTerms.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantTerms.java @@ -33,16 +33,20 @@ import java.util.ArrayList; import java.util.Iterator; import java.util.List; +import java.util.Map; +import java.util.function.Function; import java.util.stream.Collectors; public abstract class ParsedSignificantTerms extends ParsedMultiBucketAggregation implements SignificantTerms { - private static final String SCORE = "score"; - private static final String BG_COUNT = "bg_count"; - + private Map bucketMap; protected long subsetSize; + protected long getSubsetSize() { + return subsetSize; + } + @Override public List getBuckets() { return buckets; @@ -50,12 +54,10 @@ public List getBuckets() { @Override public SignificantTerms.Bucket getBucketByKey(String term) { - for (SignificantTerms.Bucket bucket : getBuckets()) { - if (bucket.getKeyAsString().equals(term)) { - return bucket; - } + if (bucketMap == null) { + bucketMap = buckets.stream().collect(Collectors.toMap(SignificantTerms.Bucket::getKeyAsString, Function.identity())); } - return null; + return bucketMap.get(term); } @Override @@ -121,8 +123,8 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params) builder.startObject(); keyToXContent(builder); builder.field(CommonFields.DOC_COUNT.getPreferredName(), getDocCount()); - builder.field(SCORE, getSignificanceScore()); - builder.field(BG_COUNT, getSupersetDf()); + builder.field(InternalSignificantTerms.SCORE, getSignificanceScore()); + builder.field(InternalSignificantTerms.BG_COUNT, getSupersetDf()); getAggregations().toXContentInternal(builder, params); builder.endObject(); return builder; @@ -148,9 +150,9 @@ static B parseSignificantTermsBucketXContent(final XCon long value = parser.longValue(); bucket.subsetDf = value; bucket.setDocCount(value); - } else if (SCORE.equals(currentFieldName)) { + } else if (InternalSignificantTerms.SCORE.equals(currentFieldName)) { bucket.score = parser.longValue(); - } else if (BG_COUNT.equals(currentFieldName)) { + } else if (InternalSignificantTerms.BG_COUNT.equals(currentFieldName)) { bucket.supersetDf = parser.longValue(); } } else if (token == XContentParser.Token.START_OBJECT) { 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 df977abbe8118..38e8624eeb9db 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregationTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregationTestCase.java @@ -67,28 +67,22 @@ protected abstract T createTestInstance(String name, List pi @Override protected final void assertFromXContent(T aggregation, ParsedAggregation parsedAggregation) { - assertMultiBucketsAggregation(aggregation, parsedAggregation, false); + assertMultiBucketsAggregations(aggregation, parsedAggregation, false); } public void testIterators() throws IOException { final T aggregation = createTestInstance(); - assertMultiBucketsAggregation(aggregation, parseAndAssert(aggregation, false), true); + assertMultiBucketsAggregations(aggregation, parseAndAssert(aggregation, false), true); } - private void assertMultiBucketsAggregation(Aggregation expected, Aggregation actual, boolean checkOrder) { + private void assertMultiBucketsAggregations(Aggregation expected, Aggregation actual, boolean checkOrder) { assertTrue(expected instanceof MultiBucketsAggregation); MultiBucketsAggregation expectedMultiBucketsAggregation = (MultiBucketsAggregation) expected; assertTrue(actual instanceof MultiBucketsAggregation); MultiBucketsAggregation actualMultiBucketsAggregation = (MultiBucketsAggregation) actual; - Class parsedClass = implementationClass(); - assertTrue(parsedClass != null && parsedClass.isInstance(actual)); - - assertTrue(expected instanceof InternalAggregation && actual instanceof ParsedAggregation); - assertEquals(expected.getName(), actual.getName()); - assertEquals(expected.getMetaData(), actual.getMetaData()); - assertEquals(((InternalAggregation) expected).getType(), ((ParsedAggregation) actual).getType()); + assertMultiBucketsAggregation(expectedMultiBucketsAggregation, actualMultiBucketsAggregation, checkOrder); List expectedBuckets = expectedMultiBucketsAggregation.getBuckets(); List actualBuckets = actualMultiBucketsAggregation.getBuckets(); @@ -117,6 +111,17 @@ private void assertMultiBucketsAggregation(Aggregation expected, Aggregation act } } + protected void assertMultiBucketsAggregation(MultiBucketsAggregation expected, MultiBucketsAggregation actual, boolean checkOrder) { + Class parsedClass = implementationClass(); + assertNotNull("Parsed aggregation class must not be null", parsedClass); + assertTrue(parsedClass.isInstance(actual)); + + assertTrue(expected instanceof InternalAggregation); + assertEquals(expected.getName(), actual.getName()); + assertEquals(expected.getMetaData(), actual.getMetaData()); + assertEquals(((InternalAggregation) expected).getType(), ((ParsedAggregation) actual).getType()); + } + protected void assertBucket(MultiBucketsAggregation.Bucket expected, MultiBucketsAggregation.Bucket actual, boolean checkOrder) { assertTrue(expected instanceof InternalMultiBucketAggregation.InternalBucket); assertTrue(actual instanceof ParsedMultiBucketAggregation.ParsedBucket); @@ -136,13 +141,13 @@ protected void assertBucket(MultiBucketsAggregation.Bucket expected, MultiBucket while (expectedIt.hasNext()) { Aggregation expectedAggregation = expectedIt.next(); Aggregation actualAggregation = actualIt.next(); - assertMultiBucketsAggregation(expectedAggregation, actualAggregation, true); + assertMultiBucketsAggregations(expectedAggregation, actualAggregation, true); } } else { for (Aggregation expectedAggregation : expectedAggregations) { Aggregation actualAggregation = actualAggregations.get(expectedAggregation.getName()); assertNotNull(actualAggregation); - assertMultiBucketsAggregation(expectedAggregation, actualAggregation, false); + assertMultiBucketsAggregations(expectedAggregation, actualAggregation, false); } } } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTermsTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTermsTestCase.java index 56e51416d42f8..9a86e44b2ac01 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTermsTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/InternalSignificantTermsTestCase.java @@ -62,6 +62,23 @@ protected void assertReduced(InternalSignificantTerms reduced, List