Skip to content

Percentiles aggregation validation checks for range #51871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 11, 2020
Merged

Percentiles aggregation validation checks for range #51871

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8e18e66
disallow duplicate percentile definitions and out of range
Feb 4, 2020
442fb22
adapt tests to avoid duplicates for percentiles
Feb 5, 2020
5f0cc8f
fix one more wrong percentile test
Feb 6, 2020
43dcf62
remove the duplication check, tbd for a separate 8.0 only PR
Feb 7, 2020
3726ea8
apply review suggestion
Feb 7, 2020
b3fe9a1
remove unit test for dups
Feb 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions ...ain/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,18 @@ public PercentilesAggregationBuilder percentiles(double... percents) {
}
double[] sortedPercents = Arrays.copyOf(percents, percents.length);
Arrays.sort(sortedPercents);
double previousPercent = -1.0;
for (double percent : sortedPercents) {
if (percent < 0.0 || percent > 100.0) {
throw new IllegalArgumentException("percent must be in [0,100], got [" + percent + "]: [" + name + "]");
}

if(percent == previousPercent) {
Copy link
Member

@nik9000 nik9000 Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should have an extra space after if and before (.

throw new IllegalArgumentException("percent [" + percent + "] has been specified twice: [" + name + "]");
}
previousPercent = percent;
}

this.percents = sortedPercents;
return this;
}
Expand Down
16 changes: 9 additions & 7 deletions server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<>();
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(HDRPercentilesIT.class).info("Using percentiles={}", Arrays.toString(percentiles));
return percentiles;
}
Expand Down
15 changes: 15 additions & 0 deletions server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentilesTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,21 @@ 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 testDuplicatePercentilesThrows() throws IOException {
PercentilesAggregationBuilder builder = new PercentilesAggregationBuilder("testAgg");
IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> builder.percentiles(5, 42, 10, 99, 42, 87));
assertEquals("percent [42.0] has been specified twice: [testAgg]", ex.getMessage());
}

public void testExceptionMultipleMethods() throws IOException {
final String illegalAgg = "{\n" +
" \"percentiles\": {\n" +
Expand Down
16 changes: 9 additions & 7 deletions server/src/test/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
5 changes: 2 additions & 3 deletions ...st/java/org/elasticsearch/xpack/analytics/mapper/HistogramPercentileAggregationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down