ValueFetchers should access all values via fetchValues()
#69137
Conversation
WIP - still failures in InnerHitsIT
| .startArray("field1"); | ||
| for (int j = 0; j < numInnerObjects; j++) { | ||
| source.startObject().field("x", "y").endObject(); | ||
| source.startObject().field("x", "y" + i + ":" + j).endObject(); |
There was a problem hiding this comment.
I used these to debug some deep weirdness with how InnerHits uses its lookups, but they're useful so I think they can stay
| /** | ||
| * Value fetcher that loads from doc values. | ||
| */ | ||
| public final class DocValueFetcher implements ValueFetcher { |
There was a problem hiding this comment.
Essentially all of this class is now moved into ValuesLookup.docValues() so it might be worth just removing it and replacing it with lambdas wherever it is used.
| return parseLong(value, roundUp, now); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
We use DocValueFormat as part of a hash key now (so that a lookup can access the same field with different formats and still use caching); this was the only impl with no hashCode()/equals()
| return searchContext.searcher(); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This is now moved to the wrapping SearchContext, so that inner hits can have their own version of the lookup that filters the source appropriately.
| // Store the loaded source on the hit context so that fetch subphases can access it. | ||
| // Also make it available to scripts by storing it on the shared SearchLookup instance. | ||
| hitContext.sourceLookup().setSource(fieldsVisitor.source()); | ||
| hitContext.valuesLookup().source().setSource(fieldsVisitor.source()); |
There was a problem hiding this comment.
This is still pretty ugly - would be nice to more stored fields access into ValuesLookup as well so that the lookup can handle access to everything and we don't need to set the source separately - but that's a much bigger change and is not necessary for the current project.
| public void setNextReader(LeafReaderContext readerContext) { | ||
| for (DocValueField f : fields) { | ||
| f.fetcher.setNextReader(readerContext); | ||
| } |
There was a problem hiding this comment.
Handled at the top-level FetchPhase now
| } | ||
|
|
||
| @Override | ||
| public SearchLookup getSearchLookup() { |
There was a problem hiding this comment.
Slightly tricksy; inner hits goes and runs a new FetchPhase for each document, and because positioning of the ValuesLookup is now done by the FetchPhase we need to have a new lookup for each inner hits context so that documents with multiple nested types don't interfere with each other. We still only have one lookup per nested type throughout the phase so the overhead is kept low.
| @Override | ||
| public FetchSubPhaseProcessor getProcessor(FetchContext searchContext) { | ||
| if (searchContext.innerHits() == null) { | ||
| if (searchContext.innerHits() == null || searchContext.innerHits().getInnerHits().isEmpty()) { |
There was a problem hiding this comment.
Not really related but I saw we were running inner hits for every doc even if none were asked for because by default we return an empty list here, not null.
| } | ||
| } | ||
|
|
||
| public ScriptDocValues<?> get(String name, DocValueFormat format) { |
There was a problem hiding this comment.
This is largely analogous to get(field) below, with the addition of formatting. The FormatKey is so that a fields call can request the same field multiple times with different formats.
1 participant
We currently have two major implementations of ValueFetcher:
SourceLookuppassed directly to
ValueFetcher.fetchValues()SearchLookuppassed in viathe QueryExecutionContext when the fetcher is built. This also requires
a separate method to be called when moving between segments.
This split confuses the API, and the second implementation in particular will
cause problems if we try and use these fetchers to build index-time scripts.
This commit adds a new interface,
ValuesLookup, which wraps bothsourceand
doc, and changes the signature ofValueFetcher.fetchValues()toaccept this in place of
SourceLookup. Positioning of this ValuesLookup isthe responsibility of the caller, meaning that we can remove the
setNextReadermethod on
ValueFetcher. Access to formatted doc values is moved intoLeafDocLookup.Relates to #68984