Handle with illegalArgumentExceptions negative values in HDR percenti…

…le aggregations (elastic#116174) This commit changes to an illegalArgumentException which translate in a bad request (400).
alexey-ivanov-es · Nov 28, 2024 · b1f5888 · b1f5888
1 parent eaffa1c
commit b1f5888
Show file tree

Hide file tree

Showing 6 changed files with 57 additions and 3 deletions.
diff --git a/docs/changelog/116174.yaml b/docs/changelog/116174.yaml
@@ -0,0 +1,7 @@
+pr: 116174
+summary: Handle with `illegalArgumentExceptions` negative values in HDR percentile
+  aggregations
+area: Aggregations
+type: bug
+issues:
+ - 115777
diff --git a/modules/aggregations/build.gradle b/modules/aggregations/build.gradle
@@ -49,4 +49,5 @@ dependencies {
 tasks.named("yamlRestCompatTestTransform").configure({ task ->
   task.skipTest("aggregations/date_agg_per_day_of_week/Date aggregartion per day of week", "week-date behaviour has changed")
   task.skipTest("aggregations/time_series/Configure with no synthetic source", "temporary until backport")
+  task.skipTest("aggregations/percentiles_hdr_metric/Negative values test", "returned exception has changed")
 })
diff --git a/...ons/src/yamlRestTest/resources/rest-api-spec/test/aggregations/percentiles_hdr_metric.yml b/...ons/src/yamlRestTest/resources/rest-api-spec/test/aggregations/percentiles_hdr_metric.yml
@@ -446,4 +446,4 @@ setup:
   - match: { aggregations.percentiles_int.values.75\.0: 101.0615234375 }
   - match: { aggregations.percentiles_int.values.95\.0: 151.1240234375 }
   - match: { aggregations.percentiles_int.values.99\.0: 151.1240234375 }
-  - match: { _shards.failures.0.reason.type: array_index_out_of_bounds_exception }
+  - match: { _shards.failures.0.reason.type: illegal_argument_exception }
diff --git a/.../java/org/elasticsearch/search/aggregations/metrics/AbstractHDRPercentilesAggregator.java b/.../java/org/elasticsearch/search/aggregations/metrics/AbstractHDRPercentilesAggregator.java
@@ -61,7 +61,11 @@ public void collect(int doc, long bucket) throws IOException {
                 if (values.advanceExact(doc)) {
                     final DoubleHistogram state = getExistingOrNewHistogram(bigArrays(), bucket);
                     for (int i = 0; i < values.docValueCount(); i++) {
-                        state.recordValue(values.nextValue());
+                        final double value = values.nextValue();
+                        if (value < 0) {
+                            throw new IllegalArgumentException("Negative values are not supported by HDR aggregation");
+                        }
+                        state.recordValue(value);
                     }
                 }
             }
@@ -74,8 +78,12 @@ protected LeafBucketCollector getLeafCollector(NumericDoubleValues values, LeafB
             @Override
             public void collect(int doc, long bucket) throws IOException {
                 if (values.advanceExact(doc)) {
+                    final double value = values.doubleValue();
+                    if (value < 0) {
+                        throw new IllegalArgumentException("Negative values are not supported by HDR aggregation");
+                    }
                     final DoubleHistogram state = getExistingOrNewHistogram(bigArrays(), bucket);
-                    state.recordValue(values.doubleValue());
+                    state.recordValue(value);
                 }
             }
         };

diff --git a/...java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksAggregatorTests.java b/...java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksAggregatorTests.java
@@ -10,6 +10,7 @@
 package org.elasticsearch.search.aggregations.metrics;
 
 import org.apache.lucene.document.Document;
+import org.apache.lucene.document.NumericDocValuesField;
 import org.apache.lucene.document.SortedNumericDocValuesField;
 import org.apache.lucene.index.IndexReader;
 import org.apache.lucene.index.MultiReader;
@@ -29,6 +30,9 @@
 import java.util.Iterator;
 import java.util.List;
 
+import static java.util.Collections.singleton;
+import static org.hamcrest.Matchers.equalTo;
+
 public class HDRPercentileRanksAggregatorTests extends AggregatorTestCase {
 
     @Override
@@ -100,4 +104,26 @@ public void testEmptyValues() throws IOException {
 
         assertThat(e.getMessage(), Matchers.equalTo("[values] must not be an empty array: [my_agg]"));
     }
+
+    public void testInvalidNegativeNumber() throws IOException {
+        try (Directory dir = newDirectory(); RandomIndexWriter iw = new RandomIndexWriter(random(), dir)) {
+            iw.addDocument(singleton(new NumericDocValuesField("number", 60)));
+            iw.addDocument(singleton(new NumericDocValuesField("number", 40)));
+            iw.addDocument(singleton(new NumericDocValuesField("number", -20)));
+            iw.addDocument(singleton(new NumericDocValuesField("number", 10)));
+            iw.commit();
+
+            PercentileRanksAggregationBuilder aggBuilder = new PercentileRanksAggregationBuilder("my_agg", new double[] { 0.1, 0.5, 12 })
+                .field("number")
+                .method(PercentilesMethod.HDR);
+            MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType("number", NumberFieldMapper.NumberType.LONG);
+            try (IndexReader reader = iw.getReader()) {
+                IllegalArgumentException e = expectThrows(
+                    IllegalArgumentException.class,
+                    () -> searchAndReduce(reader, new AggTestConfig(aggBuilder, fieldType))
+                );
+                assertThat(e.getMessage(), equalTo("Negative values are not supported by HDR aggregation"));
+            }
+        }
+    }
 }
diff --git a/...est/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesAggregatorTests.java b/...est/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesAggregatorTests.java
@@ -164,6 +164,18 @@ public void testHdrThenTdigestSettings() throws Exception {
         assertThat(e.getMessage(), equalTo("Cannot set [compression] because the method has already been configured for HDRHistogram"));
     }
 
+    public void testInvalidNegativeNumber() {
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
+            testCase(new MatchAllDocsQuery(), iw -> {
+                iw.addDocument(singleton(new NumericDocValuesField("number", 60)));
+                iw.addDocument(singleton(new NumericDocValuesField("number", 40)));
+                iw.addDocument(singleton(new NumericDocValuesField("number", -20)));
+                iw.addDocument(singleton(new NumericDocValuesField("number", 10)));
+            }, hdr -> { fail("Aggregation should have failed due to negative value"); });
+        });
+        assertThat(e.getMessage(), equalTo("Negative values are not supported by HDR aggregation"));
+    }
+
     private void testCase(Query query, CheckedConsumer<RandomIndexWriter, IOException> buildIndex, Consumer<InternalHDRPercentiles> verify)
         throws IOException {
         MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType("number", NumberFieldMapper.NumberType.LONG);