Use fallback synthetic source for copy_to and doc_values: false cases #112294

lkts · 2024-08-28T16:28:24Z

Closes #109546.
Closes #110038.
Closes #110753.

elasticsearchmachine · 2024-09-05T22:39:07Z

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

elasticsearchmachine · 2024-09-05T22:52:03Z

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

...apper-extras/src/main/java/org/elasticsearch/index/mapper/extras/ScaledFloatFieldMapper.java

kkrik-es · 2024-09-06T11:27:21Z

test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java

-                    }
-                )
-            );
-        }


Shall we add a catch-all test to exercise copy_to in all subclasses?

This is tricky because copy_to only works with text-like data. F.e. for histogram it does not work even though technically it is supported.

...rk/src/main/java/org/elasticsearch/index/mapper/TextFieldFamilySyntheticSourceTestSetup.java

kkrik-es · 2024-09-06T11:38:50Z

Oops, sorry for the merge conflicts.. I think if you just keep your version it should be good..

martijnvg

LGTM

salvatore-campagna · 2024-09-10T07:09:39Z

...les/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/match_only_text/10_basic.yml

+              mode: synthetic
+            properties:
+              foo:
+                type: match_only_text


if here we add store: true...are we going to store the value twice?

Yes but it is not necessarily the same value (e.g. normalizers).

salvatore-campagna · 2024-09-10T07:10:32Z

...les/mapper-extras/src/yamlRestTest/resources/rest-api-spec/test/match_only_text/10_basic.yml

+        id:    "1"
+        refresh: true
+        body:
+          foo: "Apache Lucene powers Elasticsearch"


Should we also test with an array of values?

I'll add them in a follow-up.

salvatore-campagna · 2024-09-10T07:11:29Z

