Skip to content

Percentile/Ranks should return null instead of NaN when empty #30460

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jun 18, 2018
Merged

Percentile/Ranks should return null instead of NaN when empty #30460

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b7a7f7f
Percentile/Ranks should return null instead of NaN when empty
polyfractal May 7, 2018
9b3460b
Merge remote-tracking branch 'origin/master' into consistent_missing_…
polyfractal May 14, 2018
fc34656
Centralize empty test, test xcontent roundtrip with empty occasionally
polyfractal May 15, 2018
d6009e6
Merge remote-tracking branch 'origin/master' into consistent_missing_…
polyfractal May 16, 2018
1e6801a
Merge remote-tracking branch 'origin/master' into consistent_missing_…
polyfractal Jun 5, 2018
cd9bac7
Review cleanup: centralize tests more
polyfractal Jun 7, 2018
433a654
Review cleanup: add note to 7.0 breaking changes
polyfractal Jun 7, 2018
25ff123
Explicitly check for NaN and manually return value
polyfractal Jun 13, 2018
8be43b7
Merge remote-tracking branch 'origin/master' into consistent_missing_…
polyfractal Jun 13, 2018
98d9c9b
Move NaN check to Decimal#DocValueFormat, better comments
polyfractal Jun 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions ...ain/java/org/elasticsearch/search/aggregations/metrics/percentiles/ParsedPercentiles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
Expand All @@ -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));
}
}
Expand Down
10 changes: 5 additions & 5 deletions ...ticsearch/search/aggregations/metrics/percentiles/hdr/AbstractInternalHDRPercentiles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down
10 changes: 5 additions & 5 deletions ...h/search/aggregations/metrics/percentiles/tdigest/AbstractInternalTDigestPercentiles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion ...rg/elasticsearch/search/aggregations/metrics/percentiles/AbstractPercentilesTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,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();
Expand Down
58 changes: 58 additions & 0 deletions ...asticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<T extends InternalAggregation & PercentileRanks>
extends AbstractPercentilesTestCase<T> {

Expand All @@ -39,4 +49,52 @@ protected final void assertFromXContent(T aggregation, ParsedAggregation parsedA
Class<? extends ParsedPercentiles> parsedClass = implementationClass();
assertTrue(parsedClass != null && parsedClass.isInstance(parsedAggregation));
}

public void testEmptyRanksXContent() throws IOException {
Copy link
Member

@cbuescher cbuescher Jun 7, 2018

Choose a reason for hiding this comment

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

You mentioned earlier you cannot push this test up into AbstractPercentilesTestCase because of some subtle difference, but I cannot spot it. Do you remember what it was? Otherwise I'd give it another try to push it up.

Copy link
Contributor Author

@polyfractal polyfractal Jun 7, 2018

Choose a reason for hiding this comment

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

It's super tiny: Percentiles uses percent() / percentAsString() while PercentileRanks uses percentile() / percentileAsString().

I could collapse them into a single test and then do an instanceOf or getType() and switch on that if you think it'd be cleaner. Less test code duplication, but a bit more fragile.

Copy link
Member

@cbuescher cbuescher Jun 7, 2018

Choose a reason for hiding this comment

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

Ah, I see it now. What about pulling the test up and just doing the two lines of assertions that are different in their own little helper method that you overwrite differently in both cases? I'm usually also not a fan of doing so much code acrobatics in tests but in this case I think the gain in non-duplicated lines of code would justify it. I don't think its super important though, thanks for pointing out the difference.

Copy link
Contributor Author

@polyfractal polyfractal Jun 7, 2018

Choose a reason for hiding this comment

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

++ this cleaned up nicely. Thanks for the suggestion!

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));
}
}
56 changes: 56 additions & 0 deletions ...rg/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<T extends InternalAggregation & Percentiles> extends AbstractPercentilesTestCase<T> {

@Override
Expand All @@ -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));
}
}
1 change: 1 addition & 0 deletions ...csearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesRanksTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.List;
import java.util.Map;


public class InternalHDRPercentilesRanksTests extends InternalPercentilesRanksTestCase<InternalHDRPercentileRanks> {

@Override
Expand Down