Use FallbackSyntheticSourceBlockLoader for text fields #126237
Conversation
elasticsearchmachine
added
v9.1.0
needs:triage
| // See TextFieldMapper.SyntheticSourceHelper#usingSyntheticSourceDelegate | ||
| // and TextFieldMapper#canUseSyntheticSourceDelegateForLoading(). | ||
| boolean usingSyntheticSourceDelegate = docValues || store; | ||
| boolean canUseSyntheticSourceDelegateForLoading = usingSyntheticSourceDelegate && ignoreAbove == null && (index || store); |
There was a problem hiding this comment.
I don't really understand why do we require multi field to be indexed here (the actual impl is in TextFieldMapper).
There was a problem hiding this comment.
Yes, that is strange (only doc values / stored should matter). What failure occurs when we don't check for index here?
| return KeywordFieldBlockLoaderTests.expectedValue(keywordMultiFieldMapping, value, params, testContext); | ||
| } | ||
|
|
||
| // Even if multi-field is not eligible for loading it can still be used to produce synthetic source |
There was a problem hiding this comment.
This whole thing is due to the fact that not all syntheticSourceDelegate are eligible to be used by block loader.
| // KeywordFieldBlockLoaderTest | ||
| // It is here since KeywordFieldBlockLoaderTest does not really need it | ||
| if (params.syntheticSource() && testContext.forceFallbackSyntheticSource() == false && usingSyntheticSourceDelegate) { | ||
| var nullValue = (String) keywordMultiFieldMapping.get("null_value"); |
There was a problem hiding this comment.
If syntheticSourceDelegate is used than null_value of the multi field will be in the resulting synthetic source. This is good because we'll preserve such the values during reindex. On the other hand this is inconsistent - synthetic source for text behaves differently depending on the presence of multi field and mapping parameters of multi field (text ignores nulls).
| if (params.syntheticSource() && testContext.forceFallbackSyntheticSource() == false && usingSyntheticSourceDelegate) { | ||
| var nullValue = (String) keywordMultiFieldMapping.get("null_value"); | ||
|
|
||
| // Due to how TextFieldMapper#blockReaderDisiLookup works this is complicated. |
There was a problem hiding this comment.
Another inconsistency - block loader returns a null_value from multi field instead of null only if text field itself is not indexed. If it is, it returns nothing.
lkts
added
>enhancement
auto-backport
elasticsearchmachine
added
Team:StorageEngine
and removed
needs:triage
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
Hi @lkts, I've created a changelog YAML for you. |
| // See TextFieldMapper.SyntheticSourceHelper#usingSyntheticSourceDelegate | ||
| // and TextFieldMapper#canUseSyntheticSourceDelegateForLoading(). | ||
| boolean usingSyntheticSourceDelegate = docValues || store; | ||
| boolean canUseSyntheticSourceDelegateForLoading = usingSyntheticSourceDelegate && ignoreAbove == null && (index || store); |
There was a problem hiding this comment.
Yes, that is strange (only doc values / stored should matter). What failure occurs when we don't check for index here?
This also covers
annotated_textbecauseAnnotatedTextFieldTypeinheritsTextFieldType.