...ext/src/main/java/org/elasticsearch/index/mapper/annotatedtext/AnnotatedTextFieldMapper.java

        }

        var kwd = TextFieldMapper.SyntheticSourceHelper.getKeywordFieldMapperForSyntheticSource(this);
        if (kwd != null) {
-            return kwd.syntheticFieldLoader(leafName());
+            return new SyntheticSourceSupport.Native(kwd.syntheticFieldLoader(leafName()));


Maybe we can consider naming the two synthetic source modes WithDocValues and WithStoredFields instead of Native and Fallback? Like:

SyntheticSourceSupport.WithDocValues

SyntheticSourceSupport.WithStoredFields

That's not really the distinction i am trying to make. Yes, _ignored_source is a stored field that holds the value used for synthetic source but it is also possible that the field itself uses a different stored field for synthetic source. That becomes pretty blurry.

The difference between Native and Fallback is who controls synthetic source logic. In Native it's the mapper, in Fallback it's on a layer above and the mapper does not control what happens.

salvatore-campagna · 2024-09-10T07:13:28Z

...t/src/yamlRestTest/resources/rest-api-spec/test/mapper_annotatedtext/20_synthetic_source.yml

+              mode: synthetic
+            properties:
+              annotated_text:
+                type: annotated_text


Same as before...if we add store: true do we store twice?

salvatore-campagna · 2024-09-10T07:14:34Z

...pi-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.create/20_synthetic_source.yml

+            _source:
+              mode: synthetic
+            properties:
+              number:


Should we test copy_to in combination with ignore_above and ignore_malformed?

We test it with ignore_malformed for unsigned_long f.e.

ignore_above is a good point, i'll follow up.

salvatore-campagna · 2024-09-10T07:18:31Z

server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java

+     */
+    protected enum SyntheticSourceMode {
+        NATIVE,
+        FALLBACK


Asked this above already: should we name these something like WITH_DOC_VALUES and WITH_STORED_FIELDS?

salvatore-campagna · 2024-09-10T07:20:58Z

server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java

@@ -35,7 +35,8 @@ public Set<NodeFeature> getFeatures() {
            ObjectMapper.SUBOBJECTS_AUTO,
            KeywordFieldMapper.KEYWORD_NORMALIZER_SYNTHETIC_SOURCE,
            SourceFieldMapper.SYNTHETIC_SOURCE_STORED_FIELDS_ADVANCE_FIX,
-            Mapper.SYNTHETIC_SOURCE_KEEP_FEATURE
+            Mapper.SYNTHETIC_SOURCE_KEEP_FEATURE,
+            SourceFieldMapper.SYNTHETIC_SOURCE_RESTRICTIONS_REMOVED


SYNTHETIC_SOURCE_WITH_COPY_TO_AND_DOC_VALUES_FALSE_SUPPORT? I Know it is longer but much more descriptive.

salvatore-campagna · 2024-09-10T07:22:30Z

...java/org/elasticsearch/logsdb/datageneration/datasource/DefaultMappingParametersHandler.java

    }

    private Supplier<Map<String, Object>> scaledFloatMapping() {
        return () -> {
            var scalingFactor = ESTestCase.randomFrom(10, 1000, 100000, 100.5);
-            return Map.of("scaling_factor", scalingFactor, "store", ESTestCase.randomBoolean(), "index", ESTestCase.randomBoolean());
+            return Map.of(
+                "scaling_factor",


nit: indentation

salvatore-campagna · 2024-09-10T07:24:47Z

73 files is quite a lot anyway

salvatore-campagna · 2024-09-10T07:26:15Z

Don't forget the PR description

elasticsearchmachine · 2024-09-10T15:08:19Z

Hi @lkts, I've updated the changelog YAML for you.

lkts · 2024-09-10T15:24:37Z

@elasticmachine update branch

elasticsearchmachine added needs:triage Requires assignment of a team area label v8.16.0 labels Aug 28, 2024

lkts marked this pull request as draft August 28, 2024 16:28

lkts added 2 commits September 5, 2024 15:37

Support copy_to with synthetic source using fallback mechanism

19c044d

Use fallback synthetic source for copy_to and doc_values: false cases

bda4de6

lkts force-pushed the synthetic_source_copy_to branch from 97a1ea5 to bda4de6 Compare September 5, 2024 22:37

lkts changed the title ~~Support copy_to with synthetic source using fallback mechanism~~ Use fallback synthetic source for copy_to and doc_values: false cases Sep 5, 2024

lkts added >feature :StorageEngine/Mapping The storage related side of mappings >enhancement and removed >feature labels Sep 5, 2024

lkts requested review from martijnvg, salvatore-campagna and kkrik-es September 5, 2024 22:38

Update docs/changelog/112294.yaml

30aefb3

lkts marked this pull request as ready for review September 5, 2024 22:51

elasticsearchmachine added Team:StorageEngine and removed needs:triage Requires assignment of a team area label labels Sep 5, 2024

Fix

2950b2e

kkrik-es reviewed Sep 6, 2024

View reviewed changes

...apper-extras/src/main/java/org/elasticsearch/index/mapper/extras/ScaledFloatFieldMapper.java Show resolved Hide resolved

kkrik-es reviewed Sep 6, 2024

View reviewed changes

...rk/src/main/java/org/elasticsearch/index/mapper/TextFieldFamilySyntheticSourceTestSetup.java Show resolved Hide resolved

kkrik-es closed this Sep 6, 2024

kkrik-es reopened this Sep 6, 2024

lkts added 3 commits September 6, 2024 11:43

Merge branch 'main' into synthetic_source_copy_to

305818a

Fix tests

cdbe5c1

Fix compilation

936a2dc

Fix one more test

489f030

kkrik-es approved these changes Sep 9, 2024

View reviewed changes

martijnvg approved these changes Sep 9, 2024

View reviewed changes

lkts added 2 commits September 9, 2024 14:36

increase test coverage

bf2ccb8

style

352e9c7

salvatore-campagna reviewed Sep 10, 2024

View reviewed changes

Update docs/changelog/112294.yaml

24de7b3

elasticmachine and others added 2 commits September 10, 2024 17:24

Merge branch 'main' into synthetic_source_copy_to

23c0890

Address feedback

ff4342a

lkts merged commit 082e721 into elastic:main Sep 10, 2024
15 checks passed

lkts deleted the synthetic_source_copy_to branch September 10, 2024 19:12

lkts mentioned this pull request Sep 12, 2024

Ensure that fields copied using copy_to are not present in synthetic source #112625

Merged

lkts mentioned this pull request Sep 20, 2024

Correctly identify parent of copy_to destination field for synthetic source purposes #113153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use fallback synthetic source for copy_to and doc_values: false cases #112294

Use fallback synthetic source for copy_to and doc_values: false cases #112294

lkts commented Aug 28, 2024 •

edited

Loading

elasticsearchmachine commented Sep 5, 2024

elasticsearchmachine commented Sep 5, 2024

kkrik-es Sep 6, 2024

lkts Sep 6, 2024 •

edited

Loading

kkrik-es commented Sep 6, 2024

martijnvg left a comment

salvatore-campagna Sep 10, 2024

lkts Sep 10, 2024

salvatore-campagna Sep 10, 2024

lkts Sep 10, 2024

salvatore-campagna Sep 10, 2024 •

edited

Loading

lkts Sep 10, 2024

salvatore-campagna Sep 10, 2024

salvatore-campagna Sep 10, 2024

lkts Sep 10, 2024

lkts Sep 10, 2024

salvatore-campagna Sep 10, 2024

salvatore-campagna Sep 10, 2024

salvatore-campagna Sep 10, 2024

salvatore-campagna commented Sep 10, 2024

salvatore-campagna commented Sep 10, 2024

elasticsearchmachine commented Sep 10, 2024

lkts commented Sep 10, 2024

Use fallback synthetic source for copy_to and doc_values: false cases #112294

Use fallback synthetic source for copy_to and doc_values: false cases #112294

Conversation

lkts commented Aug 28, 2024 • edited Loading

elasticsearchmachine commented Sep 5, 2024

elasticsearchmachine commented Sep 5, 2024

Choose a reason for hiding this comment

lkts Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

kkrik-es commented Sep 6, 2024

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna commented Sep 10, 2024

salvatore-campagna commented Sep 10, 2024

elasticsearchmachine commented Sep 10, 2024

lkts commented Sep 10, 2024

lkts commented Aug 28, 2024 •

edited

Loading

lkts Sep 6, 2024 •

edited

Loading

salvatore-campagna Sep 10, 2024 •

edited

Loading