Skip to content
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

Added unit test coverage for SignificantTerms #24904

Merged
merged 3 commits into from
Jun 23, 2017

Conversation

markharwood
Copy link
Contributor

Covers GlobalOrdinalsSignificantTermsAggregator, GlobalOrdinalsSignificantTermsAggregator.WithHash, SignificantLongTermsAggregator and SignificantStringTermsAggregator

Removed integration test

Relates #22278

@markharwood markharwood added :Analytics/Aggregations Aggregations >test Issues or PRs that are addressing/adding tests labels May 26, 2017
Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

I think we should add some of the tests from the removed integration test suite. Specifically I think we should test with include/exclude, with a different heuristic and unmapped/partially unmapped fields

@rjernst
Copy link
Member

rjernst commented Jun 13, 2017

@markharwood Do you want to address Colin's feedback and get this in?

@markharwood
Copy link
Contributor Author

Looking at this now

@markharwood
Copy link
Contributor Author

@colings86 Care to take another look?

Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

Left one comment but LGTM


// Search "odd"
SignificantTerms terms = searchAndReduce(searcher, new TermQuery(new Term("text", "odd")), sigAgg, textFieldType);

Copy link
Contributor

Choose a reason for hiding this comment

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

should we check that the number of buckets returned is zero instead to ensure nothing weird is going on?

…, GlobalOrdinalsSignificantTermsAggregator.WithHash, SignificantLongTermsAggregator and SignificantStringTermsAggregator

Removed integration test

Relates elastic#22278
…ound_filter. Requested include/exclude tests already exist in this unit test.
…us check on number of segments which was recently causing issues in SignificantTextAggregatorTests
@markharwood
Copy link
Contributor Author

jenkins test this

@markharwood markharwood merged commit 973530f into elastic:master Jun 23, 2017
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jun 26, 2017
* master:
  Move more token filters to analysis-common module
  Test: Allow merging mock secure settings (elastic#25387)
  Remove remaining `index.mapper.single_type` setting usage from tests (elastic#25388)
  Remove dead logger prefix code
  Tests: Improve stability and logging of TemplateUpgradeServiceIT tests (elastic#25386)
  Remove `index.mapping.single_type=false` from reindex tests (elastic#25365)
  Adapt `SearchIT#testSearchWithParentJoin` to new join field (elastic#25379)
  Added unit test coverage for SignificantTerms (elastic#24904)
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants