Deprecate specifying the same percentile multiple times in percentiles (

#65252) Deprecate specifying the same percentile multiple times in percentiles aggregation Regression of: #52257 Fixes: #65240
elastic · Nov 30, 2020 · 32bfb4b · 32bfb4b
1 parent 6f50a7a
commit 32bfb4b
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 1 deletion.
diff --git a/...ain/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java b/...ain/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java
@@ -21,6 +21,7 @@
 
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.xcontent.ConstructingObjectParser;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.search.aggregations.AggregationBuilder;
@@ -44,6 +45,7 @@ public class PercentilesAggregationBuilder extends AbstractPercentilesAggregatio
 
     private static final double[] DEFAULT_PERCENTS = new double[] { 1, 5, 25, 50, 75, 95, 99 };
     private static final ParseField PERCENTS_FIELD = new ParseField("percents");
+    private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(PercentilesAggregationBuilder.class);
 
     private static final ConstructingObjectParser<PercentilesAggregationBuilder, String> PARSER;
     static {
@@ -112,11 +114,17 @@ private static double[] validatePercentiles(double[] percents, String aggName) {
             throw new IllegalArgumentException("[percents] must not be empty: [" + aggName + "]");
         }
         double[] sortedPercents = Arrays.copyOf(percents, percents.length);
+        double previousPercent = -1.0;
         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 + "]: [" + aggName + "]");
             }
+
+            if (percent == previousPercent) {
+                deprecationLogger.deprecate("percents", "percent [{}] has been specified more than once, percents must be unique", percent);
+            }
+            previousPercent = percent;
         }
         return sortedPercents;
     }

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
@@ -78,6 +78,21 @@ public void testOutOfRangePercentilesThrows() throws IOException {
         assertEquals("percent must be in [0,100], got [104.0]: [testAgg]", ex.getMessage());
     }
 
+    public void testDuplicatePercentilesDeprecated() throws IOException {
+        PercentilesAggregationBuilder builder = new PercentilesAggregationBuilder("testAgg");
+
+        // throws in 8.x, deprecated in 7.x
+        builder.percentiles(5, 42, 10, 99, 42, 87);
+
+        assertWarnings("percent [42.0] has been specified more than once, percents must be unique");
+
+        builder.percentiles(5, 42, 42, 43, 43, 87);
+        assertWarnings(
+            "percent [42.0] has been specified more than once, percents must be unique",
+            "percent [43.0] has been specified more than once, percents must be unique"
+        );
+    }
+
     public void testExceptionMultipleMethods() throws IOException {
         final String illegalAgg = "{\n" +
             "       \"percentiles\": {\n" +

diff --git a/...t/java/org/elasticsearch/xpack/transform/transforms/pivot/TransformAggregationsTests.java b/...t/java/org/elasticsearch/xpack/transform/transforms/pivot/TransformAggregationsTests.java
@@ -151,7 +151,7 @@ public void testGetAggregationOutputTypesPercentiles() {
         assertEquals("percentiles", outputTypes.get("percentiles.10"));
 
         // note: using the constructor, omits validation, in reality this test might fail
-        percentialAggregationBuilder = new PercentilesAggregationBuilder("percentiles").percentiles(1.0, 5.0, 5.0, 10.0);
+        percentialAggregationBuilder = new PercentilesAggregationBuilder("percentiles", new double[] { 1, 5, 5, 10 }, null);
 
         inputAndOutputTypes = TransformAggregations.getAggregationInputAndOutputTypes(percentialAggregationBuilder);
         assertTrue(inputAndOutputTypes.v1().isEmpty());