Percentile/Ranks should return null instead of NaN when empty

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

@@ -16,3 +16,9 @@ Cross-Cluster-Search::
 
 Rest API::
 * The Clear Cache API only supports `POST` as HTTP method
+
+Aggregations::
+* The Percentiles and PercentileRanks aggregations now return `null` in the REST response,
+ instead of `NaN`. This makes it consistent with the rest of the aggregations. Note:
+ this only applies to the REST response, the java objects continue to return `NaN` (also
+ consistent with other aggregations)

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

@@ -394,6 +394,22 @@ public String format(long value) {
 
  @Override
  public String format(double value) {
+ /**
+ * Explicitly check for NaN, since it formats to "�" or "NaN" depending on JDK version.
+ *
+ * Decimal formatter uses the JRE's default symbol list (via Locale.ROOT above). In JDK8,
+ * this translates into using {@link sun.util.locale.provider.JRELocaleProviderAdapter}, which loads
+ * {@link sun.text.resources.FormatData} for symbols. There, `NaN` is defined as `\ufffd` (�)
+ *
+ * In JDK9+, {@link sun.util.cldr.CLDRLocaleProviderAdapter} is used instead, which loads
+ * {@link sun.text.resources.cldr.FormatData}. There, `NaN` is defined as `"NaN"`
+ *
+ * Since the character � isn't very useful, and makes the output change depending on JDK version,
+ * we manually check to see if the value is NaN and return the string directly.
+ */
+ if (Double.isNaN(value)) {
+ return String.valueOf(Double.NaN);
+ }
  return format.format(value);
  }
 

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

@@ -92,9 +92,9 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params)
  builder.startObject(CommonFields.VALUES.getPreferredName());
  for (Map.Entry<Double, Double> percentile : percentiles.entrySet()) {
  Double key = percentile.getKey();
- builder.field(String.valueOf(key), percentile.getValue());
-
- if (valuesAsString) {
+ Double value = percentile.getValue();
+ builder.field(String.valueOf(key), value.isNaN() ? null : value);
+ if (valuesAsString && value.isNaN() == false) {
  builder.field(key + "_as_string", getPercentileAsString(key));
  }
  }
@@ -106,8 +106,9 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params)
  builder.startObject();
  {
  builder.field(CommonFields.KEY.getPreferredName(), key);
- builder.field(CommonFields.VALUE.getPreferredName(), percentile.getValue());
- if (valuesAsString) {
+ Double value = percentile.getValue();
+ builder.field(CommonFields.VALUE.getPreferredName(), value.isNaN() ? null : value);
+ if (valuesAsString && value.isNaN() == false) {
  builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), getPercentileAsString(key));
  }
  }

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

@@ -123,9 +123,9 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th
  for(int i = 0; i < keys.length; ++i) {
  String key = String.valueOf(keys[i]);
  double value = value(keys[i]);
- builder.field(key, value);
- if (format != DocValueFormat.RAW) {
- builder.field(key + "_as_string", format.format(value));
+ builder.field(key, state.getTotalCount() == 0 ? null : value);
+ if (format != DocValueFormat.RAW && state.getTotalCount() > 0) {
+ builder.field(key + "_as_string",  format.format(value).toString());
  }
  }
  builder.endObject();
@@ -135,8 +135,8 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th
  double value = value(keys[i]);
  builder.startObject();
  builder.field(CommonFields.KEY.getPreferredName(), keys[i]);
- builder.field(CommonFields.VALUE.getPreferredName(), value);
- if (format != DocValueFormat.RAW) {
+ builder.field(CommonFields.VALUE.getPreferredName(), state.getTotalCount() == 0 ? null : value);
+ if (format != DocValueFormat.RAW && state.getTotalCount() > 0) {
  builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), format.format(value).toString());
  }
  builder.endObject();

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

@@ -106,9 +106,9 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th
  for(int i = 0; i < keys.length; ++i) {
  String key = String.valueOf(keys[i]);
  double value = value(keys[i]);
- builder.field(key, value);
- if (format != DocValueFormat.RAW) {
- builder.field(key + "_as_string", format.format(value));
+ builder.field(key, state.size() == 0 ? null : value);
+ if (format != DocValueFormat.RAW && state.size() > 0) {
+ builder.field(key + "_as_string", format.format(value).toString());
  }
  }
  builder.endObject();
@@ -118,8 +118,8 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th
  double value = value(keys[i]);
  builder.startObject();
  builder.field(CommonFields.KEY.getPreferredName(), keys[i]);
- builder.field(CommonFields.VALUE.getPreferredName(), value);
- if (format != DocValueFormat.RAW) {
+ builder.field(CommonFields.VALUE.getPreferredName(), state.size() == 0 ? null : value);
+ if (format != DocValueFormat.RAW && state.size() > 0) {
  builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), format.format(value).toString());
  }
  builder.endObject();

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

