ValueFetchers now return a StoredFieldsSpec #94820
Conversation
romseygeek
added
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
iverase
left a comment
iverase
left a comment
There was a problem hiding this comment.
I did a first pass and looks good to me. I think there is a potential performance bug in PreloadedFieldLookupProvider that needs to be addressed.
| public void populateFieldLookup(FieldLookup fieldLookup, int doc) throws IOException { | ||
| String field = fieldLookup.fieldType().name(); | ||
| if (storedFields.containsKey(field)) { | ||
| fieldLookup.setValues(storedFields.get(field)); |
There was a problem hiding this comment.
Should we return here if the stored field is preloaded? If not we are always loading it with the backupLoader.
There was a problem hiding this comment.
Oops, yes, good spot... I'll add some better testing here as well.
| PreloadedSourceProvider sourceProvider = new PreloadedSourceProvider(); | ||
| context.getSearchExecutionContext().setSourceProvider(sourceProvider); | ||
| PreloadedFieldLookupProvider fieldLookupProvider = new PreloadedFieldLookupProvider(); | ||
| context.getSearchExecutionContext().setLookupProviders(sourceProvider, ctx -> fieldLookupProvider); |
There was a problem hiding this comment.
Thinking out loud ... Is really required to treat _source different to any other (stored) field here apart from historical reasons? I guess it is but does it mean we can potentially preload source twice.
There was a problem hiding this comment.
_source might be synthesised rather than come from a stored field so it needs to be handled separately. It might be possible to explicitly ask for _source via _fields which would load things twice, and we can probably intercept that and make sure it doesn't happen. Will add more tests!
There was a problem hiding this comment.
We already have a check for _source in stored fields, which gets ignored.
| @Override | ||
| public StoredFieldsSpec storedFieldsSpec() { | ||
| // TODO can we get finer-grained information from the FieldFetcher for this? | ||
| return StoredFieldsSpec.NEEDS_SOURCE; |
| this.fieldChain = Collections.emptySet(); | ||
| this.sourceProvider = sourceProvider; | ||
| this.fieldDataLookup = fieldDataLookup; | ||
| this.fieldLookupProvider = fieldLookupProvider; |
There was a problem hiding this comment.
Is this used somewhere else?
| new LeafDocLookup(fieldTypeLookup, this::getForField, context), | ||
| sourceProvider, | ||
| new LeafStoredFieldsLookup(fieldTypeLookup, () -> context.reader().storedFields()) | ||
| new LeafStoredFieldsLookup(fieldTypeLookup, LeafFieldLookupProvider.fromStoredFields().apply(context)) |
There was a problem hiding this comment.
I wonder if you mean here to use fieldLookupProvider?
|
@elasticmachine run elasticsearch-ci/bwc |
iverase
left a comment
iverase
left a comment
There was a problem hiding this comment.
LGTM! learnt quite a bit while reviewing it.
|
|
||
| private void addValue(Object value) { | ||
| destination.add(field.valueForDisplay(value)); | ||
| destination.add(value); |
There was a problem hiding this comment.
Just for my curiosity, Was this call (#valueForDisplay) not necessary?
There was a problem hiding this comment.
It's moved into FieldLookup.setValues(), mainly because there are now two paths that can set it (here in the stored fields provider and also in the fetchphase pre-loaded implementation)
| * Value fetcher that loads from doc values. | ||
| */ | ||
| // TODO rename this? It doesn't load from doc values, it loads from fielddata | ||
| // Which might be doc values, but might not be... |
There was a problem hiding this comment.
Wow, so FormattedDocValues can actually come from a stored field?
|
@elasticmachine update branch |
4 participants
This allows us to be more conservative about what needs to be loaded
when using the
fieldsAPI, and opens up the possibility of avoidingusing stored fields or source altogether if we can use doc values to
fetch values.