Fix IndexOutOfBoundsException in histograms for NaN doubles (#26787) #26856
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
jpountz
approved these changes
Oct 6, 2017
|
@elasticmachine please test it |
jpountz
pushed a commit
that referenced
this pull request
Oct 6, 2017
jpountz
pushed a commit
that referenced
this pull request
Oct 6, 2017
jpountz
pushed a commit
that referenced
this pull request
Oct 6, 2017
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Oct 6, 2017
* ccr: (42 commits) [DOCS] Added info about snapshotting your data before an upgrade. Add documentation about disabling `_field_names`. (elastic#26813) Remove UnsortedNumericDoubleValues (elastic#26817) Fix IndexOutOfBoundsException in histograms for NaN doubles (elastic#26787) (elastic#26856) [TEST] Added skipping the `headers` feature to the Bulk REST YAML test Update type-field.asciidoc Fix search_after with geo distance sorting (elastic#26891) Use proper logging placeholder for Netty logging Add Netty channel information on write and flush failure Remove deploying in JBoss documentation Document JVM option MaxFDLimit for macOS () Add additional low-level logging handler () Unwrap causes when maybe dying Change log level on write and flush failure to warn [TEST] add test to ensure legacy list syntax in yml works fine Bump BWC version for settings serialization to 6.1.0 Removed void token filter entries and added two tests Added Bengali Analyzer to Elasticsearch with respect to the lucene update(PR#238) Fix toString() in SnapshotStatus (elastic#26852) elastic#26870 change bwc version for fuzzy_transpositions to 6.1 after backport ...
jasontedor
added a commit
to martijnvg/elasticsearch
that referenced
this pull request
Oct 6, 2017
* ccr: (110 commits) Use LF line endings in Painless generated files (elastic#26822) [DOCS] Added info about snapshotting your data before an upgrade. Add documentation about disabling `_field_names`. (elastic#26813) Remove UnsortedNumericDoubleValues (elastic#26817) Fix IndexOutOfBoundsException in histograms for NaN doubles (elastic#26787) (elastic#26856) [TEST] Added skipping the `headers` feature to the Bulk REST YAML test Update type-field.asciidoc Fix search_after with geo distance sorting (elastic#26891) Use proper logging placeholder for Netty logging Add Netty channel information on write and flush failure Remove deploying in JBoss documentation Document JVM option MaxFDLimit for macOS () Add additional low-level logging handler () Unwrap causes when maybe dying Change log level on write and flush failure to warn [TEST] add test to ensure legacy list syntax in yml works fine Bump BWC version for settings serialization to 6.1.0 Removed void token filter entries and added two tests Added Bengali Analyzer to Elasticsearch with respect to the lucene update(PR#238) Fix toString() in SnapshotStatus (elastic#26852) ...
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Nov 28, 2018
In this test we were randomizing different values but minDocCount was hardcoded to 1. It's important to test other values, especially `0` as it's the default. The test needed some adapting in the way buckets are randomly generated: all aggs need to share the same interval, minDocCount and emptyBucketInfo. Also assertions need to take into account that more (or less) buckets are expected depending on minDocCount. This was originated by elastic#35921 and its need to test adding empty buckets as part of the reduce phase. Also relates to elastic#26856 as one more key comparison needed to use `Double.compare` to properly handle `NaN` values, this was triggered by the increased test coverage.
javanna
added a commit
that referenced
this pull request
Nov 28, 2018
In `InternalHistogramTests` we were randomizing different values but `minDocCount` was hardcoded to `1`. It's important to test other values, especially `0` as it's the default. To make this possible, the test needed some adapting in the way buckets are randomly generated: all aggs need to share the same `interval`, `minDocCount` and `emptyBucketInfo`. Also assertions need to take into account that more (or less) buckets are expected depending on `minDocCount`. This was originated by #35921 and its need to test adding empty buckets as part of the reduce phase. Also relates to #26856 as one more key comparison needed to use `Double.compare` to properly handle `NaN` values, which was triggered by the increased test coverage.
javanna
added a commit
that referenced
this pull request
Dec 3, 2018
In `InternalHistogramTests` we were randomizing different values but `minDocCount` was hardcoded to `1`. It's important to test other values, especially `0` as it's the default. To make this possible, the test needed some adapting in the way buckets are randomly generated: all aggs need to share the same `interval`, `minDocCount` and `emptyBucketInfo`. Also assertions need to take into account that more (or less) buckets are expected depending on `minDocCount`. This was originated by #35921 and its need to test adding empty buckets as part of the reduce phase. Also relates to #26856 as one more key comparison needed to use `Double.compare` to properly handle `NaN` values, which was triggered by the increased test coverage.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The linked issue should contain all information necessary to review.
There might be better ways to test this change, my experience with the Elasticsearch test design is limited.
I also have a working patch against the 5.6 branch, which I care about more at this point (having the fix in a future 5.x release). Let me know if you'd like me to submit that one separately.
Thanks!