diff --git a/docs/changelog/96668.yaml b/docs/changelog/96668.yaml new file mode 100644 index 0000000000000..483c0f462743f --- /dev/null +++ b/docs/changelog/96668.yaml @@ -0,0 +1,6 @@ +pr: 96668 +summary: Fix iteration of empty percentiles throwing Null Pointer Exception +area: Aggregations +type: bug +issues: + - 96626 diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalHDRPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalHDRPercentiles.java index 4abe4200e88bc..a1d6800c66b5c 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalHDRPercentiles.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalHDRPercentiles.java @@ -21,6 +21,8 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.util.Arrays; +import java.util.Collections; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Objects; @@ -28,6 +30,7 @@ abstract class AbstractInternalHDRPercentiles extends InternalNumericMetricsAggregation.MultiValue { + protected static final Iterator EMPTY_ITERATOR = Collections.emptyIterator(); private static final DoubleHistogram EMPTY_HISTOGRAM_THREE_DIGITS = new DoubleHistogram(3); private static final DoubleHistogram EMPTY_HISTOGRAM_ZERO_DIGITS = new EmptyDoubleHdrHistogram(); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalTDigestPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalTDigestPercentiles.java index ed0ab17ce1004..4fb10c4b170c7 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalTDigestPercentiles.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalTDigestPercentiles.java @@ -19,12 +19,16 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collections; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Objects; abstract class AbstractInternalTDigestPercentiles extends InternalNumericMetricsAggregation.MultiValue { + protected static final Iterator EMPTY_ITERATOR = Collections.emptyIterator(); + // NOTE: using compression = 1.0 empty histograms will track just about 5 centroids. // This reduces the amount of data to serialize and deserialize. private static final TDigestState EMPTY_HISTOGRAM = new EmptyTDigestState(); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentileRanks.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentileRanks.java index 7a2bf3f5b820d..640e66c6fc96d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentileRanks.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentileRanks.java @@ -14,6 +14,7 @@ import java.io.IOException; import java.util.Iterator; import java.util.Map; +import java.util.Objects; public class InternalHDRPercentileRanks extends AbstractInternalHDRPercentiles implements PercentileRanks { public static final String NAME = "hdr_percentile_ranks"; @@ -43,6 +44,9 @@ public String getWriteableName() { @Override public Iterator iterator() { + if (state == null) { + return EMPTY_ITERATOR; + } return new Iter(keys, state); } @@ -93,7 +97,7 @@ public static class Iter implements Iterator { public Iter(double[] values, DoubleHistogram state) { this.values = values; - this.state = state; + this.state = Objects.requireNonNull(state); i = 0; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentiles.java index d8a71d19506e3..1ba4656196c77 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentiles.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentiles.java @@ -14,6 +14,7 @@ import java.io.IOException; import java.util.Iterator; import java.util.Map; +import java.util.Objects; public class InternalHDRPercentiles extends AbstractInternalHDRPercentiles implements Percentiles { public static final String NAME = "hdr_percentiles"; @@ -43,6 +44,9 @@ public String getWriteableName() { @Override public Iterator iterator() { + if (state == null) { + return EMPTY_ITERATOR; + } return new Iter(keys, state); } @@ -83,7 +87,7 @@ public static class Iter implements Iterator { public Iter(double[] percents, DoubleHistogram state) { this.percents = percents; - this.state = state; + this.state = Objects.requireNonNull(state); i = 0; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentileRanks.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentileRanks.java index c0c79afc494cf..9fc1c5980faf9 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentileRanks.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentileRanks.java @@ -13,6 +13,7 @@ import java.io.IOException; import java.util.Iterator; import java.util.Map; +import java.util.Objects; public class InternalTDigestPercentileRanks extends AbstractInternalTDigestPercentiles implements PercentileRanks { public static final String NAME = "tdigest_percentile_ranks"; @@ -42,6 +43,9 @@ public String getWriteableName() { @Override public Iterator iterator() { + if (state == null) { + return EMPTY_ITERATOR; + } return new Iter(keys, state); } @@ -89,7 +93,7 @@ public static class Iter implements Iterator { public Iter(double[] values, TDigestState state) { this.values = values; - this.state = state; + this.state = Objects.requireNonNull(state); i = 0; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentiles.java index bf4f08f126797..f3aea151b1112 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentiles.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentiles.java @@ -13,6 +13,7 @@ import java.io.IOException; import java.util.Iterator; import java.util.Map; +import java.util.Objects; public class InternalTDigestPercentiles extends AbstractInternalTDigestPercentiles implements Percentiles { public static final String NAME = "tdigest_percentiles"; @@ -42,6 +43,9 @@ public String getWriteableName() { @Override public Iterator iterator() { + if (state == null) { + return EMPTY_ITERATOR; + } return new Iter(keys, state); } @@ -79,7 +83,7 @@ public static class Iter implements Iterator { public Iter(double[] percents, TDigestState state) { this.percents = percents; - this.state = state; + this.state = Objects.requireNonNull(state); i = 0; } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesTestCase.java index 6f8b554500433..d5867c8ebefcd 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesTestCase.java @@ -24,9 +24,11 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.NoSuchElementException; import java.util.function.Predicate; import java.util.stream.Stream; +import static java.util.Collections.emptyMap; import static java.util.stream.Collectors.toList; import static org.hamcrest.Matchers.equalTo; import static org.mockito.Mockito.mock; @@ -55,7 +57,7 @@ private T createTestInstance(String name, Map metadata, boolean for (int i = 0; i < numValues; ++i) { values[i] = randomDouble(); } - return createTestInstance(name, metadata, keyed, format, percents, values); + return createTestInstance(name, metadata, keyed, format, percents, values, false); } protected abstract T createTestInstance( @@ -64,7 +66,8 @@ protected abstract T createTestInstance( boolean keyed, DocValueFormat format, double[] percents, - double[] values + double[] values, + boolean empty ); protected abstract Class implementationClass(); @@ -104,7 +107,7 @@ public void testEmptyRanksXContent() throws IOException { boolean keyed = randomBoolean(); DocValueFormat docValueFormat = randomNumericDocValueFormat(); - T agg = createTestInstance("test", Collections.emptyMap(), keyed, docValueFormat, percents, new double[0]); + T agg = createTestInstance("test", Collections.emptyMap(), keyed, docValueFormat, percents, new double[0], false); for (Percentile percentile : agg) { Double value = percentile.getValue(); @@ -147,4 +150,26 @@ public void testEmptyRanksXContent() throws IOException { assertThat(Strings.toString(builder), equalTo(expected)); } + + public void testEmptyIterator() { + final double[] percents = randomPercents(false); + + final Iterable aggregation = createTestInstance( + "test", + emptyMap(), + false, + randomNumericDocValueFormat(), + percents, + new double[] {}, + true + ); + + for (var ignored : aggregation) { + fail("empty expected"); + } + + final Iterator it = aggregation.iterator(); + assertFalse(it.hasNext()); + expectThrows(NoSuchElementException.class, it::next); + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesRanksTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesRanksTests.java index 47015201f58f5..ac43929878f47 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesRanksTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesRanksTests.java @@ -30,9 +30,13 @@ protected InternalHDRPercentileRanks createTestInstance( boolean keyed, DocValueFormat format, double[] percents, - double[] values + double[] values, + boolean empty ) { + if (empty) { + return new InternalHDRPercentileRanks(name, percents, null, keyed, format, metadata); + } final DoubleHistogram state = new DoubleHistogram(3); Arrays.stream(values).forEach(state::recordValue); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesTests.java index 6e986f69c1c50..1c9cb88ee3e2b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalHDRPercentilesTests.java @@ -31,9 +31,14 @@ protected InternalHDRPercentiles createTestInstance( boolean keyed, DocValueFormat format, double[] percents, - double[] values + double[] values, + boolean empty ) { + if (empty) { + return new InternalHDRPercentiles(name, percents, null, keyed, format, metadata); + } + final DoubleHistogram state = new DoubleHistogram(3); Arrays.stream(values).forEach(state::recordValue); @@ -76,7 +81,15 @@ public void testIterator() { values[i] = randomDouble(); } - InternalHDRPercentiles aggregation = createTestInstance("test", emptyMap(), false, randomNumericDocValueFormat(), percents, values); + InternalHDRPercentiles aggregation = createTestInstance( + "test", + emptyMap(), + false, + randomNumericDocValueFormat(), + percents, + values, + false + ); Iterator iterator = aggregation.iterator(); Iterator nameIterator = aggregation.valueNames().iterator(); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesRanksTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesRanksTests.java index 27d3fafee1319..cdefca8f72eaf 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesRanksTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesRanksTests.java @@ -29,8 +29,12 @@ protected InternalTDigestPercentileRanks createTestInstance( boolean keyed, DocValueFormat format, double[] percents, - double[] values + double[] values, + boolean empty ) { + if (empty) { + return new InternalTDigestPercentileRanks(name, percents, null, keyed, format, metadata); + } final TDigestState state = new TDigestState(100); Arrays.stream(values).forEach(state::add); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesTests.java index 5c8e6aa2700c9..1df9e8152101f 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalTDigestPercentilesTests.java @@ -30,8 +30,12 @@ protected InternalTDigestPercentiles createTestInstance( boolean keyed, DocValueFormat format, double[] percents, - double[] values + double[] values, + boolean empty ) { + if (empty) { + return new InternalTDigestPercentiles(name, percents, null, keyed, format, metadata); + } final TDigestState state = new TDigestState(100); Arrays.stream(values).forEach(state::add); @@ -126,7 +130,8 @@ public void testIterator() { false, randomNumericDocValueFormat(), percents, - values + values, + false ); Iterator iterator = aggregation.iterator();