-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Add more tests for top_hits aggregation #22754
Conversation
Add unit tests for `TopHitsAggregator` and convert some snippets in docs for `top_hits` aggregation to `// CONSOLE`. Relates to elastic#22278 Relates to elastic#18160
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.
LGTM
@@ -122,7 +122,13 @@ public void collect(int docId, long bucket) throws IOException { | |||
// In the QueryPhase we don't need this protection, because it is build into the IndexSearcher, | |||
// but here we create collectors ourselves and we need prevent OOM because of crazy an offset and size. | |||
topN = Math.min(topN, subSearchContext.searcher().getIndexReader().maxDoc()); | |||
TopDocsCollector<?> topLevelCollector = sort != null ? TopFieldCollector.create(sort.sort, topN, true, subSearchContext.trackScores(), subSearchContext.trackScores()) : TopScoreDocCollector.create(topN); | |||
TopDocsCollector<?> topLevelCollector; |
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.
👍 much better readable
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.
Thanks! I think it is pretty rare a ternary that spans multiple lines is more readable than a multi-line if.
Thanks for reviewing @martijnvg! |
* master: (47 commits) Remove non needed import use expectThrows instead of manually testing exception Fix checkstyle and a test Update after review Read ec2 discovery address from aws instance tags Invalidate cached query results if query timed out (elastic#22807) Add remaining generated painless API Generate reference links for painless API (elastic#22775) [TEST] Fix ElasticsearchExceptionTests Add parsing method for ElasticsearchException.generateThrowableXContent() (elastic#22783) Improve connection closing in `RemoteClusterConnection` (elastic#22804) Docs: Cluster allocation explain should be on one page Remove DFS_QUERY_AND_FETCH as a search type (elastic#22787) Add repository-url module and move URLRepository (elastic#22752) fix date-processor to a new default year for every new pipeline execution. (elastic#22601) Add tests for top_hits aggregation (elastic#22754) [TEST] Added this for 93a28b0 submitted via elastic#22772 Fix typo in comment in OsProbe.java Add new ruby search library to community clients doc (elastic#22765) RangeQuery WITHIN case now normalises query (elastic#22431) ...
Add unit tests for
TopHitsAggregator
and convert some snippets indocs for
top_hits
aggregation to// CONSOLE
.Relates to #22278
Relates to #18160