-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Refactor aggregator test case #90149
Refactor aggregator test case #90149
Conversation
Pinging @elastic/es-analytics-geo (Team:Analytics) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's much nicer!
MappedFieldType... fieldTypes) { | ||
public AggTestConfig(IndexSearcher searcher, Query query, AggregationBuilder builder, MappedFieldType... fieldTypes) { | ||
this(searcher, query, builder, DEFAULT_MAX_BUCKETS, randomBoolean(), fieldTypes); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if Query
should default to MatchAllDocs
. Otherwise, 👍
* @param splitLeavesIntoSeparateAggregators If true this creates a new {@link Aggregator} | ||
* for each leaf as though it were a separate index. If false this aggregates | ||
* all leaves together, like we do in production. | ||
* @param aggTestConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zap this?
@elasticmachine update branch |
This refactors
searchAndReduce
to use a parameter object, which removes a bunch of redundant overrides, and makes life much easier for adding new parameters in the future. I need that second part to add a parameter for testing caching over in #90114 so that the exactly two tests which are failing can tell the framework they don't actually expect to be cached.Yaks--;