Skip to content
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

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

Merged
merged 10 commits into from
Jun 18, 2018
Merged

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

Changes from 1 commit
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
7 changes: 6 additions & 1 deletion ...java/org/elasticsearch/search/aggregations/metrics/InternalNumericMetricsAggregation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Is this only the case for certain locales? I would bne suprised if some JDKs would return a weird UTF8 character in all cases. To make this comment more readable it would probably also make sense to put in the bad utf8 value as octal or hex codepoint and to clarify under which circumstances this happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not actually sure how this behaves across Locales, but I don't think it matters for us. We seem to always initialize the Decimal DocValueFormat with Locale.Root which I believe uses the JRE's default symbol table.

So for JDK8 the root locale will use JRELocaleProviderAdapter to get the symbols, which loads sun.text.resources.FormatData, and you can see the NaN symbol is \uFFFD

For JDK 9+, the root locale will use CLDRLocaleProviderAdapter, which loads sun.text.resources.cldr.FormatData. And in that resource file you can see the NaN symbol is "NaN" (Can't find a link to the code, but you can see it in your IDE).

++ to making the comment more descriptive. I'll try to distill this thread into a sane comment, and probably leave a reference to the comments here in case anyone wants to see more info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an aside, I really wonder why Oracle thought � would be a good default representation of "NaN"... :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I don't like how this was implemented, looking at it. Going to move it over to the DocValueFormat itself, so that it only applies to the Decimal formatter when looking at doubles... otherwise it'll be checked against all formatters (geo, IP, etc). Harmless I think, but no need.

Double value = value(name);
if (value.isNaN()) {
return String.valueOf(Double.NaN);
}
return format.format(value).toString();
}

@Override
Expand Down