-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Set new providers before building FetchSubPhaseProcessors #97460
Conversation
Pinging @elastic/es-analytics-geo (Team:Analytics) |
Hi @romseygeek, I've created a changelog YAML for you. |
Pinging @elastic/es-search (Team:Search) |
Bad bot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a lot for tracking this down! I left a comment on testing but no need for another review.
@@ -100,6 +100,10 @@ private SearchHits buildSearchHits(SearchContext context, Profiler profiler) { | |||
FetchContext fetchContext = new FetchContext(context); | |||
SourceLoader sourceLoader = context.newSourceLoader(); | |||
|
|||
PreloadedSourceProvider sourceProvider = new PreloadedSourceProvider(); | |||
PreloadedFieldLookupProvider fieldLookupProvider = new PreloadedFieldLookupProvider(); | |||
context.getSearchExecutionContext().setLookupProviders(sourceProvider, ctx -> fieldLookupProvider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to add a unit test as well? I guess it really has to do with how top hits interacts with the fetch phase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, unit tests for this will be tricky. We don't really have unit tests for any of the FetchPhase at the moment as the context structures that it uses are so heavy.
💚 Backport successful
|
) The FetchPhase sets source and field providers on its SearchLookup instance to ensure that we only have to load stored fields once per document. However, these were being set after subprocessors were constructed, and the FetchFieldsPhase subprocessor pulls a reference to the SearchLookup at construction time. This meant that the FieldsFetcher was not using these providers, instead using the standard source and fields lookup, in effect meaning that fields access would load source and fields separately. This would mostly just be an issue with performance; however, the top hits aggregation calls the fetch phase multiple times, with the result that the second and subsequent buckets in a top hits aggregation would see the source lookup from the previous bucket when building their FetchFieldsPhase subprocessor. This commit moves the call to SearchExecutionContext#setLookupProviders() in FetchPhase#buildSearchHits() before the call to build fetch phase subprocessors, ensuring that the fetch fields phase sees the correct SearchLookup. Fixes elastic#96284
…97500) The FetchPhase sets source and field providers on its SearchLookup instance to ensure that we only have to load stored fields once per document. However, these were being set after subprocessors were constructed, and the FetchFieldsPhase subprocessor pulls a reference to the SearchLookup at construction time. This meant that the FieldsFetcher was not using these providers, instead using the standard source and fields lookup, in effect meaning that fields access would load source and fields separately. This would mostly just be an issue with performance; however, the top hits aggregation calls the fetch phase multiple times, with the result that the second and subsequent buckets in a top hits aggregation would see the source lookup from the previous bucket when building their FetchFieldsPhase subprocessor. This commit moves the call to SearchExecutionContext#setLookupProviders() in FetchPhase#buildSearchHits() before the call to build fetch phase subprocessors, ensuring that the fetch fields phase sees the correct SearchLookup. Fixes #96284
The FetchPhase sets source and field providers on its SearchLookup instance
to ensure that we only have to load stored fields once per document. However,
these were being set after subprocessors were constructed, and the
FetchFieldsPhase subprocessor pulls a reference to the SearchLookup at
construction time. This meant that the FieldsFetcher was not using these
providers, instead using the standard source and fields lookup, in effect meaning
that
fields
access would load source and fields separately.This would mostly just be an issue with performance; however, the top hits
aggregation calls the fetch phase multiple times, with the result that the second
and subsequent buckets in a top hits aggregation would see the source lookup
from the previous bucket when building their FetchFieldsPhase subprocessor.
This commit moves the call to
SearchExecutionContext#setLookupProviders()
in
FetchPhase#buildSearchHits()
before the call to build fetch phase subprocessors,ensuring that the fetch fields phase sees the correct SearchLookup.
Fixes #96284