Return empty data instead of throwing when Fields API tries to access non-available "fielddata" #99583
Conversation
| @@ -106,7 +111,7 @@ public SearchExecutionContext( | |||
| int shardRequestIndex, | |||
| IndexSettings indexSettings, | |||
| BitsetFilterCache bitsetFilterCache, | |||
| BiFunction<MappedFieldType, FieldDataContext, IndexFieldData<?>> indexFieldDataLookup, | |||
| IndexFieldDataLookup indexFieldDataLookup, | |||
There was a problem hiding this comment.
I thought that introducing an interface here instead of adding another BiFunction would have made things clearer, but I have second thoughts about it. See SearchLookup for contrast, where instead I introduced an additional function
| */ | ||
| public SearchLookup( | ||
| Function<String, MappedFieldType> fieldTypeLookup, | ||
| TriFunction<MappedFieldType, Supplier<SearchLookup>, MappedFieldType.FielddataOperation, IndexFieldData<?>> fieldDataLookup, | ||
| TriFunction<MappedFieldType, Supplier<SearchLookup>, MappedFieldType.FielddataOperation, Boolean> fieldDataPredicate, |
There was a problem hiding this comment.
Here I introduced a second TriFunction instead of an interface that includes both the lookup and the predicate - see SearchExecutionContext for that approach.
I'd say we keep one for consistency, but it's not clear to me which one is more readable so I kept both and I am asking for feedback.
| @@ -660,7 +661,17 @@ public SearchExecutionContext newSearchExecutionContext( | |||
| shardRequestIndex, | |||
| indexSettings, | |||
| indexCache.bitsetFilterCache(), | |||
| indexFieldData::getForField, | |||
| new SearchExecutionContext.IndexFieldDataLookup() { | |||
There was a problem hiding this comment.
Here is where the single interface with both lookup and predicate methods is "worse": the anonymous class is not brilliant to read.
If we like the single interface, I would like to extract a static inner class (adapter) to make it nicer (more readable). Thoughts?
| @@ -100,6 +104,23 @@ private FieldFactoryWrapper getFactoryForField(String fieldName) { | |||
| throw new IllegalArgumentException("No field found for [" + fieldName + "] in mapping"); | |||
| } | |||
|
|
|||
| if (fieldDataPredicate.apply(fieldType, SCRIPT) == false) { | |||
| return new FieldFactoryWrapper(new DocValuesScriptFieldFactory() { | |||
There was a problem hiding this comment.
This is temporary; if we are happy with this approach, I would suggest to make FieldFactoryWrapper an interface with 2 concrete implementations, one equivalent to the concrete class now, one "empty". Hiding the factory, etc.
But there is a preliminary Q on this point: I see the factory wrapper is (can) shared with doc-API access and cached. Should we avoid it? Should we pull the check one level up (inside getScriptField), avoid the wrapper and the cache altogether? (I think it might be wrong to cache it, as the predicate could return different answers based on context).
I wrote it like this before realizing the predicate could change based on context; maybe the cache is for the captured context only, so no problem, but better check and in doubt move this out.
ldematte
changed the title
Address the problem highlighted in #94403
The PR is unfortunately large because I went through to make the change complete and buildable (fixing all the tests and mocks too). For review, I think the best way is to concentrate on 5 files/classes:
MappedFieldType.javaSearchExecutionContext.java, the constructor and how it is used inIndexService.java(I added a self-comment there)SearchLookup.java- I added a self-comment there as well, to request inputLeafDocLookup.javain the code path specific for field-API accessFieldFactoryWrapperalternatives to return anEmptyField- either make it better or remove it/move things one level up.After exploring the interactions between context, docs, fielddata etc. I think that the best way to solve the issue is if there is a way for
MappedFieldTypes to expose if they can read fielddata. This way the field API can avoid using the fielddata builder (and throwing), and return anEmptyFieldinstead.To do this, I added a
public boolean isFielddataSupported(FieldDataContext fieldDataContext)toMappedFieldType; exposing it toLeafDocLookupwith the appropriate context required a bit of propagation, throughIndexService,SearchExecutionContextandSearchLookupand the implementation of the new method in everyMappedFieldTypederivate class. In the end however code changes were simple, but spread across the codebase.Closes #94403