Consolidate all access to document values through SearchLookup

[romseygeek]

Followup to #62511

The various Fetch subphases need to access a number of document-specific values: stored fields, docvalue fields, _source, script outputs. This is currently done via a mix of access to a SearchLookup object (for docvalue fields, _source and script) and a special fields visitor for stored fields. The stored fields logic also handles loading source, and is dealt with specially in FetchPhase. Nested documents also handle loading source differently, and some of this logic is in FetchPhase. We then have value fetchers, which are built by passing a QueryShardContext (which refers to a SearchLookup) and then used by passing a SourceLookup. This all makes things very difficult to reason about, as responsibility for loading data, caching across multiple calls, and advancing to new documents is split across several different classes.

I'd like to try consolidating all access to document values to be through SearchLookup.

• StoredFieldsLookup should be able to load more than one field at a time (currently we do everything via a SingleFieldVisitor) and should make use of the fast stored fields reader where possible
• SourceLookup should use StoredFieldsLookup, again to reduce the number of calls to IndexReader#document
• SourceLookup should be aware of nested mappings and automatically return the correct source when positioned on nested or root documents
• Adding stored fields to a search hit should be done via a dedicated fetch sub phase that reads data from SearchLookup
• Positioning of the search lookup should be done by whatever owns it - probably the root collector in the query phase, or the FetchPhase main loop in the fetch phase.
• ValueFetchers should load data via a passed-in SearchLookup and not have to worry about settings reader contexts.

I think organising things in this way will make reasoning about how data is fetched and which classes have responsibility for what much easier.

[elasticmachine]

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

[romseygeek]

cc @jtibshirani @cbuescher

[jtibshirani]

Would this consolidation cover doc value fields as well? I wonder if we should remove QueryShardContext#getForField and instead have callers use SearchLookup.

[romseygeek]

That makes sense to me @jtibshirani

[jimczi]

StoredFieldsLookup should be able to load more than one field at a time (currently we do everything via a SingleFieldVisitor) and should make use of the fast stored fields reader where possible

SourceLookup should use StoredFieldsLookup, again to reduce the number of calls to IndexReader#document

I have some concerns regarding the proposal for stored fields. Today they are used marginally and we are looking at ways to remove them entirely. In practice, using a stored field other than _source in a script is discouraged, so I wonder why the optimization presented here would be helpful.
I like the separation between source lookup and stored fields lookup, I don't think they should use the same path. Source lookup can be optimized in many ways so I'd prefer to look on this side rather than optimizing for an edge case that we want to remove in the near future (stored fields).

I think organising things in this way will make reasoning about how data is fetched and which classes have responsibility for what much easier.

It's a big refactoring so I wonder what other benefits we're seeing here. I agree that some parts are hard to follow if you don't know the contract (nested fields access or positioning the lookup objects) but I think it would be beneficial to separate the concerns a bit. The nested context for instance is complex and not well tested so any change can break some esoteric use cases.
I understand the need to clean up the code but I'd prefer to see a good reason to do so that links to a specific feature. For instance improving source lookups seems very important with the introduction of runtime fields.
That should help in terms of prioritization and for the evaluation of the risk.

[romseygeek]

I understand the need to clean up the code but I'd prefer to see a good reason to do so that links to a specific feature. For instance improving source lookups seems very important with the introduction of runtime fields.
That should help in terms of prioritization and for the evaluation of the risk.

The initial motivation is to make runtime fields work with nested documents. SourceLookup doesn't know anything about nested documents, so we handle this at the moment during Fetch by externally setting a value onto the lookup if the current doc is nested. If we are fetching several docs from the same root, but are not using an innerhits context, we end up loading the root document several times - not too much of an issue when you're only looking at 10 docs, but possibly very inefficient in the context of a runtime field. This external mutability makes reasoning about what is actually contained within a source lookup difficult (anything with access to it could set it to something random). It also doesn't really work well with runtime field scripts, which just want to advance to a given document. So:

• SourceLookup should be aware of nested mappings and automatically return the correct source when positioned on nested or root documents

SourceLookup requires loading from stored fields, and we'd like to do this as efficiently as possible for runtime fields - also for the fetch phase, although that's less of an issue generally. We also load _id, _ignored and _routing metadata fields from stored fields, and plugins can add their own metadata fields to be loaded here. This information needs to be loaded from stored fields because it is separate from the _source, so whatever happens with the stored option on mapped fields, we will still need to have a way to load arbitrary data from stored fields, and we will want this to happen efficiently. Highlighting can also make use of loading data from stored fields to avoid parsing source, and this currently doesn't make use of the fast sequential reader. Rather than chasing down everywhere that uses a CustomFieldsVisitor and checking to see if it can use the sequential reader, I think it makes more sense to consolidate this into a single class that handles loading of data from stored fields. And once we're doing that, it makes sense to make use of the class that already basically does all this (except inefficiently), which is LeafStoredFieldsLookup. Which is why I wanted to start with improving the efficiency of LeafStoredFieldsLookup, and getting the other parts of the system which load data from stored fields to use it.

@jtibshirani commented elsewhere that she's worried about the interfaces getting confused here, and I agree with that - we don't really need to expose all this stuff as maps outside of a scripting context. So maybe we need something that's separate from the Lookup objects we already have.

The final parts are mainly about making it easier to unit test the FetchPhase. At the moment, loading stored fields and metadata, and filtering source for nested documents are all done in the FetchPhase class itself; we have pluggable sub phase processors, which would be an obvious interface to build unit tests around, but they can't work in isolation because of all this work done in FetchPhase. So in order to beef up testing I think we should re-organise things a bit here.

Re-reading what I've written, maybe it makes more sense to split this into two sub-issues; one for handling nested documents in runtime fields, and one for improving the testability of the fetch phase?