Skip to content

Commit 1502812

Browse files
polyfractalpolyfractal
authored
Percentile/Ranks should return null instead of NaN when empty (#30460)
The other metric aggregations (min/max/etc) return `null` as their XContent value and string when nothing was computed (due to empty/missing fields). Percentiles and Percentile Ranks, however, return `NaN `which is inconsistent and confusing for the user. This fixes the inconsistency by making the aggs return `null`. This applies to both the numeric value and the "as string" value. Note: like the metric aggs, this does not change the value if fetched directly from the percentiles object, which will return as `NaN`/`"NaN"`. This only changes the XContent output. While this is a bugfix, it still breaks bwc in a minor way as the response changes from prior version. Closes #29066
1 parent c4f8df3 commit 1502812

File tree

9 files changed

+112
-16
lines changed

9 files changed

+112
-16
lines changed

‎docs/reference/release-notes/7.0.0-alpha1.asciidoc

+6
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,9 @@ Cross-Cluster-Search::
1616

1717
Rest API::
1818
* The Clear Cache API only supports `POST` as HTTP method
19+
20+
Aggregations::
21+
* The Percentiles and PercentileRanks aggregations now return `null` in the REST response,
22+
instead of `NaN`. This makes it consistent with the rest of the aggregations. Note:
23+
this only applies to the REST response, the java objects continue to return `NaN` (also
24+
consistent with other aggregations)

‎server/src/main/java/org/elasticsearch/search/DocValueFormat.java

+16
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,22 @@ public String format(long value) {
394394

395395
@Override
396396
public String format(double value) {
397+
/**
398+
* Explicitly check for NaN, since it formats to "�" or "NaN" depending on JDK version.
399+
*
400+
* Decimal formatter uses the JRE's default symbol list (via Locale.ROOT above). In JDK8,
401+
* this translates into using {@link sun.util.locale.provider.JRELocaleProviderAdapter}, which loads
402+
* {@link sun.text.resources.FormatData} for symbols. There, `NaN` is defined as `\ufffd` (�)
403+
*
404+
* In JDK9+, {@link sun.util.cldr.CLDRLocaleProviderAdapter} is used instead, which loads
405+
* {@link sun.text.resources.cldr.FormatData}. There, `NaN` is defined as `"NaN"`
406+
*
407+
* Since the character � isn't very useful, and makes the output change depending on JDK version,
408+
* we manually check to see if the value is NaN and return the string directly.
409+
*/
410+
if (Double.isNaN(value)) {
411+
return String.valueOf(Double.NaN);
412+
}
397413
return format.format(value);
398414
}
399415

‎server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/ParsedPercentiles.java

+6-5
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,9 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params)
9292
builder.startObject(CommonFields.VALUES.getPreferredName());
9393
for (Map.Entry<Double, Double> percentile : percentiles.entrySet()) {
9494
Double key = percentile.getKey();
95-
builder.field(String.valueOf(key), percentile.getValue());
96-
97-
if (valuesAsString) {
95+
Double value = percentile.getValue();
96+
builder.field(String.valueOf(key), value.isNaN() ? null : value);
97+
if (valuesAsString && value.isNaN() == false) {
9898
builder.field(key + "_as_string", getPercentileAsString(key));
9999
}
100100
}
@@ -106,8 +106,9 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params)
106106
builder.startObject();
107107
{
108108
builder.field(CommonFields.KEY.getPreferredName(), key);
109-
builder.field(CommonFields.VALUE.getPreferredName(), percentile.getValue());
110-
if (valuesAsString) {
109+
Double value = percentile.getValue();
110+
builder.field(CommonFields.VALUE.getPreferredName(), value.isNaN() ? null : value);
111+
if (valuesAsString && value.isNaN() == false) {
111112
builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), getPercentileAsString(key));
112113
}
113114
}

‎server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/AbstractInternalHDRPercentiles.java

