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.

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

Promote longs to doubles when a terms agg mixes decimal and non-decimal numbers #22449

Merged
merged 4 commits into from Jan 10, 2017

Conversation

Projects
None yet
3 participants
@jimczi
Copy link
Member

commented Jan 5, 2017

This change makes the terms aggregation work when the buckets coming from different indices are a mix of decimal numbers and non-decimal numbers. In this case non-decimal number (longs) are promoted to decimal (double) which can result in a loss of precision for big numbers.

Fixes #22232

@jpountz

jpountz approved these changes Jan 5, 2017

Copy link
Contributor

left a comment

Looks good, I just left a suggestion about the implementation.

core/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongTerms.java Outdated
*/
promoteLongsToDouble = true;
decimalFormat = ((DoubleTerms) agg).format;
break;

This comment has been minimized.

Copy link
@jpountz

jpountz Jan 5, 2017

Contributor

Maybe in that case we could just return agg.doReduce on this DoubleTerms instance which would reuse the logic we have in DoubleTerms rather than duplicating it below?

@jimczi

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2017

Thanks @jpountz I pushed a modif to address your feedback. Should we merge this to 5.x ?

jimczi added some commits Jan 5, 2017

Promote longs to doubles when a terms agg mixes decimal and non-decim…
…al number

This change makes the terms aggregation work when the buckets coming from different indices are a mix of decimal numbers and non-decimal numbers. In this case non-decimal number (longs) are promoted to decimal (double) which can result in a loss of precision for big numbers.

Fixes #22232

@jimczi jimczi merged commit 433c822 into elastic:master Jan 10, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

@jimczi jimczi deleted the jimczi:terms_longs_doubles branch Jan 10, 2017

@jimczi

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2017

This should not be back ported to 5.x (this should be considered as a breaking change) until #22265 is resolved.

@tylerfontaine

This comment has been minimized.

Copy link
Member

commented Mar 1, 2017

@jimczi with #22265 closed, will this be backported to 5.x, or are we still considering it a breaking change, and it won't be available until 6.0?

Edit. Apologies - it isn't closed. I misread the feed on that one. Sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.