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

Sampler agg could not be used with Terms agg’s order. #10785

Closed
wants to merge 3 commits into from
Closed

Sampler agg could not be used with Terms agg’s order. #10785

wants to merge 3 commits into from

Conversation

markharwood
Copy link
Contributor

The Sampler agg was not capable of collecting samples for more than one parent bucket.
Added a Junit test case and changed BestDocsDeferringCollector to internally maintain collections per parent bucket.

Closes #10719

@markharwood
Copy link
Contributor Author

@jpountz + @colings86 suggested reviewers if you have the time please :)

@@ -189,7 +263,7 @@ public void replayRelatedMatches(ScoreDoc[] sd) throws IOException {
if ((rebased >= 0) && (rebased <= maxDocId)) {
currentScore = scoreDoc.score;
currentDocId = rebased;
leafCollector.collect(rebased, 0);
leafCollector.collect(rebased, scoreDoc.shardIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a comment here reminding that the shardIndex is being used to store the parent bucket ordinal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@colings86
Copy link
Contributor

@markharwood left a couple of small comments

@markharwood
Copy link
Contributor Author

Rebased on latest master

private int shardSize;
private PerSegmentCollects perSegCollector;
private int matchedDocs;
private BigArrays bigArrays;
Copy link
Contributor

Choose a reason for hiding this comment

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

can it be final?

@jpountz
Copy link
Contributor

jpountz commented May 19, 2015

Left some minor comments, otherwise it looks good to me

The Sampler agg was not capable of collecting samples for more than one parent bucket.
Added a Junit test case and changed BestDocsDeferringCollector to internally maintain collections per parent bucket.

Closes #10719
…wing comment from @jpountz. Added use of final, rebased on master.
@markharwood
Copy link
Contributor Author

@jpountz changed to always return 0 for docCount if no prior collections rather than erroring.
I believe that is what you meant here #10785 (comment). Good to go?

@jpountz
Copy link
Contributor

jpountz commented May 22, 2015

This is what I meant indeed! LGTM

@markharwood
Copy link
Contributor Author

Pushed to master in 8c3500a

@kevinkluge kevinkluge removed the review label May 22, 2015
@clintongormley clintongormley changed the title Aggregation fix: Sampler agg could not be used with Terms agg’s order. Sampler agg could not be used with Terms agg’s order. Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sampler Aggregator cannot be used in terms agg order
4 participants