Enable SourceLookup to leverage sequential stored fields reader#63035
Merged
cbuescher merged 6 commits intoelastic:masterfrom Oct 6, 2020
Merged
Enable SourceLookup to leverage sequential stored fields reader#63035cbuescher merged 6 commits intoelastic:masterfrom
cbuescher merged 6 commits intoelastic:masterfrom
Conversation
cbuescher
changed the title
Member
Author
|
Using elastic/rally-tracks#133 the current state shows improvements when using runtime fields in "docvalue_fields" but not inside queries.
|
cbuescher
force-pushed
the
issue-62621
branch
2 times, most recently
from
c49cd2c to
9d31ccf
Compare
cbuescher
force-pushed
the
issue-62621
branch
from
9d31ccf to
0362496
Compare
Member
Author
|
Plugging the optimization in SourceLookup#setSegmentAndDocument like in 6924c53 now also shows significant speed up when using runtime fields in a query: Baseline: master
|
Member
Author
|
I also tested terms aggregation on a runtime field over a time slice of the http_log docs (elastic/rally-tracks@ee9e11b), this PR also shows noticable speedups:
|
Member
Author
|
@elasticmachine run elasticsearch-ci/packaging-sample-windows |
cbuescher
changed the title
cbuescher
added
:Search/Search
Collaborator
|
Pinging @elastic/es-search (:Search/Search) |
jimczi
requested changes
Oct 1, 2020
Contributor
jimczi
left a comment
jimczi
left a comment
There was a problem hiding this comment.
Glad to see that the pr improves queries and aggregations significantly. I left one comment regarding some changes that could be removed.
| LeafReaderContext context, | ||
| int docId, | ||
| SourceLookup sourceLookup, | ||
| CheckedBiConsumer<Integer, FieldsVisitor, IOException> fieldReader, |
Contributor
There was a problem hiding this comment.
Can you remove this change now that SourceLookup leverages the sequential reader automatically ?
added 2 commits
October 1, 2020 18:56
Member
Author
|
@jimczi thanks for the review, I removed the left-overs from previous iterations |
jimczi
approved these changes
Oct 5, 2020
Contributor
jimczi
left a comment
jimczi
left a comment
There was a problem hiding this comment.
LGTM.
It would be nice to know the effect of the change when using the runtime field in conjunction with a sparse indexed field but we can tackle this in the rally challenge that you're iterating on. So +1 to merge as is.
Member
Author
|
@jimczi thanks, my understanding is this should go to 7.10 as well? |
cbuescher
pushed a commit
to cbuescher/elasticsearch
that referenced
this pull request
Oct 6, 2020
…tic#63035) In elastic#62509 we already plugged faster sequential access for stored fields in the fetch phase. This PR now adds using the potentially better field reader also in SourceLookup. Rally exeriments are showing that this speeds up e.g. when runtime fields that are using "_source" are added e.g. via "docvalue_fields" or are used in queries or aggs. Closes elastic#62621
cbuescher
pushed a commit
that referenced
this pull request
Oct 6, 2020
…) (#63316) In #62509 we already plugged faster sequential access for stored fields in the fetch phase. This PR now adds using the potentially better field reader also in SourceLookup. Rally exeriments are showing that this speeds up e.g. when runtime fields that are using "_source" are added e.g. via "docvalue_fields" or are used in queries or aggs. Closes #62621
Member
Author
|
I went ahead mergin to 7.x with #63316, let me know if there are any concerns with this. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This is a WIP draft of using similar optimizations as in #62509 for SourceLookup.
This PR is using the potentially better field reader from the fetch phase also in SourceLookup.
First rally exeriments show that this speeds up e.g. when runtime fields that are using "_source" are
added e.g. via "docvalue_fields" in the search hits. Using runtime fields that are using "_source" in
queries or aggs isn't affected so far.