Change nested agg to execute in doc id order #8454
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| if (childDocs.docID() > prevParentDoc) { | ||
| childDocId = childDocs.docID(); | ||
| } else { | ||
| childDocId = childDocs.advance(prevParentDoc); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think
if (prevParentDoc == -1) {
childDocId = childDocs.nextDoc();
} else {
if (childDocs.docID() > prevParentDoc) {
childDocId = childDocs.docID();
} else {
childDocId = childDocs.advance(prevParentDoc);
}
}could just be replaced with
if (childDocs.docID() > prevParentDoc) {
childDocId = childDocs.docID();
} else {
childDocId = childDocs.advance(prevParentDoc + 1);
}?
(No more check that the previous parent doc is -1, and advance to prevParentDoc+1 instead of prevParentDoc)
|
@martijnvg I like the change and just left one minor comment about a potential simplification. |
martijnvg
force-pushed
the
improvements/nested_agg
branch
from
20bfb27
to
452476b
Compare
|
@jpountz I updated the PR with your simplification and it looks much better now! |
|
LGTM, thanks @martijnvg ! |
martijnvg
force-pushed
the
improvements/nested_agg
branch
from
452476b
to
1957262
Compare
…sed bitset in `nested` agg. Also the nested agg now requires docs to be consumed / scored in order. Closes elastic#8454
martijnvg
force-pushed
the
improvements/nested_agg
branch
from
1957262
to
5c7614f
Compare
martijnvg
added a commit
that referenced
this pull request
Nov 14, 2014
…sed bitset in `nested` agg. Also the nested agg now requires docs to be consumed / scored in order. Closes #8454
martijnvg
added a commit
that referenced
this pull request
Nov 14, 2014
…sed bitset in `nested` agg. Also the nested agg now requires docs to be consumed / scored in order. Closes #8454
clintongormley
changed the title
martijnvg
added a commit
to martijnvg/elasticsearch
that referenced
this pull request
Jan 26, 2015
… the same parent doc id. This bug was introduced by elastic#8454 which allowed the childFilter to only be consumed once. By adding the child docid buffering multiple buckets can now be emitted by the same doc id. This child docid buffering only happens in the scope of the current root document, so the amount of child doc ids buffered is small. Closes elastic#9317 Closes elastic#9346
martijnvg
added a commit
that referenced
this pull request
Jan 26, 2015
… the same parent doc id. This bug was introduced by #8454 which allowed the childFilter to only be consumed once. By adding the child docid buffering multiple buckets can now be emitted by the same doc id. This child docid buffering only happens in the scope of the current root document, so the amount of child doc ids buffered is small. Closes #9317 Closes #9346
martijnvg
added a commit
that referenced
this pull request
Jan 26, 2015
… the same parent doc id. This bug was introduced by #8454 which allowed the childFilter to only be consumed once. By adding the child docid buffering multiple buckets can now be emitted by the same doc id. This child docid buffering only happens in the scope of the current root document, so the amount of child doc ids buffered is small. Closes #9317 Closes #9346
martijnvg
added a commit
that referenced
this pull request
Jan 26, 2015
… the same parent doc id. This bug was introduced by #8454 which allowed the childFilter to only be consumed once. By adding the child docid buffering multiple buckets can now be emitted by the same doc id. This child docid buffering only happens in the scope of the current root document, so the amount of child doc ids buffered is small. Closes #9317 Closes #9346
clintongormley
added
:Nested Docs
and removed
:Analytics/Aggregations
clintongormley
changed the title
mute
pushed a commit
to mute/elasticsearch
that referenced
this pull request
Jul 29, 2015
…sed bitset in `nested` agg. Also the nested agg now requires docs to be consumed / scored in order. Closes elastic#8454
mute
pushed a commit
to mute/elasticsearch
that referenced
this pull request
Jul 29, 2015
… the same parent doc id. This bug was introduced by elastic#8454 which allowed the childFilter to only be consumed once. By adding the child docid buffering multiple buckets can now be emitted by the same doc id. This child docid buffering only happens in the scope of the current root document, so the amount of child doc ids buffered is small. Closes elastic#9317 Closes elastic#9346
clintongormley
added
:Search/Search
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By executing in docid order the child level filters don't require random access bitset any more and can use normal docid set iterators. Also the child filters don't need the bitset cache anymore and can rely on the normal filter cache.
Note: PR is against 1.x branch