@@ -19,6 +19,10 @@
 
 package org.elasticsearch.search.aggregations.metrics.percentiles;
 
+import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.search.DocValueFormat;
 import org.elasticsearch.search.aggregations.Aggregation.CommonFields;
 import org.elasticsearch.search.aggregations.InternalAggregation;
@@ -27,11 +31,14 @@
 
 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.function.Predicate;
 
+import static org.hamcrest.Matchers.equalTo;
+
 public abstract class AbstractPercentilesTestCase<T extends InternalAggregation & Iterable<Percentile>>
  extends InternalAggregationTestCase<T> {
 
@@ -49,7 +56,7 @@ public void setUp() throws Exception {
 
  @Override
  protected T createTestInstance(String name, List<PipelineAggregator> pipelineAggregators, Map<String, Object> metaData) {
- int numValues = randomInt(100);
+ int numValues = frequently() ? randomInt(100) : 0;
  double[] values = new double[numValues];
  for (int i = 0; i < numValues; ++i) {
  values[i] = randomDouble();
@@ -89,4 +96,53 @@ public static double[] randomPercents(boolean sorted) {
  protected Predicate<String> excludePathsFromXContentInsertion() {
  return path -> path.endsWith(CommonFields.VALUES.getPreferredName());
  }
+
+ protected abstract void assertPercentile(T agg, Double value);
+
+ public void testEmptyRanksXContent() throws IOException {
+ double[] percents = new double[]{1,2,3};
+ boolean keyed = randomBoolean();
+ DocValueFormat docValueFormat = randomNumericDocValueFormat();
+
+ T agg = createTestInstance("test", Collections.emptyList(), Collections.emptyMap(), keyed, docValueFormat, percents, new double[0]);
+
+ for (Percentile percentile : agg) {
+ Double value = percentile.getValue();
+ assertPercentile(agg, value);
+ }
+
+ XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint();
+ builder.startObject();
+ agg.doXContentBody(builder, ToXContent.EMPTY_PARAMS);
+ builder.endObject();
+ String expected;
+ if (keyed) {
+ expected = "{\n" +
+ " \"values\" : {\n" +
+ " \"1.0\" : null,\n" +
+ " \"2.0\" : null,\n" +
+ " \"3.0\" : null\n" +
+ " }\n" +
+ "}";
+ } else {
+ expected = "{\n" +
+ " \"values\" : [\n" +
+ " {\n" +
+ " \"key\" : 1.0,\n" +
+ " \"value\" : null\n" +
+ " },\n" +
+ " {\n" +
+ " \"key\" : 2.0,\n" +
+ " \"value\" : null\n" +
+ " },\n" +
+ " {\n" +
+ " \"key\" : 3.0,\n" +
+ " \"value\" : null\n" +
+ " }\n" +
+ " ]\n" +
+ "}";
+ }
+
+ assertThat(Strings.toString(builder), equalTo(expected));
+ }
 }

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

@@ -22,6 +22,8 @@
 import org.elasticsearch.search.aggregations.InternalAggregation;
 import org.elasticsearch.search.aggregations.ParsedAggregation;
 
+import static org.hamcrest.Matchers.equalTo;
+
 public abstract class InternalPercentilesRanksTestCase<T extends InternalAggregation & PercentileRanks>
  extends AbstractPercentilesTestCase<T> {
 
@@ -39,4 +41,10 @@ protected final void assertFromXContent(T aggregation, ParsedAggregation parsedA
  Class<? extends ParsedPercentiles> parsedClass = implementationClass();
  assertTrue(parsedClass != null && parsedClass.isInstance(parsedAggregation));
  }
+
+ @Override
+ protected void assertPercentile(T agg, Double value) {
+ assertThat(agg.percent(value), equalTo(Double.NaN));
+ assertThat(agg.percentAsString(value), equalTo("NaN"));
+ }
 }

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

@@ -24,6 +24,8 @@
 
 import java.util.List;
 
+import static org.hamcrest.Matchers.equalTo;
+
 public abstract class InternalPercentilesTestCase<T extends InternalAggregation & Percentiles> extends AbstractPercentilesTestCase<T> {
 
  @Override
@@ -49,4 +51,10 @@ public static double[] randomPercents() {
  }
  return percents;
  }
+
+ @Override
+ protected void assertPercentile(T agg, Double value) {
+ assertThat(agg.percentile(value), equalTo(Double.NaN));
+ assertThat(agg.percentileAsString(value), equalTo("NaN"));
+ }
 }

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

@@ -31,6 +31,7 @@
 import java.util.List;
 import java.util.Map;
 
+
 public class InternalHDRPercentilesRanksTests extends InternalPercentilesRanksTestCase<InternalHDRPercentileRanks> {
 
  @Override