From c1ba6997ff64008e8a843fda77905a54ff33bcec Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Tue, 18 Apr 2017 16:54:17 +0200 Subject: [PATCH] AbstractParsedPercentiles should use Percentile class (#24160) Now the Percentile interface has been merged with the InternalPercentile class in core (#24154) the AbstractParsedPercentiles should use it. This commit also changes InternalPercentilesRanksTestCase so that it now tests the iterator obtained from parsed percentiles ranks aggregations. Adding this new test raised an issue in the iterators where key and value are "swapped" in internal implementations when building the iterators (see InternalTDigestPercentileRanks.Iter constructor that accepts the `keys` as the first parameter named `values`, each key being mapped to the `value` field of Percentile class). This is because percentiles ranks aggs inverts percentiles/values compared to the percentiles aggs. * Add assume in InternalAggregationTestCase * Update after Luca review --- .../AbstractParsedPercentiles.java | 2 +- .../hdr/ParsedHDRPercentileRanks.java | 19 +++++++++ .../tdigest/ParsedTDigestPercentileRanks.java | 19 +++++++++ .../InternalAggregationTestCase.java | 9 ++-- .../InternalPercentilesRanksTestCase.java | 42 +++++++++++++++++++ 5 files changed, 85 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/AbstractParsedPercentiles.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/AbstractParsedPercentiles.java index f48e7257b1c..a45d089cc79 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/AbstractParsedPercentiles.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/AbstractParsedPercentiles.java @@ -81,7 +81,7 @@ public abstract class AbstractParsedPercentiles extends ParsedAggregation implem @Override public Percentile next() { Map.Entry next = iterator.next(); - return new InternalPercentile(next.getKey(), next.getValue()); + return new Percentile(next.getKey(), next.getValue()); } }; } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/ParsedHDRPercentileRanks.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/ParsedHDRPercentileRanks.java index 31fc8f88cfc..79749df1176 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/ParsedHDRPercentileRanks.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/ParsedHDRPercentileRanks.java @@ -23,8 +23,10 @@ import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.search.aggregations.metrics.percentiles.AbstractParsedPercentiles; import org.elasticsearch.search.aggregations.metrics.percentiles.ParsedPercentileRanks; +import org.elasticsearch.search.aggregations.metrics.percentiles.Percentile; import java.io.IOException; +import java.util.Iterator; public class ParsedHDRPercentileRanks extends ParsedPercentileRanks { @@ -33,6 +35,23 @@ public class ParsedHDRPercentileRanks extends ParsedPercentileRanks { return InternalHDRPercentileRanks.NAME; } + @Override + public Iterator iterator() { + final Iterator iterator = super.iterator(); + return new Iterator() { + @Override + public boolean hasNext() { + return iterator.hasNext(); + } + + @Override + public Percentile next() { + Percentile percentile = iterator.next(); + return new Percentile(percentile.getValue(), percentile.getPercent()); + } + }; + } + private static ObjectParser PARSER = new ObjectParser<>(ParsedHDRPercentileRanks.class.getSimpleName(), true, ParsedHDRPercentileRanks::new); static { diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/ParsedTDigestPercentileRanks.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/ParsedTDigestPercentileRanks.java index 57f3df04115..3d51e98d622 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/ParsedTDigestPercentileRanks.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/ParsedTDigestPercentileRanks.java @@ -23,8 +23,10 @@ import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.search.aggregations.metrics.percentiles.AbstractParsedPercentiles; import org.elasticsearch.search.aggregations.metrics.percentiles.ParsedPercentileRanks; +import org.elasticsearch.search.aggregations.metrics.percentiles.Percentile; import java.io.IOException; +import java.util.Iterator; public class ParsedTDigestPercentileRanks extends ParsedPercentileRanks { @@ -33,6 +35,23 @@ public class ParsedTDigestPercentileRanks extends ParsedPercentileRanks { return InternalTDigestPercentileRanks.NAME; } + @Override + public Iterator iterator() { + final Iterator iterator = super.iterator(); + return new Iterator() { + @Override + public boolean hasNext() { + return iterator.hasNext(); + } + + @Override + public Percentile next() { + Percentile percentile = iterator.next(); + return new Percentile(percentile.getValue(), percentile.getPercent()); + } + }; + } + private static ObjectParser PARSER = new ObjectParser<>(ParsedTDigestPercentileRanks.class.getSimpleName(), true, ParsedTDigestPercentileRanks::new); static { diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/InternalAggregationTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/InternalAggregationTestCase.java index d6b50f69bc9..e6c5098761c 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/InternalAggregationTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/InternalAggregationTestCase.java @@ -54,7 +54,6 @@ import static java.util.Collections.emptyList; import static java.util.Collections.singletonMap; import static org.elasticsearch.common.xcontent.XContentHelper.toXContent; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent; -import static org.hamcrest.Matchers.containsString; public abstract class InternalAggregationTestCase extends AbstractWireSerializingTestCase { @@ -169,6 +168,10 @@ public abstract class InternalAggregationTestCase final NamedXContentRegistry xContentRegistry = xContentRegistry(); final T aggregation = createTestInstance(); + //norelease Remove this assumption when all aggregations can be parsed back. + assumeTrue("This test does not support the aggregation type yet", + getNamedXContents().stream().filter(entry -> entry.name.match(aggregation.getType())).count() > 0); + final ToXContent.Params params = new ToXContent.MapParams(singletonMap(RestSearchAction.TYPED_KEYS_PARAM, "true")); final boolean humanReadable = randomBoolean(); final XContentType xContentType = randomFrom(XContentType.values()); @@ -199,10 +202,6 @@ public abstract class InternalAggregationTestCase final BytesReference parsedBytes = toXContent((ToXContent) parsedAggregation, xContentType, params, humanReadable); assertToXContentEquivalent(originalBytes, parsedBytes, xContentType); assertFromXContent(aggregation, (ParsedAggregation) parsedAggregation); - - } catch (NamedXContentRegistry.UnknownNamedObjectException e) { - //norelease Remove this catch block when all aggregations can be parsed back. - assertThat(e.getMessage(), containsString("Unknown Aggregation")); } } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java index 23acfcd779a..be7d55e5447 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java @@ -19,15 +19,26 @@ package org.elasticsearch.search.aggregations.metrics.percentiles; +import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.rest.action.search.RestSearchAction; import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.aggregations.Aggregation; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.InternalAggregationTestCase; import org.elasticsearch.search.aggregations.ParsedAggregation; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import java.io.IOException; +import java.util.Iterator; import java.util.List; import java.util.Map; +import static java.util.Collections.singletonMap; +import static org.elasticsearch.common.xcontent.XContentHelper.toXContent; + public abstract class InternalPercentilesRanksTestCase extends InternalAggregationTestCase { @Override @@ -64,5 +75,36 @@ public abstract class InternalPercentilesRanksTestCase it = ((PercentileRanks) aggregation).iterator(); + final Iterator parsedIt = ((PercentileRanks) parsedAggregation).iterator(); + while (it.hasNext()) { + assertEquals(it.next(), parsedIt.next()); + } + } + protected abstract Class parsedParsedPercentileRanksClass(); }