Percentiles aggregation validation checks for range (#51871)

disallow to specify percentile out of range [0,100]. This also fixes a problem in transform by failing validation if an invalid percentile configuration is used.
elastic · Feb 11, 2020 · 8b9e18e · 8b9e18e
1 parent c82a050
commit 8b9e18e
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 17 deletions.
diff --git a/...ain/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java b/...ain/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java
@@ -181,6 +181,12 @@ public PercentilesAggregationBuilder percentiles(double... percents) {
         }
         double[] sortedPercents = Arrays.copyOf(percents, percents.length);
         Arrays.sort(sortedPercents);
+        for (double percent : sortedPercents) {
+            if (percent < 0.0 || percent > 100.0) {
+                throw new IllegalArgumentException("percent must be in [0,100], got [" + percent + "]: [" + name + "]");
+            }
+        }
+
         this.percents = sortedPercents;
         return this;
     }

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
@@ -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<Class<? extends Plugin>> 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<Double> uniquedPercentiles = new HashSet<>();
+        while (uniquedPercentiles.size() < length) {
             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/PercentilesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentilesTests.java
@@ -70,6 +70,15 @@ 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 testExceptionMultipleMethods() throws IOException {
         final String illegalAgg = "{\n" +
             "       \"percentiles\": {\n" +

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
@@ -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<Class<? extends Plugin>> 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<Double> 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;
     }

diff --git a/...st/java/org/elasticsearch/xpack/analytics/mapper/HistogramPercentileAggregationTests.java b/...st/java/org/elasticsearch/xpack/analytics/mapper/HistogramPercentileAggregationTests.java
@@ -7,19 +7,18 @@
 
 
 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;
 import org.elasticsearch.action.search.SearchResponse;
 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();