+5-5
Original file line numberDiff line numberDiff line change
@@ -123,9 +123,9 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th
123123
for(int i = 0; i < keys.length; ++i) {
124124
String key = String.valueOf(keys[i]);
125125
double value = value(keys[i]);
126-
builder.field(key, value);
127-
if (format != DocValueFormat.RAW) {
128-
builder.field(key + "_as_string", format.format(value));
126+
builder.field(key, state.getTotalCount() == 0 ? null : value);
127+
if (format != DocValueFormat.RAW && state.getTotalCount() > 0) {
128+
builder.field(key + "_as_string", format.format(value).toString());
129129
}
130130
}
131131
builder.endObject();
@@ -135,8 +135,8 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th
135135
double value = value(keys[i]);
136136
builder.startObject();
137137
builder.field(CommonFields.KEY.getPreferredName(), keys[i]);
138-
builder.field(CommonFields.VALUE.getPreferredName(), value);
139-
if (format != DocValueFormat.RAW) {
138+
builder.field(CommonFields.VALUE.getPreferredName(), state.getTotalCount() == 0 ? null : value);
139+
if (format != DocValueFormat.RAW && state.getTotalCount() > 0) {
140140
builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), format.format(value).toString());
141141
}
142142
builder.endObject();

‎server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/AbstractInternalTDigestPercentiles.java

+5-5
Original file line numberDiff line numberDiff line change
@@ -106,9 +106,9 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th
106106
for(int i = 0; i < keys.length; ++i) {
107107
String key = String.valueOf(keys[i]);
108108
double value = value(keys[i]);
109-
builder.field(key, value);
110-
if (format != DocValueFormat.RAW) {
111-
builder.field(key + "_as_string", format.format(value));
109+
builder.field(key, state.size() == 0 ? null : value);
110+
if (format != DocValueFormat.RAW && state.size() > 0) {
111+
builder.field(key + "_as_string", format.format(value).toString());
112112
}
113113
}
114114
builder.endObject();
@@ -118,8 +118,8 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th
118118
double value = value(keys[i]);
119119
builder.startObject();
120120
builder.field(CommonFields.KEY.getPreferredName(), keys[i]);
121-
builder.field(CommonFields.VALUE.getPreferredName(), value);
122-
if (format != DocValueFormat.RAW) {
121+
builder.field(CommonFields.VALUE.getPreferredName(), state.size() == 0 ? null : value);
122+
if (format != DocValueFormat.RAW && state.size() > 0) {
123123
builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), format.format(value).toString());
124124
}
125125
builder.endObject();

‎server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/AbstractPercentilesTestCase.java

+57-1
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,10 @@
1919

2020
package org.elasticsearch.search.aggregations.metrics.percentiles;
2121

22+
import org.elasticsearch.common.Strings;
23+
import org.elasticsearch.common.xcontent.ToXContent;
24+
import org.elasticsearch.common.xcontent.XContentBuilder;
25+
import org.elasticsearch.common.xcontent.json.JsonXContent;
2226
import org.elasticsearch.search.DocValueFormat;
2327
import org.elasticsearch.search.aggregations.Aggregation.CommonFields;
2428
import org.elasticsearch.search.aggregations.InternalAggregation;
@@ -27,11 +31,14 @@
2731

2832
import java.io.IOException;
2933
import java.util.Arrays;
34+
import java.util.Collections;
3035
import java.util.Iterator;
3136
import java.util.List;
3237
import java.util.Map;
3338
import java.util.function.Predicate;
3439

