Use FallbackSyntheticSourceBlockLoader for text fields by lkts · Pull Request #126237 · elastic/elasticsearch

lkts · 2025-04-03T17:16:45Z

This also covers annotated_text because AnnotatedTextFieldType inherits TextFieldType.

lkts · 2025-04-03T17:17:52Z

server/src/test/java/org/elasticsearch/index/mapper/blockloader/TextFieldBlockLoaderTests.java

+            // See TextFieldMapper.SyntheticSourceHelper#usingSyntheticSourceDelegate
+            // and TextFieldMapper#canUseSyntheticSourceDelegateForLoading().
+            boolean usingSyntheticSourceDelegate = docValues || store;
+            boolean canUseSyntheticSourceDelegateForLoading = usingSyntheticSourceDelegate && ignoreAbove == null && (index || store);


I don't really understand why do we require multi field to be indexed here (the actual impl is in TextFieldMapper).

Yes, that is strange (only doc values / stored should matter). What failure occurs when we don't check for index here?

I think you'll get different results sometimes, i can't remember now.

lkts · 2025-04-03T17:19:17Z

server/src/test/java/org/elasticsearch/index/mapper/blockloader/TextFieldBlockLoaderTests.java

+                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


This whole thing is due to the fact that not all syntheticSourceDelegate are eligible to be used by block loader.

lkts · 2025-04-03T17:22:42Z

server/src/test/java/org/elasticsearch/index/mapper/blockloader/TextFieldBlockLoaderTests.java

+            // 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");


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).

lkts · 2025-04-03T17:24:37Z

server/src/test/java/org/elasticsearch/index/mapper/blockloader/TextFieldBlockLoaderTests.java

+            if (params.syntheticSource() && testContext.forceFallbackSyntheticSource() == false && usingSyntheticSourceDelegate) {
+                var nullValue = (String) keywordMultiFieldMapping.get("null_value");
+
+                // Due to how TextFieldMapper#blockReaderDisiLookup works this is complicated.


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.

elasticsearchmachine · 2025-04-03T18:59:36Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

elasticsearchmachine · 2025-04-03T18:59:36Z

Hi @lkts, I've created a changelog YAML for you.

dnhatn

LGTM. Thanks Sasha!

martijnvg

LGTM 👍

martijnvg · 2025-04-04T06:59:02Z

server/src/test/java/org/elasticsearch/index/mapper/blockloader/TextFieldBlockLoaderTests.java

+            // See TextFieldMapper.SyntheticSourceHelper#usingSyntheticSourceDelegate
+            // and TextFieldMapper#canUseSyntheticSourceDelegateForLoading().
+            boolean usingSyntheticSourceDelegate = docValues || store;
+            boolean canUseSyntheticSourceDelegateForLoading = usingSyntheticSourceDelegate && ignoreAbove == null && (index || store);


Yes, that is strange (only doc values / stored should matter). What failure occurs when we don't check for index here?

elasticsearchmachine · 2025-04-07T16:33:55Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 126237

lkts · 2025-04-07T20:00:40Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

…6430) (cherry picked from commit 21ff72b) # Conflicts: # server/src/test/java/org/elasticsearch/index/mapper/blockloader/KeywordFieldBlockLoaderTests.java

lkts added 2 commits April 2, 2025 08:33

wip

3576d9a

Use FallbackSyntheticSourceBlockLoader for text fields

3b1f671

lkts requested review from dnhatn, martijnvg and not-napoleon April 3, 2025 17:16

elasticsearchmachine added v9.1.0 needs:triage Requires assignment of a team area label labels Apr 3, 2025

lkts commented Apr 3, 2025

View reviewed changes

lkts added >enhancement auto-backport Automatically create backport pull requests when merged :StorageEngine/Mapping The storage related side of mappings v8.19.0 labels Apr 3, 2025

elasticsearchmachine added Team:StorageEngine and removed needs:triage Requires assignment of a team area label labels Apr 3, 2025

lkts and others added 3 commits April 3, 2025 11:59

Update docs/changelog/126237.yaml

197c3f7

Merge branch 'main' into synthetic_source_block_loader_text

2cb9b13

Merge branch 'main' into synthetic_source_block_loader_text

e1a3663

dnhatn approved these changes Apr 3, 2025

View reviewed changes

martijnvg approved these changes Apr 4, 2025

View reviewed changes

lkts merged commit 21ff72b into elastic:main Apr 7, 2025
17 checks passed

lkts deleted the synthetic_source_block_loader_text branch April 7, 2025 16:32

elasticsearchmachine added the backport pending label Apr 7, 2025

lkts mentioned this pull request Apr 7, 2025

[8.x] Use FallbackSyntheticSourceBlockLoader for text fields (#126237) #126430

Merged

lkts mentioned this pull request Apr 7, 2025

Optimize block loader of semantic_text field for synthetic source #126434

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use FallbackSyntheticSourceBlockLoader for text fields#126237

Use FallbackSyntheticSourceBlockLoader for text fields#126237
lkts merged 5 commits intoelastic:mainfrom
lkts:synthetic_source_block_loader_text

lkts commented Apr 3, 2025

lkts Apr 3, 2025

martijnvg Apr 4, 2025

lkts Apr 4, 2025

lkts Apr 3, 2025

lkts Apr 3, 2025 •

edited

Loading

lkts Apr 3, 2025 •

edited

Loading

elasticsearchmachine commented Apr 3, 2025

elasticsearchmachine commented Apr 3, 2025

dnhatn left a comment

martijnvg left a comment

martijnvg Apr 4, 2025

Uh oh!

elasticsearchmachine commented Apr 7, 2025

lkts commented Apr 7, 2025

Labels

4 participants

Conversation

lkts commented Apr 3, 2025

lkts Apr 3, 2025

Choose a reason for hiding this comment

martijnvg Apr 4, 2025

Choose a reason for hiding this comment

lkts Apr 4, 2025

Choose a reason for hiding this comment

lkts Apr 3, 2025

Choose a reason for hiding this comment

lkts Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

lkts Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

elasticsearchmachine commented Apr 3, 2025

elasticsearchmachine commented Apr 3, 2025

dnhatn left a comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

martijnvg Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Apr 7, 2025

💔 Backport failed

lkts commented Apr 7, 2025

💚 All backports created successfully

Questions ?

Labels

4 participants

lkts Apr 3, 2025 •

edited

Loading

lkts Apr 3, 2025 •

edited

Loading