From 8e18e662ecf265873ff5d6cc156b87b23dfbffad Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Tue, 4 Feb 2020 16:37:22 +0100 Subject: [PATCH 1/6] disallow duplicate percentile definitions and out of range --- .../metrics/PercentilesAggregationBuilder.java | 12 ++++++++++++ .../aggregations/metrics/PercentilesTests.java | 15 +++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java index 8860e1b52e6f7..e65d9e9d5e457 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java @@ -181,6 +181,18 @@ public PercentilesAggregationBuilder percentiles(double... percents) { } double[] sortedPercents = Arrays.copyOf(percents, percents.length); Arrays.sort(sortedPercents); + double previousPercent = -1.0; + for (double percent : sortedPercents) { + if (percent < 0.0 || percent > 100.0) { + throw new IllegalArgumentException("percent must be in [0,100], got [" + percent + "]: [" + name + "]"); + } + + if(percent == previousPercent) { + throw new IllegalArgumentException("percent [" + percent + "] has been specified twice: [" + name + "]"); + } + previousPercent = percent; + } + this.percents = sortedPercents; return this; } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentilesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentilesTests.java index 2f8d22889e317..bb5c8a77f2108 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentilesTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentilesTests.java @@ -70,6 +70,21 @@ public void testNullOrEmptyPercentilesThrows() throws IOException { assertEquals("[percents] must not be empty: [testAgg]", ex.getMessage()); } + public void testOutOfRangePercentilesThrows() throws IOException { + PercentilesAggregationBuilder builder = new PercentilesAggregationBuilder("testAgg"); + IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> builder.percentiles(-0.4)); + assertEquals("percent must be in [0,100], got [-0.4]: [testAgg]", ex.getMessage()); + + ex = expectThrows(IllegalArgumentException.class, () -> builder.percentiles(104)); + assertEquals("percent must be in [0,100], got [104.0]: [testAgg]", ex.getMessage()); + } + + public void testDuplicatePercentilesThrows() throws IOException { + PercentilesAggregationBuilder builder = new PercentilesAggregationBuilder("testAgg"); + IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> builder.percentiles(5, 42, 10, 99, 42, 87)); + assertEquals("percent [42.0] has been specified twice: [testAgg]", ex.getMessage()); + } + public void testExceptionMultipleMethods() throws IOException { final String illegalAgg = "{\n" + " \"percentiles\": {\n" + From 442fb226130c1f6f1eecc11611de2d1dfa6e237f Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Wed, 5 Feb 2020 12:03:42 +0100 Subject: [PATCH 2/6] adapt tests to avoid duplicates for percentiles --- .../aggregations/metrics/HDRPercentilesIT.java | 16 +++++++++------- .../metrics/TDigestPercentilesIT.java | 16 +++++++++------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java index 3bab055161aee..5f49bbf783fa2 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java @@ -26,19 +26,21 @@ import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptType; import org.elasticsearch.search.aggregations.AggregationTestScriptsPlugin; +import org.elasticsearch.search.aggregations.BucketOrder; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.bucket.filter.Filter; import org.elasticsearch.search.aggregations.bucket.global.Global; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram; import org.elasticsearch.search.aggregations.bucket.terms.Terms; -import org.elasticsearch.search.aggregations.BucketOrder; import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import static java.util.Collections.emptyMap; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; @@ -67,21 +69,21 @@ protected Collection> nodePlugins() { private static double[] randomPercentiles() { final int length = randomIntBetween(1, 20); - final double[] percentiles = new double[length]; - for (int i = 0; i < percentiles.length; ++i) { + final Set uniquedPercentiles = new HashSet<>(); + for (int i = 0; i < length; ++i) { switch (randomInt(20)) { case 0: - percentiles[i] = 0; + uniquedPercentiles.add(0.0); break; case 1: - percentiles[i] = 100; + uniquedPercentiles.add(100.0); break; default: - percentiles[i] = randomDouble() * 100; + uniquedPercentiles.add(randomDouble() * 100); break; } } - Arrays.sort(percentiles); + double[] percentiles= uniquedPercentiles.stream().mapToDouble(Double::doubleValue).sorted().toArray(); LogManager.getLogger(HDRPercentilesIT.class).info("Using percentiles={}", Arrays.toString(percentiles)); return percentiles; } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesIT.java index 88a2b450eac17..7c607de570ccc 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesIT.java @@ -26,19 +26,21 @@ import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptType; import org.elasticsearch.search.aggregations.AggregationTestScriptsPlugin; +import org.elasticsearch.search.aggregations.BucketOrder; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.bucket.filter.Filter; import org.elasticsearch.search.aggregations.bucket.global.Global; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram; import org.elasticsearch.search.aggregations.bucket.terms.Terms; -import org.elasticsearch.search.aggregations.BucketOrder; import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import static java.util.Collections.emptyMap; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; @@ -66,21 +68,21 @@ protected Collection> nodePlugins() { private static double[] randomPercentiles() { final int length = randomIntBetween(1, 20); - final double[] percentiles = new double[length]; - for (int i = 0; i < percentiles.length; ++i) { + final Set uniquedPercentiles = new HashSet<>(); + for (int i = 0; i < length; ++i) { switch (randomInt(20)) { case 0: - percentiles[i] = 0; + uniquedPercentiles.add(0.0); break; case 1: - percentiles[i] = 100; + uniquedPercentiles.add(100.0); break; default: - percentiles[i] = randomDouble() * 100; + uniquedPercentiles.add(randomDouble() * 100); break; } } - Arrays.sort(percentiles); + double[] percentiles= uniquedPercentiles.stream().mapToDouble(Double::doubleValue).sorted().toArray(); LogManager.getLogger(TDigestPercentilesIT.class).info("Using percentiles={}", Arrays.toString(percentiles)); return percentiles; } From 5f0cc8fc513e43b3b8401532033097f52dd290e9 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Thu, 6 Feb 2020 09:15:34 +0100 Subject: [PATCH 3/6] fix one more wrong percentile test --- .../mapper/HistogramPercentileAggregationTests.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/mapper/HistogramPercentileAggregationTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/mapper/HistogramPercentileAggregationTests.java index e33f5dae95c1e..6d95f74e4428a 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/mapper/HistogramPercentileAggregationTests.java +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/mapper/HistogramPercentileAggregationTests.java @@ -7,11 +7,11 @@ import com.tdunning.math.stats.Centroid; + import org.HdrHistogram.DoubleHistogram; import org.HdrHistogram.DoubleHistogramIterationValue; import org.apache.lucene.util.TestUtil; import org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest; - import org.elasticsearch.action.admin.indices.refresh.RefreshRequest; import org.elasticsearch.action.bulk.BulkRequest; import org.elasticsearch.action.index.IndexRequest; @@ -19,7 +19,6 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.plugins.Plugin; - import org.elasticsearch.search.aggregations.AggregationBuilders; import org.elasticsearch.search.aggregations.metrics.InternalHDRPercentiles; import org.elasticsearch.search.aggregations.metrics.InternalTDigestPercentiles; @@ -222,7 +221,7 @@ public void testTDigestHistogram() throws Exception { PercentilesAggregationBuilder builder = AggregationBuilders.percentiles("agg").field("inner.data").method(PercentilesMethod.TDIGEST) - .compression(compression).percentiles(10, 25, 500, 75); + .compression(compression).percentiles(10, 25, 50, 75); SearchResponse responseRaw = client().prepareSearch("raw").addAggregation(builder).get(); SearchResponse responsePreAgg = client().prepareSearch("pre_agg").addAggregation(builder).get(); From 43dcf62a4fc4a64217d5351a5cecba250c07a06d Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Fri, 7 Feb 2020 10:49:12 +0100 Subject: [PATCH 4/6] remove the duplication check, tbd for a separate 8.0 only PR --- .../aggregations/metrics/PercentilesAggregationBuilder.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java index e65d9e9d5e457..29f36f117fff1 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java @@ -181,16 +181,10 @@ public PercentilesAggregationBuilder percentiles(double... percents) { } double[] sortedPercents = Arrays.copyOf(percents, percents.length); Arrays.sort(sortedPercents); - double previousPercent = -1.0; for (double percent : sortedPercents) { if (percent < 0.0 || percent > 100.0) { throw new IllegalArgumentException("percent must be in [0,100], got [" + percent + "]: [" + name + "]"); } - - if(percent == previousPercent) { - throw new IllegalArgumentException("percent [" + percent + "] has been specified twice: [" + name + "]"); - } - previousPercent = percent; } this.percents = sortedPercents; From 3726ea8dfc1c0cecb9522207c0fa9334fcd38b2b Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Fri, 7 Feb 2020 10:49:34 +0100 Subject: [PATCH 5/6] apply review suggestion --- .../search/aggregations/metrics/HDRPercentilesIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java index 5f49bbf783fa2..94f5ae0fb606a 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java @@ -70,7 +70,7 @@ protected Collection> nodePlugins() { private static double[] randomPercentiles() { final int length = randomIntBetween(1, 20); final Set uniquedPercentiles = new HashSet<>(); - for (int i = 0; i < length; ++i) { + while (uniquedPercentiles.size() < length) { switch (randomInt(20)) { case 0: uniquedPercentiles.add(0.0); From b3fe9a1dfe879c654fd4e7f926b64235c0a4486b Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Fri, 7 Feb 2020 10:53:12 +0100 Subject: [PATCH 6/6] remove unit test for dups --- .../search/aggregations/metrics/PercentilesTests.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentilesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentilesTests.java index bb5c8a77f2108..14036421ae935 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentilesTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentilesTests.java @@ -79,12 +79,6 @@ public void testOutOfRangePercentilesThrows() throws IOException { assertEquals("percent must be in [0,100], got [104.0]: [testAgg]", ex.getMessage()); } - public void testDuplicatePercentilesThrows() throws IOException { - PercentilesAggregationBuilder builder = new PercentilesAggregationBuilder("testAgg"); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> builder.percentiles(5, 42, 10, 99, 42, 87)); - assertEquals("percent [42.0] has been specified twice: [testAgg]", ex.getMessage()); - } - public void testExceptionMultipleMethods() throws IOException { final String illegalAgg = "{\n" + " \"percentiles\": {\n" +