40+
import static org.hamcrest.Matchers.equalTo;
41+
3542
public abstract class AbstractPercentilesTestCase<T extends InternalAggregation & Iterable<Percentile>>
3643
extends InternalAggregationTestCase<T> {
3744

@@ -49,7 +56,7 @@ public void setUp() throws Exception {
4956

5057
@Override
5158
protected T createTestInstance(String name, List<PipelineAggregator> pipelineAggregators, Map<String, Object> metaData) {
52-
int numValues = randomInt(100);
59+
int numValues = frequently() ? randomInt(100) : 0;
5360
double[] values = new double[numValues];
5461
for (int i = 0; i < numValues; ++i) {
5562
values[i] = randomDouble();
@@ -89,4 +96,53 @@ public static double[] randomPercents(boolean sorted) {
8996
protected Predicate<String> excludePathsFromXContentInsertion() {
9097
return path -> path.endsWith(CommonFields.VALUES.getPreferredName());
9198
}
99+
100+
protected abstract void assertPercentile(T agg, Double value);
101+
102+
public void testEmptyRanksXContent() throws IOException {
103+
double[] percents = new double[]{1,2,3};
104+
boolean keyed = randomBoolean();
105+
DocValueFormat docValueFormat = randomNumericDocValueFormat();
106+
107+
T agg = createTestInstance("test", Collections.emptyList(), Collections.emptyMap(), keyed, docValueFormat, percents, new double[0]);
108+
109+
for (Percentile percentile : agg) {
110+
Double value = percentile.getValue();
111+
assertPercentile(agg, value);
112+
}
113+
114+
XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint();
115+
builder.startObject();
116+
agg.doXContentBody(builder, ToXContent.EMPTY_PARAMS);
117+
builder.endObject();
118+
String expected;
119+
if (keyed) {
120+
expected = "{\n" +
121+
" \"values\" : {\n" +
122+
" \"1.0\" : null,\n" +
123+
" \"2.0\" : null,\n" +
124+
" \"3.0\" : null\n" +
125+
" }\n" +
126+
"}";
127+
} else {
128+
expected = "{\n" +
129+
" \"values\" : [\n" +
130+
" {\n" +
131+
" \"key\" : 1.0,\n" +
132+
" \"value\" : null\n" +
133+
" },\n" +
134+
" {\n" +
135+
" \"key\" : 2.0,\n" +
136+
" \"value\" : null\n" +
137+
" },\n" +
138+
" {\n" +
139+
" \"key\" : 3.0,\n" +
140+
" \"value\" : null\n" +
141+
" }\n" +
142+
" ]\n" +
143+
"}";
144+
}
145+
146+
assertThat(Strings.toString(builder), equalTo(expected));
147+
}
92148
}

‎server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java

+8
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
import org.elasticsearch.search.aggregations.InternalAggregation;
2323
import org.elasticsearch.search.aggregations.ParsedAggregation;
2424

25+
import static org.hamcrest.Matchers.equalTo;
26+
2527
public abstract class InternalPercentilesRanksTestCase<T extends InternalAggregation & PercentileRanks>
2628
extends AbstractPercentilesTestCase<T> {
2729

@@ -39,4 +41,10 @@ protected final void assertFromXContent(T aggregation, ParsedAggregation parsedA
3941
Class<? extends ParsedPercentiles> parsedClass = implementationClass();
4042
assertTrue(parsedClass != null && parsedClass.isInstance(parsedAggregation));
4143
}
44+
45+
@Override
46+
protected void assertPercentile(T agg, Double value) {
47+
assertThat(agg.percent(value), equalTo(Double.NaN));
48+
assertThat(agg.percentAsString(value), equalTo("NaN"));
49+
}
4250
}

‎server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesTestCase.java

+8
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424

2525
import java.util.List;
2626

27+
import static org.hamcrest.Matchers.equalTo;
28+
2729
public abstract class InternalPercentilesTestCase<T extends InternalAggregation & Percentiles> extends AbstractPercentilesTestCase<T> {
2830

2931
@Override
@@ -49,4 +51,10 @@ public static double[] randomPercents() {
4951
}
5052
return percents;
5153
}
54+
55+
@Override
56+
protected void assertPercentile(T agg, Double value) {
57+
assertThat(agg.percentile(value), equalTo(Double.NaN));
58+
assertThat(agg.percentileAsString(value), equalTo("NaN"));
59+
}
5260
}

‎server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesRanksTests.java

+1
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import java.util.List;
3232
import java.util.Map;
3333

34+
3435
public class InternalHDRPercentilesRanksTests extends InternalPercentilesRanksTestCase<InternalHDRPercentileRanks> {
3536

3637
@Override

0 commit comments

Comments
 (0)