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

Add the ability to use the breadth_first mode with nested aggregations (such as top_hits) which require access to score information. #18127

Merged
merged 1 commit into from
May 4, 2016

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented May 4, 2016

The score is recomputed lazily for each document belonging to a top bucket.
Relates to #9825

@jimczi
Copy link
Contributor Author

jimczi commented May 4, 2016

@jpountz I misunderstood how the DeferringBucketCollector works especially this part:
The order parameter can still be used to refer to data from a child aggregation when using the breadth_first setting - the parent aggregation understands that this child aggregation will need to be called first before any of the other child aggregations.

This means that we don't need to make it smarter ;).

final PackedLongValues.Iterator docDeltaIterator = entry.docDeltas.iterator();
final PackedLongValues.Iterator buckets = entry.buckets.iterator();
int doc = 0;
for (long i = 0, end = entry.docDeltas.size(); i < end; ++i) {
doc += docDeltaIterator.next();
if (needsScores) {
docIt.advance(doc);
Copy link
Contributor

Choose a reason for hiding this comment

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

calling docIt.advance(doc) is illegal if the scorer is already positioned on doc, so it should be something like:

if (docIt.docId() < doc) {
  docIt.advance(doc);
}
assert docIt.doc() == doc; // aggregations should only be replayed on matching documents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, thanks.

@jpountz
Copy link
Contributor

jpountz commented May 4, 2016

Thanks @jimferenczi. I left some comments.

@jimczi
Copy link
Contributor Author

jimczi commented May 4, 2016

Thanks @jpountz.
I pushed d01ab76 to address your comments.

@jpountz
Copy link
Contributor

jpountz commented May 4, 2016

LGTM

…s (such as `top_hits`) which require access to score information.

The score is recomputed lazily for each document belonging to a top bucket.
Relates to #9825
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.

3 participants