Replace AggregatorTestCase#search with AggregatorTestCase#searchAndReduce#60683
Merged
jimczi merged 4 commits intoelastic:masterfrom Aug 6, 2020
Merged
Conversation
…duce This commit removes the ability to test the top level result of an aggregator before it runs the final reduce. All aggregator tests that use AggregatorTestCase#search are rewritten with AggregatorTestCase#searchAndReduce in order to ensure that we test the final output (the one sent to the end user) rather than an intermediary result that could be different. This change also removes spurious commits triggered on top of a random index writer. These commits slow down the tests and are redundant with the commits that the random index writer performs.
Collaborator
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
not-napoleon
approved these changes
Aug 4, 2020
Member
not-napoleon
left a comment
There was a problem hiding this comment.
Looks good. Please clean up that one for loop that got missed, otherwise 👍
| toMerge.add(buckets.get(startIdx)); // Don't remove the startIdx bucket because it will be replaced by the merged bucket | ||
|
|
||
| reduceContext.consumeBucketsAndMaybeBreak(- (toMerge.size() - 1)); | ||
| int toRemove = toMerge.stream().mapToInt(b -> countInnerBucket(b)+1).sum(); |
Member
There was a problem hiding this comment.
Is this an actual bug you found while making this change?
Contributor
Author
There was a problem hiding this comment.
A minor one, yes. The max bucket count is not accurate when buckets are auto-merged.
...src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java
Outdated
Show resolved
Hide resolved
...va/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java
Outdated
Show resolved
Hide resolved
nik9000
approved these changes
Aug 5, 2020
Member
nik9000
left a comment
There was a problem hiding this comment.
I think for the auto date histo and variable width histo I'll miss being able to assert on things that aren't yet finally reduced, but it is worth it.
jimczi
added a commit
to jimczi/elasticsearch
that referenced
this pull request
Aug 6, 2020
This commit fixes the computation of the subset size on empty buckets (doc count of 0). The aggregator test refactoring in elastic#60683 revealed this bug.
jimczi
added a commit
that referenced
this pull request
Aug 6, 2020
This commit fixes the computation of the subset size on empty buckets (doc count of 0). The aggregator test refactoring in #60683 revealed this bug.
jimczi
added a commit
that referenced
this pull request
Aug 6, 2020
This commit fixes the computation of the subset size on empty buckets (doc count of 0). The aggregator test refactoring in #60683 revealed this bug.
nik9000
added a commit
that referenced
this pull request
Aug 19, 2020
With #60683 we stopped forcing aggregating all docs using a single Aggregator which made some of our accuracy assumptions about the stats aggregator incorrect. This adds a test that does the forcing and asserts the old accuracy and adds a test without the forcing with much looser accuracy guarantees. Closes #61132
nik9000
added a commit
that referenced
this pull request
Aug 19, 2020
With #60683 we stopped forcing aggregating all docs using a single Aggregator which made some of our accuracy assumptions about the stats aggregator incorrect. This adds a test that does the forcing and asserts the old accuracy and adds a test without the forcing with much looser accuracy guarantees. Closes #61132
61 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
This commit removes the ability to test the top level result of an aggregator
before it runs the final reduce. All aggregator tests that use AggregatorTestCase#search
are rewritten with AggregatorTestCase#searchAndReduce in order to ensure that we test
the final output (the one sent to the end user) rather than an intermediary result
that could be different.
This change also removes spurious commits triggered on top of a random index writer.
These commits slow down the tests and are redundant with the commits that the
random index writer performs.