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?
There was a problem hiding this comment.
I think you'll get different results sometimes, i can't remember now.
| 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 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?
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
4 participants
This also covers
annotated_textbecauseAnnotatedTextFieldTypeinheritsTextFieldType.