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

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 interface DocValueFormat extends NamedWriteable {
 
         @Override
         public String format(double value) {
+            /**
+             * Explicitly check for NaN, since it formats to "<EFBFBD>" 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` (<EFBFBD>)
+             *
+             * 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 <EFBFBD> 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 @@ public abstract class ParsedPercentiles extends ParsedAggregation implements Ite
             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 @@ public abstract class ParsedPercentiles extends ParsedAggregation implements Ite
                 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 @@ abstract class AbstractInternalHDRPercentiles extends InternalNumericMetricsAggr
             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 @@ abstract class AbstractInternalHDRPercentiles extends InternalNumericMetricsAggr
                 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 @@ abstract class AbstractInternalTDigestPercentiles extends InternalNumericMetrics
             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 @@ abstract class AbstractInternalTDigestPercentiles extends InternalNumericMetrics
                 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 org.elasticsearch.test.InternalAggregationTestCase;
 
 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 abstract class AbstractPercentilesTestCase<T extends InternalAggregation
 
     @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 abstract class AbstractPercentilesTestCase<T extends InternalAggregation
     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 @@ package org.elasticsearch.search.aggregations.metrics.percentiles;
 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 @@ public abstract class InternalPercentilesRanksTestCase<T extends InternalAggrega
         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 org.elasticsearch.search.aggregations.ParsedAggregation;
 
 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 abstract class InternalPercentilesTestCase<T extends InternalAggregation
         }
         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.HashMap;
 import java.util.List;
 import java.util.Map;
 
+
 public class InternalHDRPercentilesRanksTests extends InternalPercentilesRanksTestCase<InternalHDRPercentileRanks> {
 
     @Override