Skip to content

Conversation

@nik9000
Copy link
Member

@nik9000 nik9000 commented Jan 23, 2017

Add unit tests for TopHitsAggregator and convert some snippets in
docs for top_hits aggregation to // CONSOLE.

Relates to #22278
Relates to #18160

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
@nik9000 nik9000 added :Analytics/Aggregations Aggregations review >test Issues or PRs that are addressing/adding tests v5.3.0 v6.0.0-alpha1 labels Jan 23, 2017
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

// 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 much better readable

Copy link
Member Author

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.

@nik9000 nik9000 merged commit d704a88 into elastic:master Jan 25, 2017
@nik9000
Copy link
Member Author

nik9000 commented Jan 25, 2017

Thanks for reviewing @martijnvg!

master: d704a88
5.x: d10dcea

nik9000 added a commit that referenced this pull request Jan 25, 2017
Add unit tests for `TopHitsAggregator` and convert some snippets in
docs for `top_hits` aggregation to `// CONSOLE`.

Relates to #22278
Relates to #18160
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 26, 2017
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Aggregations Aggregations >test Issues or PRs that are addressing/adding tests v5.3.0 v6.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants