Clarify SourceLookup sharing across fetch subphases. #60179

jtibshirani · 2020-07-27T00:12:59Z

The SourceLookup class provides access to the _source for a particular
document, specified through SourceLookup#setSegmentAndDocument. Previously
the search context contained a single SourceLookup that was shared between
different fetch subphases. It was hard to reason about its state: is
SourceLookup set to the expected document? Is the _source already loaded and
available?

Instead of using a global source lookup, the fetch hit context now provides
access to a lookup that is set to load from the hit document.

This refactor closes #31000, since the same SourceLookup is no longer shared
between the 'fetch _source phase' and script execution.

elasticmachine · 2020-07-27T16:20:44Z

Pinging @elastic/es-search (:Search/Search)

jtibshirani · 2020-07-27T18:19:13Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/InnerHitsPhase.java

@@ -65,10 +65,6 @@ public void hitsExecute(SearchContext context, SearchHit[] hits) throws IOExcept
                }
                innerHits.docIdsToLoad(docIdsToLoad, 0, docIdsToLoad.length);
                innerHits.setId(hit.getId());
-                innerHits.lookup().source().setSource(context.lookup().source().internalSourceRef());


We set the root document's _source here, but when processing inner hits we didn't use it and instead reloaded the _source. So this is safe to remove for now.

In a follow-up PR, I will fix this to make sure the _source is loaded only once for the root doc + inner hits.

+1, that's something that we spotted sometime ago: #32818 (comment) and maybe #56210 but never implemented.

server/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.java

mayya-sharipova · 2020-07-28T02:13:12Z

@jtibshirani Thanks for the PR, I like the idea. I had an initial look, and will look more thoroughly tomorrow.

...ava/org/elasticsearch/search/aggregations/bucket/terms/SignificantTextAggregatorFactory.java

This saved _source actually never appears to be used.

server/src/main/java/org/elasticsearch/search/lookup/SearchLookup.java

jtibshirani · 2020-07-28T19:36:30Z

I caught up with @nik9000 offline about the refactor and wanted to add more context as to its benefits:

The _source loading semantics are clearer. We attempt to load the _source once at the start of the fetch phase, and explicitly add it to the HitContext to make it available for the subphases. Before we manipulated global state (setting the _source on SearchContext), and it was not obvious what the intention was.
Any subphase that overrides hitExecute (singular) will have access to the hit context, which contains a reference to the _source for the current document. The subphases that override hitsExecute (plural) do not have access to a hit context and need to reload _source. Previously this difference in behavior was unclear.

However I now see a downside to this refactor. There used to be only one way to share a SourceLookup instance (as part of SearchLookup on QueryShardContext), but now we've added another specific to the fetch phase. This means that a subphase that overwrites hitExecute and also uses a script will not share the loaded _source.

I'm curious as to what others think. I still like the refactor and feel that it moves us in a better direction.

server/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.java

server/src/main/java/org/elasticsearch/search/lookup/SearchLookup.java

jimczi · 2020-07-28T19:55:23Z

However I now see a downside to this refactor. There used to be only one way to share a SourceLookup instance (as part of SearchLookup on QueryShardContext), but now we've added another specific to the fetch phase. This means that a subphase that overwrites hitExecute and also uses a script will not share the loaded _source.

We don't have such example at the moment, right ? That's also something we can tackle in a follow up, currently we use the lookup of the query shard context to compile the scripted field but we could refactor this part to move to the new hit context ? Hits are now sorted by doc ids in fetch sub phases so it's also a good chance to get ride of the hitsExecute completely. That would allow to load the _source only once since today sub phases that use hitsExecute have no way to reuse _source ?

jimczi

I like this change. I left one comment regarding a possible hack to ensure that scripts and fetch sub phase can still share the _source but I don't feel strongly about it. We can solve the double load more nicely in a follow up if needed.

jimczi · 2020-07-29T20:18:06Z

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

-        SourceLookup sourceLookup = context.lookup().source();
-        sourceLookup.setSegmentAndDocument(subReaderContext, subDocId);
-        if (fieldsVisitor.source() != null) {
-            sourceLookup.setSource(fieldsVisitor.source());


I wonder if we can still assign the context.lookup().source() with the current _source so that _script can share the source with other fetch sub-phase. That would only work for sub-phases that use a script and override hitExecute so the new FetchFieldsPhase would benefit from this hack ? We can then think how we could clenup the context for script in a follow up ?

I don't think FetchFieldsPhase would benefit currently, because it doesn't support scripts? I like the plan of considering it in a follow-up, for example as part of converting ScriptFieldsPhase to use hitExecute.

Right, I was thinking preemptively of runtime scripted field but +1 to consider this in a follow up.

jimczi · 2020-07-29T20:24:35Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/InnerHitsPhase.java

@@ -65,10 +65,6 @@ public void hitsExecute(SearchContext context, SearchHit[] hits) throws IOExcept
                }
                innerHits.docIdsToLoad(docIdsToLoad, 0, docIdsToLoad.length);
                innerHits.setId(hit.getId());
-                innerHits.lookup().source().setSource(context.lookup().source().internalSourceRef());


+1, that's something that we spotted sometime ago: #32818 (comment) and maybe #56210 but never implemented.

...java/org/elasticsearch/search/fetch/subphase/highlight/SourceScoreOrderFragmentsBuilder.java

...ain/java/org/elasticsearch/search/fetch/subphase/highlight/SourceSimpleFragmentsBuilder.java

They are not closely related to this PR and it's clearer to keep them separate.

jtibshirani · 2020-07-29T21:17:40Z

In addition to restoring the shared SourceLookup on SearchLookup, I ended up removing all changes to the SearchLookup class, as they weren't core to this PR. So now the PR is scoped just to 'fetch' and avoids @nik9000 and @mayya-sharipova's concern around scripts sharing _source in the query phase.

Here is the final summary of changes:

Store SourceLookup on HitContext and set the source when creating hits.
Fetch subphases access source through HitContext when possible.
Stop saving parent _source before loading inner hits (it was unused).
Remove SearchContext#lookup method.

mayya-sharipova

@jtibshirani Thanks Julie, looks a very good change to me!

nik9000

LGTM2

jtibshirani · 2020-07-30T18:57:12Z

Thanks everyone, the review feedback was really helpful.

This PR fixes a regression where fvh fragments could be loaded from the wrong document _source. Some `FragmentsBuilder` implementations contain a `SourceLookup` to load from _source. The lookup should be positioned to load from the current hit document. However, since `FragmentsBuilder` are cached and shared across hits, the lookup is never updated to load from the new documents. This means we accidentally load _source from a different document. The regression was introduced in #60179, which started storing `SourceLookup` on `FragmentsBuilder`. Fixes #65533.

…5641) This PR fixes a regression where fvh fragments could be loaded from the wrong document _source. Some `FragmentsBuilder` implementations contain a `SourceLookup` to load from _source. The lookup should be positioned to load from the current hit document. However, since `FragmentsBuilder` are cached and shared across hits, the lookup is never updated to load from the new documents. This means we accidentally load _source from a different document. The regression was introduced in elastic#60179, which started storing `SourceLookup` on `FragmentsBuilder`. Fixes elastic#65533.

This PR fixes a regression where fvh fragments could be loaded from the wrong document _source. Some `FragmentsBuilder` implementations contain a `SourceLookup` to load from _source. The lookup should be positioned to load from the current hit document. However, since `FragmentsBuilder` are cached and shared across hits, the lookup is never updated to load from the new documents. This means we accidentally load _source from a different document. The regression was introduced in #60179, which started storing `SourceLookup` on `FragmentsBuilder`. Fixes #65533.

…5641) This PR fixes a regression where fvh fragments could be loaded from the wrong document _source. Some `FragmentsBuilder` implementations contain a `SourceLookup` to load from _source. The lookup should be positioned to load from the current hit document. However, since `FragmentsBuilder` are cached and shared across hits, the lookup is never updated to load from the new documents. This means we accidentally load _source from a different document. The regression was introduced in elastic#60179, which started storing `SourceLookup` on `FragmentsBuilder`. Fixes elastic#65533.

This PR fixes a regression where fvh fragments could be loaded from the wrong document _source. Some `FragmentsBuilder` implementations contain a `SourceLookup` to load from _source. The lookup should be positioned to load from the current hit document. However, since `FragmentsBuilder` are cached and shared across hits, the lookup is never updated to load from the new documents. This means we accidentally load _source from a different document. The regression was introduced in #60179, which started storing `SourceLookup` on `FragmentsBuilder`. Fixes #65533.

jtibshirani force-pushed the source-lookup branch 3 times, most recently from 58a7d72 to aca5233 Compare July 27, 2020 02:04

jtibshirani added :Search/Search Search-related issues that do not fall into other categories >refactoring v7.10.0 v8.0.0 labels Jul 27, 2020

jtibshirani marked this pull request as ready for review July 27, 2020 16:20

elasticmachine added the Team:Search Meta label for search team label Jul 27, 2020

jtibshirani commented Jul 27, 2020

View reviewed changes

jtibshirani mentioned this pull request Jul 27, 2020

_source not returned in "explain": true with certain queries #31000

Closed

jtibshirani requested review from mayya-sharipova and jimczi July 27, 2020 18:34

mayya-sharipova reviewed Jul 28, 2020

View reviewed changes

server/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.java Show resolved Hide resolved

nik9000 reviewed Jul 28, 2020

View reviewed changes

...ava/org/elasticsearch/search/aggregations/bucket/terms/SignificantTextAggregatorFactory.java Outdated Show resolved Hide resolved

jtibshirani added 4 commits July 28, 2020 11:09

Create new SourceLookup for SignificantTextAggregator.

6362be8

Store SourceLookup on HitContext and set the source when creating hits.

715e762

In highlighting, access source through HitContext.

3a17197

Stop saving parent _source before loading inner hits.

53bd276

This saved _source actually never appears to be used.

mayya-sharipova reviewed Jul 28, 2020

View reviewed changes

server/src/main/java/org/elasticsearch/search/lookup/SearchLookup.java Outdated Show resolved Hide resolved

jtibshirani added 2 commits July 28, 2020 12:22

Remove SearchContext#lookup.

9419b2b

Fix null pointer in FetchSourcePhaseTests.

0b730e9

nik9000 reviewed Jul 28, 2020

View reviewed changes

jtibshirani added 2 commits July 28, 2020 13:10

Restore shared SourceLookup to SearchLookup.

22ea748

Add comments explaining source loading + sharing.

174edb6

jtibshirani force-pushed the source-lookup branch from aca5233 to 174edb6 Compare July 28, 2020 20:40

jimczi approved these changes Jul 29, 2020

View reviewed changes

jtibshirani added 3 commits July 29, 2020 13:51

Merge remote-tracking branch 'upstream/master' into source-lookup

e43c4e0

Undo all changes to SearchLookup class.

99c6461

They are not closely related to this PR and it's clearer to keep them separate.

Avoid passing whole HitContext to the highlight fragments builders.

8ef525b

jtibshirani added 3 commits July 29, 2020 14:48

Remove unnecessary method mock in AggregatorTestCase.

a61ce6c

Fix checkstyle violation.

e0b5d5f

Undo the change to SourceLookup sharing in SIgnificantTextAggregator.

b4888ef

mayya-sharipova approved these changes Jul 30, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into source-lookup

be6cfe6

nik9000 approved these changes Jul 30, 2020

View reviewed changes

jtibshirani merged commit 9d28328 into elastic:master Jul 30, 2020

jtibshirani deleted the source-lookup branch July 30, 2020 18:57

jtibshirani mentioned this pull request Jul 30, 2020

Clarify SourceLookup sharing across fetch subphases. #60484

Merged

jtibshirani added the >bug label Jul 30, 2020

jtibshirani mentioned this pull request Sep 9, 2020

Implement fields fetch for runtime fields #61995

Merged

jtibshirani mentioned this pull request Nov 30, 2020

Fix bug where fvh fragments could be loaded from wrong doc #65641

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

ywelsch mentioned this pull request May 17, 2022

Elasticsearch inner_hits is very slow #56210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify SourceLookup sharing across fetch subphases. #60179

Clarify SourceLookup sharing across fetch subphases. #60179

jtibshirani commented Jul 27, 2020 •

edited

Loading

elasticmachine commented Jul 27, 2020

jtibshirani Jul 27, 2020

jimczi Jul 29, 2020

mayya-sharipova commented Jul 28, 2020

jtibshirani commented Jul 28, 2020 •

edited

Loading

jimczi commented Jul 28, 2020

jimczi left a comment

jimczi Jul 29, 2020

jtibshirani Jul 29, 2020

jimczi Jul 29, 2020

jimczi Jul 29, 2020

jtibshirani commented Jul 29, 2020

mayya-sharipova left a comment

nik9000 left a comment

jtibshirani commented Jul 30, 2020

Clarify SourceLookup sharing across fetch subphases. #60179

Clarify SourceLookup sharing across fetch subphases. #60179

Conversation

jtibshirani commented Jul 27, 2020 • edited Loading

elasticmachine commented Jul 27, 2020

jtibshirani Jul 27, 2020

Choose a reason for hiding this comment

jimczi Jul 29, 2020

Choose a reason for hiding this comment

mayya-sharipova commented Jul 28, 2020

jtibshirani commented Jul 28, 2020 • edited Loading

jimczi commented Jul 28, 2020

jimczi left a comment

Choose a reason for hiding this comment

jimczi Jul 29, 2020

Choose a reason for hiding this comment

jtibshirani Jul 29, 2020

Choose a reason for hiding this comment

jimczi Jul 29, 2020

Choose a reason for hiding this comment

jimczi Jul 29, 2020

Choose a reason for hiding this comment

jtibshirani commented Jul 29, 2020

mayya-sharipova left a comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

jtibshirani commented Jul 30, 2020

jtibshirani commented Jul 27, 2020 •

edited

Loading

jtibshirani commented Jul 28, 2020 •

edited

Loading