From b7a7f7f8e850acfe5f778522874b2354c227a518 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Mon, 7 May 2018 21:42:47 +0000 Subject: [PATCH 1/6] Percentile/Ranks should return null instead of NaN when empty 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 value and the string getters. Note: like the metric aggs, this does not change the value if fetched directly from the percentiles object it will return as `NaN`/`"NaN"`. This only changes the XContent output. --- .../hdr/AbstractInternalHDRPercentiles.java | 9 +- .../AbstractInternalTDigestPercentiles.java | 8 +- .../hdr/InternalHDRPercentilesRanksTests.java | 91 +++++++++++++++++++ .../hdr/InternalHDRPercentilesTests.java | 87 ++++++++++++++++++ .../InternalTDigestPercentilesRanksTests.java | 90 ++++++++++++++++++ .../InternalTDigestPercentilesTests.java | 90 ++++++++++++++++++ 6 files changed, 367 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/AbstractInternalHDRPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/AbstractInternalHDRPercentiles.java index 2ff3e6d3becb2..652a6d95f0251 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/AbstractInternalHDRPercentiles.java +++ b/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); + builder.field(key, state.getTotalCount() == 0 ? null : value); if (format != DocValueFormat.RAW) { - builder.field(key + "_as_string", format.format(value)); + builder.field(key + "_as_string", state.getTotalCount() == 0 ? null : format.format(value)); } } builder.endObject(); @@ -135,9 +135,10 @@ 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); + builder.field(CommonFields.VALUE.getPreferredName(), state.getTotalCount() == 0 ? null : value); if (format != DocValueFormat.RAW) { - builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), format.format(value)); + builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), + state.getTotalCount() == 0 ? null : format.format(value)); } builder.endObject(); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/AbstractInternalTDigestPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/AbstractInternalTDigestPercentiles.java index 143775b5bfd15..8fa1677be6856 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/AbstractInternalTDigestPercentiles.java +++ b/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); + builder.field(key, state.size() == 0 ? null : value); if (format != DocValueFormat.RAW) { - builder.field(key + "_as_string", format.format(value)); + builder.field(key + "_as_string", state.size() == 0 ? null : format.format(value)); } } builder.endObject(); @@ -118,9 +118,9 @@ 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); + builder.field(CommonFields.VALUE.getPreferredName(), state.size() == 0 ? null : value); if (format != DocValueFormat.RAW) { - builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), format.format(value)); + builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), state.size() == 0 ? null : format.format(value)); } builder.endObject(); } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesRanksTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesRanksTests.java index dcbd5cdbd5a3a..62cec4255809a 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesRanksTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesRanksTests.java @@ -20,17 +20,27 @@ package org.elasticsearch.search.aggregations.metrics.percentiles.hdr; import org.HdrHistogram.DoubleHistogram; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.Writeable.Reader; +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.metrics.percentiles.InternalPercentilesRanksTestCase; import org.elasticsearch.search.aggregations.metrics.percentiles.ParsedPercentiles; +import org.elasticsearch.search.aggregations.metrics.percentiles.Percentile; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import java.io.IOException; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import static org.hamcrest.Matchers.equalTo; + + public class InternalHDRPercentilesRanksTests extends InternalPercentilesRanksTestCase { @Override @@ -103,4 +113,85 @@ protected InternalHDRPercentileRanks mutateInstance(InternalHDRPercentileRanks i } return new InternalHDRPercentileRanks(name, percents, state, keyed, formatter, pipelineAggregators, metaData); } + + public void testEmptyRanksXContent() throws IOException { + double[] percents = new double[]{1,2,3}; + boolean keyed = randomBoolean(); + DocValueFormat docValueFormat = randomNumericDocValueFormat(); + + final DoubleHistogram state = new DoubleHistogram(3); + InternalHDRPercentileRanks agg = new InternalHDRPercentileRanks("test", percents, state, keyed, docValueFormat, + Collections.emptyList(), Collections.emptyMap()); + + for (Percentile percentile : agg) { + Double value = percentile.getValue(); + assertThat(agg.percent(value), equalTo(Double.NaN)); + assertThat(agg.percentAsString(value), equalTo("NaN")); + } + + XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint(); + builder.startObject(); + agg.doXContentBody(builder, ToXContent.EMPTY_PARAMS); + builder.endObject(); + String expected; + if (keyed && docValueFormat.equals(DocValueFormat.RAW)) { + expected = "{\n" + + " \"values\" : {\n" + + " \"1.0\" : null,\n" + + " \"2.0\" : null,\n" + + " \"3.0\" : null\n" + + " }\n" + + "}"; + } else if (keyed) { + expected = "{\n" + + " \"values\" : {\n" + + " \"1.0\" : null,\n" + + " \"1.0_as_string\" : null,\n" + + " \"2.0\" : null,\n" + + " \"2.0_as_string\" : null,\n" + + " \"3.0\" : null,\n" + + " \"3.0_as_string\" : null\n" + + " }\n" + + "}"; + } else if (keyed == false && docValueFormat.equals(DocValueFormat.RAW)) { + 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" + + "}"; + } else { + expected = "{\n" + + " \"values\" : [\n" + + " {\n" + + " \"key\" : 1.0,\n" + + " \"value\" : null,\n" + + " \"value_as_string\" : null\n" + + " },\n" + + " {\n" + + " \"key\" : 2.0,\n" + + " \"value\" : null,\n" + + " \"value_as_string\" : null\n" + + " },\n" + + " {\n" + + " \"key\" : 3.0,\n" + + " \"value\" : null,\n" + + " \"value_as_string\" : null\n" + + " }\n" + + " ]\n" + + "}"; + } + + assertThat(Strings.toString(builder), equalTo(expected)); + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesTests.java index 7f1362af04108..35932ea4fc338 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesTests.java @@ -20,14 +20,20 @@ package org.elasticsearch.search.aggregations.metrics.percentiles.hdr; import org.HdrHistogram.DoubleHistogram; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.Writeable; +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.metrics.percentiles.InternalPercentilesTestCase; import org.elasticsearch.search.aggregations.metrics.percentiles.ParsedPercentiles; import org.elasticsearch.search.aggregations.metrics.percentiles.Percentile; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import java.io.IOException; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.List; @@ -35,6 +41,7 @@ import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; +import static org.hamcrest.Matchers.equalTo; public class InternalHDRPercentilesTests extends InternalPercentilesTestCase { @@ -130,4 +137,84 @@ protected InternalHDRPercentiles mutateInstance(InternalHDRPercentiles instance) } return new InternalHDRPercentiles(name, percents, state, keyed, formatter, pipelineAggregators, metaData); } + + public void testEmptyPercentilesXContent() throws IOException { + double[] percents = new double[]{1,2,3}; + boolean keyed = randomBoolean(); + DocValueFormat docValueFormat = randomNumericDocValueFormat(); + + final DoubleHistogram state = new DoubleHistogram(3); + InternalHDRPercentiles agg = new InternalHDRPercentiles("test", percents, state, keyed, docValueFormat, + Collections.emptyList(), Collections.emptyMap()); + + for (Percentile percentile : agg) { + Double value = percentile.getValue(); + assertThat(agg.percentile(value), equalTo(Double.NaN)); + assertThat(agg.percentileAsString(value), equalTo("NaN")); + } + + XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint(); + builder.startObject(); + agg.doXContentBody(builder, ToXContent.EMPTY_PARAMS); + builder.endObject(); + String expected; + if (keyed && docValueFormat.equals(DocValueFormat.RAW)) { + expected = "{\n" + + " \"values\" : {\n" + + " \"1.0\" : null,\n" + + " \"2.0\" : null,\n" + + " \"3.0\" : null\n" + + " }\n" + + "}"; + } else if (keyed) { + expected = "{\n" + + " \"values\" : {\n" + + " \"1.0\" : null,\n" + + " \"1.0_as_string\" : null,\n" + + " \"2.0\" : null,\n" + + " \"2.0_as_string\" : null,\n" + + " \"3.0\" : null,\n" + + " \"3.0_as_string\" : null\n" + + " }\n" + + "}"; + } else if (keyed == false && docValueFormat.equals(DocValueFormat.RAW)) { + 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" + + "}"; + } else { + expected = "{\n" + + " \"values\" : [\n" + + " {\n" + + " \"key\" : 1.0,\n" + + " \"value\" : null,\n" + + " \"value_as_string\" : null\n" + + " },\n" + + " {\n" + + " \"key\" : 2.0,\n" + + " \"value\" : null,\n" + + " \"value_as_string\" : null\n" + + " },\n" + + " {\n" + + " \"key\" : 3.0,\n" + + " \"value\" : null,\n" + + " \"value_as_string\" : null\n" + + " }\n" + + " ]\n" + + "}"; + } + assertThat(Strings.toString(builder), equalTo(expected)); + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesRanksTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesRanksTests.java index 35c566c2e80cf..7f784533b082e 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesRanksTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesRanksTests.java @@ -19,17 +19,26 @@ package org.elasticsearch.search.aggregations.metrics.percentiles.tdigest; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.Writeable.Reader; +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.metrics.percentiles.InternalPercentilesRanksTestCase; import org.elasticsearch.search.aggregations.metrics.percentiles.ParsedPercentiles; +import org.elasticsearch.search.aggregations.metrics.percentiles.Percentile; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import java.io.IOException; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import static org.hamcrest.Matchers.equalTo; + public class InternalTDigestPercentilesRanksTests extends InternalPercentilesRanksTestCase { @Override @@ -118,4 +127,85 @@ protected InternalTDigestPercentileRanks mutateInstance(InternalTDigestPercentil } return new InternalTDigestPercentileRanks(name, percents, state, keyed, formatter, pipelineAggregators, metaData); } + + public void testEmptyRanksXContent() throws IOException { + double[] percents = new double[]{1,2,3}; + boolean keyed = randomBoolean(); + DocValueFormat docValueFormat = randomNumericDocValueFormat(); + + final TDigestState state = new TDigestState(100); + InternalTDigestPercentileRanks agg = new InternalTDigestPercentileRanks("test", percents, state, keyed, docValueFormat, + Collections.emptyList(), Collections.emptyMap()); + + for (Percentile percentile : agg) { + Double value = percentile.getValue(); + assertThat(agg.percent(value), equalTo(Double.NaN)); + assertThat(agg.percentAsString(value), equalTo("NaN")); + } + + + XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint(); + builder.startObject(); + agg.doXContentBody(builder, ToXContent.EMPTY_PARAMS); + builder.endObject(); + String expected; + if (keyed && docValueFormat.equals(DocValueFormat.RAW)) { + expected = "{\n" + + " \"values\" : {\n" + + " \"1.0\" : null,\n" + + " \"2.0\" : null,\n" + + " \"3.0\" : null\n" + + " }\n" + + "}"; + } else if (keyed) { + expected = "{\n" + + " \"values\" : {\n" + + " \"1.0\" : null,\n" + + " \"1.0_as_string\" : null,\n" + + " \"2.0\" : null,\n" + + " \"2.0_as_string\" : null,\n" + + " \"3.0\" : null,\n" + + " \"3.0_as_string\" : null\n" + + " }\n" + + "}"; + } else if (keyed == false && docValueFormat.equals(DocValueFormat.RAW)) { + 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" + + "}"; + } else { + expected = "{\n" + + " \"values\" : [\n" + + " {\n" + + " \"key\" : 1.0,\n" + + " \"value\" : null,\n" + + " \"value_as_string\" : null\n" + + " },\n" + + " {\n" + + " \"key\" : 2.0,\n" + + " \"value\" : null,\n" + + " \"value_as_string\" : null\n" + + " },\n" + + " {\n" + + " \"key\" : 3.0,\n" + + " \"value\" : null,\n" + + " \"value_as_string\" : null\n" + + " }\n" + + " ]\n" + + "}"; + } + assertThat(Strings.toString(builder), equalTo(expected)); + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesTests.java index 73c9b8a16084e..cf3c44bf3da7f 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesTests.java @@ -19,17 +19,26 @@ package org.elasticsearch.search.aggregations.metrics.percentiles.tdigest; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.Writeable; +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.metrics.percentiles.InternalPercentilesTestCase; import org.elasticsearch.search.aggregations.metrics.percentiles.ParsedPercentiles; +import org.elasticsearch.search.aggregations.metrics.percentiles.Percentile; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import java.io.IOException; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import static org.hamcrest.Matchers.equalTo; + public class InternalTDigestPercentilesTests extends InternalPercentilesTestCase { @Override @@ -114,4 +123,85 @@ protected InternalTDigestPercentiles mutateInstance(InternalTDigestPercentiles i } return new InternalTDigestPercentiles(name, percents, state, keyed, formatter, pipelineAggregators, metaData); } + + public void testEmptyPercentilesXContent() throws IOException { + double[] percents = new double[]{1,2,3}; + boolean keyed = randomBoolean(); + DocValueFormat docValueFormat = randomNumericDocValueFormat(); + + final TDigestState state = new TDigestState(100); + InternalTDigestPercentiles agg = new InternalTDigestPercentiles("test", percents, state, keyed, docValueFormat, + Collections.emptyList(), Collections.emptyMap()); + + for (Percentile percentile : agg) { + Double value = percentile.getValue(); + assertThat(agg.percentile(value), equalTo(Double.NaN)); + assertThat(agg.percentileAsString(value), equalTo("NaN")); + } + + + XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint(); + builder.startObject(); + agg.doXContentBody(builder, ToXContent.EMPTY_PARAMS); + builder.endObject(); + String expected; + if (keyed && docValueFormat.equals(DocValueFormat.RAW)) { + expected = "{\n" + + " \"values\" : {\n" + + " \"1.0\" : null,\n" + + " \"2.0\" : null,\n" + + " \"3.0\" : null\n" + + " }\n" + + "}"; + } else if (keyed) { + expected = "{\n" + + " \"values\" : {\n" + + " \"1.0\" : null,\n" + + " \"1.0_as_string\" : null,\n" + + " \"2.0\" : null,\n" + + " \"2.0_as_string\" : null,\n" + + " \"3.0\" : null,\n" + + " \"3.0_as_string\" : null\n" + + " }\n" + + "}"; + } else if (keyed == false && docValueFormat.equals(DocValueFormat.RAW)) { + 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" + + "}"; + } else { + expected = "{\n" + + " \"values\" : [\n" + + " {\n" + + " \"key\" : 1.0,\n" + + " \"value\" : null,\n" + + " \"value_as_string\" : null\n" + + " },\n" + + " {\n" + + " \"key\" : 2.0,\n" + + " \"value\" : null,\n" + + " \"value_as_string\" : null\n" + + " },\n" + + " {\n" + + " \"key\" : 3.0,\n" + + " \"value\" : null,\n" + + " \"value_as_string\" : null\n" + + " }\n" + + " ]\n" + + "}"; + } + assertThat(Strings.toString(builder), equalTo(expected)); + } } From fc346560abcaa1929ed1245dfb592c0faf7a6f6f Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Tue, 15 May 2018 17:40:29 +0000 Subject: [PATCH 2/6] Centralize empty test, test xcontent roundtrip with empty occasionally --- .../percentiles/ParsedPercentiles.java | 11 +-- .../hdr/AbstractInternalHDRPercentiles.java | 9 +- .../AbstractInternalTDigestPercentiles.java | 8 +- .../AbstractPercentilesTestCase.java | 2 +- .../InternalPercentilesRanksTestCase.java | 58 ++++++++++++ .../InternalPercentilesTestCase.java | 56 ++++++++++++ .../hdr/InternalHDRPercentilesRanksTests.java | 90 ------------------- .../hdr/InternalHDRPercentilesTests.java | 87 ------------------ .../InternalTDigestPercentilesRanksTests.java | 90 ------------------- .../InternalTDigestPercentilesTests.java | 90 ------------------- 10 files changed, 129 insertions(+), 372 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/ParsedPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/ParsedPercentiles.java index 3f56b21dcd8a0..2c7da76446d5a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/ParsedPercentiles.java +++ b/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 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)); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/AbstractInternalHDRPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/AbstractInternalHDRPercentiles.java index 652a6d95f0251..17b6c868b0a55 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/AbstractInternalHDRPercentiles.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/AbstractInternalHDRPercentiles.java @@ -124,8 +124,8 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th String key = String.valueOf(keys[i]); double value = value(keys[i]); builder.field(key, state.getTotalCount() == 0 ? null : value); - if (format != DocValueFormat.RAW) { - builder.field(key + "_as_string", state.getTotalCount() == 0 ? null : format.format(value)); + if (format != DocValueFormat.RAW && state.getTotalCount() > 0) { + builder.field(key + "_as_string", format.format(value)); } } builder.endObject(); @@ -136,9 +136,8 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th builder.startObject(); builder.field(CommonFields.KEY.getPreferredName(), keys[i]); builder.field(CommonFields.VALUE.getPreferredName(), state.getTotalCount() == 0 ? null : value); - if (format != DocValueFormat.RAW) { - builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), - state.getTotalCount() == 0 ? null : format.format(value)); + if (format != DocValueFormat.RAW && state.getTotalCount() > 0) { + builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), format.format(value)); } builder.endObject(); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/AbstractInternalTDigestPercentiles.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/AbstractInternalTDigestPercentiles.java index 8fa1677be6856..559b05a2cfdf2 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/AbstractInternalTDigestPercentiles.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/AbstractInternalTDigestPercentiles.java @@ -107,8 +107,8 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th String key = String.valueOf(keys[i]); double value = value(keys[i]); builder.field(key, state.size() == 0 ? null : value); - if (format != DocValueFormat.RAW) { - builder.field(key + "_as_string", state.size() == 0 ? null : format.format(value)); + if (format != DocValueFormat.RAW && state.size() > 0) { + builder.field(key + "_as_string", format.format(value)); } } builder.endObject(); @@ -119,8 +119,8 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th builder.startObject(); builder.field(CommonFields.KEY.getPreferredName(), keys[i]); builder.field(CommonFields.VALUE.getPreferredName(), state.size() == 0 ? null : value); - if (format != DocValueFormat.RAW) { - builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), state.size() == 0 ? null : format.format(value)); + if (format != DocValueFormat.RAW && state.size() > 0) { + builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), format.format(value)); } builder.endObject(); } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/AbstractPercentilesTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/AbstractPercentilesTestCase.java index e54a2a8b9a14f..233b8f940fd90 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/AbstractPercentilesTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/AbstractPercentilesTestCase.java @@ -49,7 +49,7 @@ public void setUp() throws Exception { @Override protected T createTestInstance(String name, List pipelineAggregators, Map 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(); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java index f45b7cce51e37..1ddd3d436d734 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java @@ -19,9 +19,19 @@ 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.InternalAggregation; import org.elasticsearch.search.aggregations.ParsedAggregation; +import java.io.IOException; +import java.util.Collections; + +import static org.hamcrest.Matchers.equalTo; + public abstract class InternalPercentilesRanksTestCase extends AbstractPercentilesTestCase { @@ -39,4 +49,52 @@ protected final void assertFromXContent(T aggregation, ParsedAggregation parsedA Class parsedClass = implementationClass(); assertTrue(parsedClass != null && parsedClass.isInstance(parsedAggregation)); } + + 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(); + assertThat(agg.percent(value), equalTo(Double.NaN)); + assertThat(agg.percentAsString(value), equalTo("NaN")); + } + + 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)); + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesTestCase.java index be105f2af80b6..927970a3b8e45 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesTestCase.java @@ -19,11 +19,20 @@ 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.InternalAggregation; import org.elasticsearch.search.aggregations.ParsedAggregation; +import java.io.IOException; +import java.util.Collections; import java.util.List; +import static org.hamcrest.Matchers.equalTo; + public abstract class InternalPercentilesTestCase extends AbstractPercentilesTestCase { @Override @@ -49,4 +58,51 @@ public static double[] randomPercents() { } return percents; } + + public void testEmptyPercentilesXContent() 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(); + assertThat(agg.percentile(value), equalTo(Double.NaN)); + assertThat(agg.percentileAsString(value), equalTo("NaN")); + } + + 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)); + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesRanksTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesRanksTests.java index 62cec4255809a..ee0e3602f2039 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesRanksTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesRanksTests.java @@ -20,26 +20,17 @@ package org.elasticsearch.search.aggregations.metrics.percentiles.hdr; import org.HdrHistogram.DoubleHistogram; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.Writeable.Reader; -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.metrics.percentiles.InternalPercentilesRanksTestCase; import org.elasticsearch.search.aggregations.metrics.percentiles.ParsedPercentiles; -import org.elasticsearch.search.aggregations.metrics.percentiles.Percentile; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; -import java.io.IOException; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import static org.hamcrest.Matchers.equalTo; - public class InternalHDRPercentilesRanksTests extends InternalPercentilesRanksTestCase { @@ -113,85 +104,4 @@ protected InternalHDRPercentileRanks mutateInstance(InternalHDRPercentileRanks i } return new InternalHDRPercentileRanks(name, percents, state, keyed, formatter, pipelineAggregators, metaData); } - - public void testEmptyRanksXContent() throws IOException { - double[] percents = new double[]{1,2,3}; - boolean keyed = randomBoolean(); - DocValueFormat docValueFormat = randomNumericDocValueFormat(); - - final DoubleHistogram state = new DoubleHistogram(3); - InternalHDRPercentileRanks agg = new InternalHDRPercentileRanks("test", percents, state, keyed, docValueFormat, - Collections.emptyList(), Collections.emptyMap()); - - for (Percentile percentile : agg) { - Double value = percentile.getValue(); - assertThat(agg.percent(value), equalTo(Double.NaN)); - assertThat(agg.percentAsString(value), equalTo("NaN")); - } - - XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint(); - builder.startObject(); - agg.doXContentBody(builder, ToXContent.EMPTY_PARAMS); - builder.endObject(); - String expected; - if (keyed && docValueFormat.equals(DocValueFormat.RAW)) { - expected = "{\n" + - " \"values\" : {\n" + - " \"1.0\" : null,\n" + - " \"2.0\" : null,\n" + - " \"3.0\" : null\n" + - " }\n" + - "}"; - } else if (keyed) { - expected = "{\n" + - " \"values\" : {\n" + - " \"1.0\" : null,\n" + - " \"1.0_as_string\" : null,\n" + - " \"2.0\" : null,\n" + - " \"2.0_as_string\" : null,\n" + - " \"3.0\" : null,\n" + - " \"3.0_as_string\" : null\n" + - " }\n" + - "}"; - } else if (keyed == false && docValueFormat.equals(DocValueFormat.RAW)) { - 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" + - "}"; - } else { - expected = "{\n" + - " \"values\" : [\n" + - " {\n" + - " \"key\" : 1.0,\n" + - " \"value\" : null,\n" + - " \"value_as_string\" : null\n" + - " },\n" + - " {\n" + - " \"key\" : 2.0,\n" + - " \"value\" : null,\n" + - " \"value_as_string\" : null\n" + - " },\n" + - " {\n" + - " \"key\" : 3.0,\n" + - " \"value\" : null,\n" + - " \"value_as_string\" : null\n" + - " }\n" + - " ]\n" + - "}"; - } - - assertThat(Strings.toString(builder), equalTo(expected)); - } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesTests.java index 35932ea4fc338..7f1362af04108 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesTests.java @@ -20,20 +20,14 @@ package org.elasticsearch.search.aggregations.metrics.percentiles.hdr; import org.HdrHistogram.DoubleHistogram; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.Writeable; -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.metrics.percentiles.InternalPercentilesTestCase; import org.elasticsearch.search.aggregations.metrics.percentiles.ParsedPercentiles; import org.elasticsearch.search.aggregations.metrics.percentiles.Percentile; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; -import java.io.IOException; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.List; @@ -41,7 +35,6 @@ import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; -import static org.hamcrest.Matchers.equalTo; public class InternalHDRPercentilesTests extends InternalPercentilesTestCase { @@ -137,84 +130,4 @@ protected InternalHDRPercentiles mutateInstance(InternalHDRPercentiles instance) } return new InternalHDRPercentiles(name, percents, state, keyed, formatter, pipelineAggregators, metaData); } - - public void testEmptyPercentilesXContent() throws IOException { - double[] percents = new double[]{1,2,3}; - boolean keyed = randomBoolean(); - DocValueFormat docValueFormat = randomNumericDocValueFormat(); - - final DoubleHistogram state = new DoubleHistogram(3); - InternalHDRPercentiles agg = new InternalHDRPercentiles("test", percents, state, keyed, docValueFormat, - Collections.emptyList(), Collections.emptyMap()); - - for (Percentile percentile : agg) { - Double value = percentile.getValue(); - assertThat(agg.percentile(value), equalTo(Double.NaN)); - assertThat(agg.percentileAsString(value), equalTo("NaN")); - } - - XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint(); - builder.startObject(); - agg.doXContentBody(builder, ToXContent.EMPTY_PARAMS); - builder.endObject(); - String expected; - if (keyed && docValueFormat.equals(DocValueFormat.RAW)) { - expected = "{\n" + - " \"values\" : {\n" + - " \"1.0\" : null,\n" + - " \"2.0\" : null,\n" + - " \"3.0\" : null\n" + - " }\n" + - "}"; - } else if (keyed) { - expected = "{\n" + - " \"values\" : {\n" + - " \"1.0\" : null,\n" + - " \"1.0_as_string\" : null,\n" + - " \"2.0\" : null,\n" + - " \"2.0_as_string\" : null,\n" + - " \"3.0\" : null,\n" + - " \"3.0_as_string\" : null\n" + - " }\n" + - "}"; - } else if (keyed == false && docValueFormat.equals(DocValueFormat.RAW)) { - 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" + - "}"; - } else { - expected = "{\n" + - " \"values\" : [\n" + - " {\n" + - " \"key\" : 1.0,\n" + - " \"value\" : null,\n" + - " \"value_as_string\" : null\n" + - " },\n" + - " {\n" + - " \"key\" : 2.0,\n" + - " \"value\" : null,\n" + - " \"value_as_string\" : null\n" + - " },\n" + - " {\n" + - " \"key\" : 3.0,\n" + - " \"value\" : null,\n" + - " \"value_as_string\" : null\n" + - " }\n" + - " ]\n" + - "}"; - } - assertThat(Strings.toString(builder), equalTo(expected)); - } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesRanksTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesRanksTests.java index 7f784533b082e..35c566c2e80cf 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesRanksTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesRanksTests.java @@ -19,26 +19,17 @@ package org.elasticsearch.search.aggregations.metrics.percentiles.tdigest; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.Writeable.Reader; -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.metrics.percentiles.InternalPercentilesRanksTestCase; import org.elasticsearch.search.aggregations.metrics.percentiles.ParsedPercentiles; -import org.elasticsearch.search.aggregations.metrics.percentiles.Percentile; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; -import java.io.IOException; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import static org.hamcrest.Matchers.equalTo; - public class InternalTDigestPercentilesRanksTests extends InternalPercentilesRanksTestCase { @Override @@ -127,85 +118,4 @@ protected InternalTDigestPercentileRanks mutateInstance(InternalTDigestPercentil } return new InternalTDigestPercentileRanks(name, percents, state, keyed, formatter, pipelineAggregators, metaData); } - - public void testEmptyRanksXContent() throws IOException { - double[] percents = new double[]{1,2,3}; - boolean keyed = randomBoolean(); - DocValueFormat docValueFormat = randomNumericDocValueFormat(); - - final TDigestState state = new TDigestState(100); - InternalTDigestPercentileRanks agg = new InternalTDigestPercentileRanks("test", percents, state, keyed, docValueFormat, - Collections.emptyList(), Collections.emptyMap()); - - for (Percentile percentile : agg) { - Double value = percentile.getValue(); - assertThat(agg.percent(value), equalTo(Double.NaN)); - assertThat(agg.percentAsString(value), equalTo("NaN")); - } - - - XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint(); - builder.startObject(); - agg.doXContentBody(builder, ToXContent.EMPTY_PARAMS); - builder.endObject(); - String expected; - if (keyed && docValueFormat.equals(DocValueFormat.RAW)) { - expected = "{\n" + - " \"values\" : {\n" + - " \"1.0\" : null,\n" + - " \"2.0\" : null,\n" + - " \"3.0\" : null\n" + - " }\n" + - "}"; - } else if (keyed) { - expected = "{\n" + - " \"values\" : {\n" + - " \"1.0\" : null,\n" + - " \"1.0_as_string\" : null,\n" + - " \"2.0\" : null,\n" + - " \"2.0_as_string\" : null,\n" + - " \"3.0\" : null,\n" + - " \"3.0_as_string\" : null\n" + - " }\n" + - "}"; - } else if (keyed == false && docValueFormat.equals(DocValueFormat.RAW)) { - 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" + - "}"; - } else { - expected = "{\n" + - " \"values\" : [\n" + - " {\n" + - " \"key\" : 1.0,\n" + - " \"value\" : null,\n" + - " \"value_as_string\" : null\n" + - " },\n" + - " {\n" + - " \"key\" : 2.0,\n" + - " \"value\" : null,\n" + - " \"value_as_string\" : null\n" + - " },\n" + - " {\n" + - " \"key\" : 3.0,\n" + - " \"value\" : null,\n" + - " \"value_as_string\" : null\n" + - " }\n" + - " ]\n" + - "}"; - } - assertThat(Strings.toString(builder), equalTo(expected)); - } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesTests.java index cf3c44bf3da7f..73c9b8a16084e 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/tdigest/InternalTDigestPercentilesTests.java @@ -19,26 +19,17 @@ package org.elasticsearch.search.aggregations.metrics.percentiles.tdigest; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.Writeable; -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.metrics.percentiles.InternalPercentilesTestCase; import org.elasticsearch.search.aggregations.metrics.percentiles.ParsedPercentiles; -import org.elasticsearch.search.aggregations.metrics.percentiles.Percentile; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; -import java.io.IOException; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import static org.hamcrest.Matchers.equalTo; - public class InternalTDigestPercentilesTests extends InternalPercentilesTestCase { @Override @@ -123,85 +114,4 @@ protected InternalTDigestPercentiles mutateInstance(InternalTDigestPercentiles i } return new InternalTDigestPercentiles(name, percents, state, keyed, formatter, pipelineAggregators, metaData); } - - public void testEmptyPercentilesXContent() throws IOException { - double[] percents = new double[]{1,2,3}; - boolean keyed = randomBoolean(); - DocValueFormat docValueFormat = randomNumericDocValueFormat(); - - final TDigestState state = new TDigestState(100); - InternalTDigestPercentiles agg = new InternalTDigestPercentiles("test", percents, state, keyed, docValueFormat, - Collections.emptyList(), Collections.emptyMap()); - - for (Percentile percentile : agg) { - Double value = percentile.getValue(); - assertThat(agg.percentile(value), equalTo(Double.NaN)); - assertThat(agg.percentileAsString(value), equalTo("NaN")); - } - - - XContentBuilder builder = JsonXContent.contentBuilder().prettyPrint(); - builder.startObject(); - agg.doXContentBody(builder, ToXContent.EMPTY_PARAMS); - builder.endObject(); - String expected; - if (keyed && docValueFormat.equals(DocValueFormat.RAW)) { - expected = "{\n" + - " \"values\" : {\n" + - " \"1.0\" : null,\n" + - " \"2.0\" : null,\n" + - " \"3.0\" : null\n" + - " }\n" + - "}"; - } else if (keyed) { - expected = "{\n" + - " \"values\" : {\n" + - " \"1.0\" : null,\n" + - " \"1.0_as_string\" : null,\n" + - " \"2.0\" : null,\n" + - " \"2.0_as_string\" : null,\n" + - " \"3.0\" : null,\n" + - " \"3.0_as_string\" : null\n" + - " }\n" + - "}"; - } else if (keyed == false && docValueFormat.equals(DocValueFormat.RAW)) { - 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" + - "}"; - } else { - expected = "{\n" + - " \"values\" : [\n" + - " {\n" + - " \"key\" : 1.0,\n" + - " \"value\" : null,\n" + - " \"value_as_string\" : null\n" + - " },\n" + - " {\n" + - " \"key\" : 2.0,\n" + - " \"value\" : null,\n" + - " \"value_as_string\" : null\n" + - " },\n" + - " {\n" + - " \"key\" : 3.0,\n" + - " \"value\" : null,\n" + - " \"value_as_string\" : null\n" + - " }\n" + - " ]\n" + - "}"; - } - assertThat(Strings.toString(builder), equalTo(expected)); - } } From cd9bac7f86355db58ed456e8a8e445191dd692b7 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Thu, 7 Jun 2018 15:24:06 +0000 Subject: [PATCH 3/6] Review cleanup: centralize tests more --- .../AbstractPercentilesTestCase.java | 56 ++++++++++++++++++ .../InternalPercentilesRanksTestCase.java | 58 ++----------------- .../InternalPercentilesTestCase.java | 56 ++---------------- 3 files changed, 64 insertions(+), 106 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/AbstractPercentilesTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/AbstractPercentilesTestCase.java index 233b8f940fd90..c4a3d3b2ffcef 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/AbstractPercentilesTestCase.java +++ b/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> extends InternalAggregationTestCase { @@ -89,4 +96,53 @@ public static double[] randomPercents(boolean sorted) { protected Predicate 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)); + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java index 1ddd3d436d734..a63fd42da7d96 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java @@ -19,17 +19,9 @@ 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.InternalAggregation; import org.elasticsearch.search.aggregations.ParsedAggregation; -import java.io.IOException; -import java.util.Collections; - import static org.hamcrest.Matchers.equalTo; public abstract class InternalPercentilesRanksTestCase @@ -50,51 +42,9 @@ protected final void assertFromXContent(T aggregation, ParsedAggregation parsedA assertTrue(parsedClass != null && parsedClass.isInstance(parsedAggregation)); } - 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(); - assertThat(agg.percent(value), equalTo(Double.NaN)); - assertThat(agg.percentAsString(value), equalTo("NaN")); - } - - 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)); + @Override + protected void assertPercentile(T agg, Double value) { + assertThat(agg.percent(value), equalTo(Double.NaN)); + assertThat(agg.percentAsString(value), equalTo("NaN")); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesTestCase.java index 927970a3b8e45..1024577a6b6ed 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesTestCase.java @@ -19,16 +19,9 @@ 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.InternalAggregation; import org.elasticsearch.search.aggregations.ParsedAggregation; -import java.io.IOException; -import java.util.Collections; import java.util.List; import static org.hamcrest.Matchers.equalTo; @@ -59,50 +52,9 @@ public static double[] randomPercents() { return percents; } - public void testEmptyPercentilesXContent() 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(); - assertThat(agg.percentile(value), equalTo(Double.NaN)); - assertThat(agg.percentileAsString(value), equalTo("NaN")); - } - - 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)); + @Override + protected void assertPercentile(T agg, Double value) { + assertThat(agg.percentile(value), equalTo(Double.NaN)); + assertThat(agg.percentileAsString(value), equalTo("NaN")); } } From 433a65451561d37b73bae99dabcb3bedd325304c Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Thu, 7 Jun 2018 15:27:59 +0000 Subject: [PATCH 4/6] Review cleanup: add note to 7.0 breaking changes --- docs/reference/release-notes/7.0.0-alpha1.asciidoc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/reference/release-notes/7.0.0-alpha1.asciidoc b/docs/reference/release-notes/7.0.0-alpha1.asciidoc index 1cc328f16598b..cf2e1e30be050 100644 --- a/docs/reference/release-notes/7.0.0-alpha1.asciidoc +++ b/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) \ No newline at end of file From 25ff123cdb7f22320d2a9988f7f5dfc5155352af Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Wed, 13 Jun 2018 15:26:06 +0000 Subject: [PATCH 5/6] Explicitly check for NaN and manually return value --- .../metrics/InternalNumericMetricsAggregation.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalNumericMetricsAggregation.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalNumericMetricsAggregation.java index b3439671580e7..788d1de1071ae 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalNumericMetricsAggregation.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalNumericMetricsAggregation.java @@ -79,7 +79,12 @@ protected MultiValue(StreamInput in) throws IOException { public abstract double value(String name); public String valueAsString(String name) { - return format.format(value(name)).toString(); + // Explicitly check for NaN, since it formats to "�" or "NaN" depending on JDK version + Double value = value(name); + if (value.isNaN()) { + return String.valueOf(Double.NaN); + } + return format.format(value).toString(); } @Override From 98d9c9b96b77faa28bbd774345da3d5eb2baafec Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Thu, 14 Jun 2018 19:01:16 +0000 Subject: [PATCH 6/6] Move NaN check to Decimal#DocValueFormat, better comments --- .../org/elasticsearch/search/DocValueFormat.java | 16 ++++++++++++++++ .../InternalNumericMetricsAggregation.java | 7 +------ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/DocValueFormat.java b/server/src/main/java/org/elasticsearch/search/DocValueFormat.java index 242e088747341..3a3b1c680aba1 100644 --- a/server/src/main/java/org/elasticsearch/search/DocValueFormat.java +++ b/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); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalNumericMetricsAggregation.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalNumericMetricsAggregation.java index 788d1de1071ae..b3439671580e7 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalNumericMetricsAggregation.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalNumericMetricsAggregation.java @@ -79,12 +79,7 @@ protected MultiValue(StreamInput in) throws IOException { public abstract double value(String name); public String valueAsString(String name) { - // Explicitly check for NaN, since it formats to "�" or "NaN" depending on JDK version - Double value = value(name); - if (value.isNaN()) { - return String.valueOf(Double.NaN); - } - return format.format(value).toString(); + return format.format(value(name)).toString(); } @Override