From b1f58885e1961820bebd2c93850596a39f9ca557 Mon Sep 17 00:00:00 2001 From: Ignacio Vera Date: Tue, 5 Nov 2024 15:31:08 +0100 Subject: [PATCH] Handle with illegalArgumentExceptions negative values in HDR percentile aggregations (#116174) This commit changes to an illegalArgumentException which translate in a bad request (400). --- docs/changelog/116174.yaml | 7 +++++ modules/aggregations/build.gradle | 1 + .../aggregations/percentiles_hdr_metric.yml | 2 +- .../AbstractHDRPercentilesAggregator.java | 12 +++++++-- .../HDRPercentileRanksAggregatorTests.java | 26 +++++++++++++++++++ .../HDRPercentilesAggregatorTests.java | 12 +++++++++ 6 files changed, 57 insertions(+), 3 deletions(-) create mode 100644 docs/changelog/116174.yaml diff --git a/docs/changelog/116174.yaml b/docs/changelog/116174.yaml new file mode 100644 index 0000000000000..e3403f2c1c7fc --- /dev/null +++ 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 index f558ce8b9cfdb..a1ab6363166cb 100644 --- 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/modules/aggregations/src/yamlRestTest/resources/rest-api-spec/test/aggregations/percentiles_hdr_metric.yml b/modules/aggregations/src/yamlRestTest/resources/rest-api-spec/test/aggregations/percentiles_hdr_metric.yml index 6bf37425d9af4..8b67ba7056f37 100644 --- a/modules/aggregations/src/yamlRestTest/resources/rest-api-spec/test/aggregations/percentiles_hdr_metric.yml +++ b/modules/aggregations/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/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractHDRPercentilesAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractHDRPercentilesAggregator.java index 039bd0dd67592..72e1db245338e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractHDRPercentilesAggregator.java +++ b/server/src/main/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/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksAggregatorTests.java index 7533028d6ea08..de9025795ce87 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksAggregatorTests.java +++ b/server/src/test/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/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesAggregatorTests.java index 2001286f27f0f..b8f4ab100e84c 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesAggregatorTests.java +++ b/server/src/test/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 buildIndex, Consumer verify) throws IOException { MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType("number", NumberFieldMapper.NumberType.LONG);