Configure keeping source in FieldMapper by kkrik-es · Pull Request #112706 · elastic/elasticsearch

kkrik-es · 2024-09-10T14:00:11Z

Introduces per-field param synthetic_source_keep that overrides the behavior for keeping the field source in synthetic source mode:

none : no source is stored
arrays: the incoming source is recorded as-is for arrays of a given field
all: the incoming source is recorded as is for both singleton and array values of a given field

Related to #112012

elasticsearchmachine · 2024-09-10T14:00:58Z

Hi @kkrik-es, I've created a changelog YAML for you.

…m' into synthetic-source/keep-field-param

kkrik-es · 2024-09-10T14:04:59Z

...javaRestTest/java/org/elasticsearch/datastreams/logsdb/qa/matchers/source/SourceMatcher.java

    private boolean sourceMatchesExactly(MappingTransforms.FieldMapping mapping, List<Object> expectedValues) {
-        return mapping.parentMappingParameters().stream().anyMatch(m -> m.getOrDefault("enabled", "true").equals("false"));
+        return mapping.parentMappingParameters().stream().anyMatch(m -> m.getOrDefault("enabled", "true").equals("false"))
+            || mapping.mappingParameters().getOrDefault("synthetic_source_keep", "none").equals("all");


@lkts any hints on how to cover arrays too?

I think that was the reason i passed expectedValues in here but it doesn't really work.
In theory this should work:

expectedValues.size() > 1 && mapping.mappingParameters().getOrDefault("synthetic_source_keep", "none").equals("arrays");

but it's also true when a field has a single value inside higher-level object array which won't trigger storing it in ignored source (at least in current impl).

Maybe we should embrace the pattern of "always try to exact match first and then do the custom match if that fails". I initially thought about it as temporary measure but it does make some things easier.

elasticsearch/modules/data-streams/src/javaRestTest/java/org/elasticsearch/datastreams/logsdb/qa/matchers/source/SourceMatcher.java

Line 102 in f6ace50

// There are cases when field values are stored in ignored source

What do you think?

Sounds good, there should be no complaints if we accidentally produce the same source..

But isn't this the case today, based on the code you call out above?

Yes, you can just remove sourceMatchesExactly.

Sorry I misunderstood your suggestion. I think the current form is correct, the test should report an error if the source is expected to be an exact copy but it's not - even if relaxed matching succeeds.

elasticsearchmachine · 2024-09-10T18:00:09Z

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

lkts

I have questions about changes in StringStoredFieldFieldLoader and CompositeSyntheticFieldLoader.

lkts · 2024-09-10T18:43:33Z

...javaRestTest/java/org/elasticsearch/datastreams/logsdb/qa/matchers/source/SourceMatcher.java

    private boolean sourceMatchesExactly(MappingTransforms.FieldMapping mapping, List<Object> expectedValues) {
-        return mapping.parentMappingParameters().stream().anyMatch(m -> m.getOrDefault("enabled", "true").equals("false"));
+        return mapping.parentMappingParameters().stream().anyMatch(m -> m.getOrDefault("enabled", "true").equals("false"))
+            || mapping.mappingParameters().getOrDefault("synthetic_source_keep", "none").equals("all");


I think that was the reason i passed expectedValues in here but it doesn't really work.
In theory this should work:

expectedValues.size() > 1 && mapping.mappingParameters().getOrDefault("synthetic_source_keep", "none").equals("arrays");

but it's also true when a field has a single value inside higher-level object array which won't trigger storing it in ignored source (at least in current impl).

Maybe we should embrace the pattern of "always try to exact match first and then do the custom match if that fails". I initially thought about it as temporary measure but it does make some things easier.

elasticsearch/modules/data-streams/src/javaRestTest/java/org/elasticsearch/datastreams/logsdb/qa/matchers/source/SourceMatcher.java

Line 102 in f6ace50

// There are cases when field values are stored in ignored source

What do you think?

lkts · 2024-09-10T18:51:41Z

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

        this.parts = parts;
        this.storedFieldLoadersHaveValues = false;
        this.docValuesLoadersHaveValues = false;
+        // In text mappers, the leaf name is a prefix of the full name and we want to use the leaf name, e.g. `myfield.sdfge` => 'myfield'


I think we should fix this specifically in text/keyword mapper which is the only place where this pattern is used. No need to complicate this generic code.

Moved the logic to keyword mapper, along with tests..

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

lkts · 2024-09-10T18:54:58Z

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

    @Override
    public String fieldName() {
-        return name;
+        return simpleName;


I don't think this is correct when a field is inside an object.

Good call, we need something better for this:

https://github.com/elastic/elasticsearch/blob/main/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldMapper.java#L436

Yes, either use a composite or store "actual field path" in this loader.

Used a similar approach to keyword mapper, ptal.

lkts · 2024-09-10T18:59:02Z

server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java

+    }
+
+    @Override
+    protected SyntheticSourceSupport syntheticSourceSupportForKeepTests(boolean ignoreMalformed) {


This came up recently and it sounds like we don't intend to support BigDecimal in xcontent #111937. I wonder if this is a valid test.

I tried adding casting for BigDecimal, still an issue.. It's freakin json.

# Conflicts: # test/framework/src/main/java/org/elasticsearch/logsdb/datageneration/datasource/DefaultMappingParametersHandler.java

lkts · 2024-09-11T15:16:06Z

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

+        return fullPath.substring(0, fullPath.lastIndexOf(simpleName) + simpleName.length());
+    }
+
+    public SourceLoader.SyntheticFieldLoader syntheticFieldLoader(String simpleName, boolean trimLeafNameSuffix) {


Why don't we take String fullPath as a parameter here? Then keyword and text mapper just pass fullPath() in here.

Yes sorry, simpler.

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

salvatore-campagna · 2024-09-12T13:33:52Z

.../src/yamlRestTest/resources/rest-api-spec/test/indices.create/21_synthetic_source_stored.yml

+                type: keyword
+              kw_arrays:
+                type: keyword
+                synthetic_source_keep: arrays


What happens if _source.mode is stored and we use the synthetic_source_keep parameter? I expect that setting to be ignored completely. Mappings and settings are composed in templates so it is not unlikely that we will have mixed situations.

What's the setting set to stored? I think I didn't get that part..

This thing is called synthetic_source_keep but you can have an index that is not using synthetic source, right? In that case you would have a mapping parameter that is about synthetic source....but with no synthetic source (stored source). This might happen because of template and component template composition and to me looks a bit odd. Also nothing prevents a user from setting this parameter explicitly without using synthetic source.

Moreover, some users might adopt an index mode (like logsdb or tsdb) which uses syntehtic source without actually knowing that synthetic source is used under the hood. Then you have to explain them that they need a parameter called synthetic_source_keep to preserve arrays...which requires telling them that synthetic source is used under the hood.

Some index modes might also have both source modes...

I don't know TBH what is the right thing here...it just looks a bit odd to me.

Another thing is the distinction between arrays and singletons...users might not necessarily know if a field is a single-value field or a multi-value field because they not necessarily control the mapping. Think about fields coming from OTel or some integration...we are asking them to make a choice...and most of the time, just to be on the safe side, they will need to chose all because they don't know if the filed is single-value or multi-value.

IMO we should have just two choices, preserve_array, true or false. I understand that not making a distinction between singleton and arrays might result in performance penalty...under stored source preserve_array is a no-op.

We had a long thread on this topic in #112397.. Names are hard, we can argue for and against each option.

all should be used very deliberately, and probably rarely. The options for arrays is more widely applicable and easier to grasp, since this is a known limitation of synthetic source. It's thus better to have separate options for arrays and [arrays + singletons].

salvatore-campagna · 2024-09-12T13:35:21Z

I think this needs to be documented with some examples...there might be corner cases where using this is not trivial. Is the documentation updated in another PR?

kkrik-es · 2024-09-12T14:58:58Z

I think this needs to be documented with some examples...there might be corner cases where using this is not trivial. Is the documentation updated in another PR?

Yeah there will be another PR updating objects, I'll probably add documentation then. We also need to document the index-level setting..

martijnvg

LGTM - left one question for my understanding.

martijnvg · 2024-09-13T09:49:02Z

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

                && fieldMapper.syntheticSourceMode() == FieldMapper.SyntheticSourceMode.FALLBACK;
            boolean fieldWithStoredArraySource = mapper instanceof FieldMapper fieldMapper
-                && context.sourceKeepModeFromIndexSettings() == Mapper.SourceKeepMode.ARRAYS;
+                && getSourceKeepMode(context, fieldMapper.sourceKeepMode()) != Mapper.SourceKeepMode.NONE;


question, should this just be: getSourceKeepMode(context, fieldMapper.sourceKeepMode()) = ARRAYS;, since this method is for dealing with arrays?

We want to keep array source both for ARRAYS and ALL values, I think.

Right, maybe my assumption (which may be wrong) is that in this method we only deal with arrays, so I thought just checking for ARRAYS should be sufficient.

Based on the definition of ALL, the source should be recorded when parsing an array. Maybe I'm missing something?

Right, I understand that arrays are included with ALL. I think this code will work. I just wondered whether getSourceKeepMode(context, fieldMapper.sourceKeepMode()) = ARRAYS; would work here too, since only arrays should be handled by the parseNonDynamicArray(...) method?

The parseNonDynamicArray method is called when parsing an array in a doc. In both ARRAYS and ALL cases, we want to record the source . If we just check for ARRAYS here, the array source won't get recorded in the ALL case - this is similar to the check above for objectMapper.storeArraySource() that applies to objects.

Ok, I see now. This makes sense now.

# Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java # server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java # test/framework/src/main/java/org/elasticsearch/logsdb/datageneration/datasource/DefaultMappingParametersHandler.java

# Conflicts: # modules/data-streams/src/javaRestTest/java/org/elasticsearch/datastreams/logsdb/qa/StandardVersusLogsIndexModeRandomDataChallengeRestIT.java

kkrik-es · 2024-09-19T11:53:26Z

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

+        for (var field : ignoredFieldsMissingValues) {
+            fields.put(field.name(), field);
+        }
+        context.deduplicateIgnoredFieldValues(fields.keySet());


Fyi, I added a yaml test and it caught a bug where there may be duplicate values, as a leaf array may be recorded separately and before identifying that all leaf elements need to be recorded in the second pass. This fixes it.

Do you have an example? I don't get it.

https://github.com/elastic/elasticsearch/blob/main/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.create/21_synthetic_source_stored.yml#L698

Values 1000, 2000 were showing up in the beginning of the list too.

Introduces per-field param `synthetic_source_keep` that overrides the behavior for keeping the field source in synthetic source mode: - `none` : no source is stored - `arrays`: the incoming source is recorded as-is for arrays of a given field - `all`: the incoming source is recorded as is for both singleton and array values of a given field Related to elastic#112012

elasticsearchmachine · 2024-09-19T13:30:56Z

💚 Backport successful

Status	Branch	Result
✅	8.x

Introduces per-field param `synthetic_source_keep` that overrides the behavior for keeping the field source in synthetic source mode: - `none` : no source is stored - `arrays`: the incoming source is recorded as-is for arrays of a given field - `all`: the incoming source is recorded as is for both singleton and array values of a given field Related to #112012

This PR introduces a new track parameter, `synthetic_source_keep` which is used to control the behaviour of synthetic source for all field types. It can have values `none`, `arrays` or `all` (`all` not usable when set at index level). See elastic/elasticsearch#112706 to understand the effect of each value. Later on we will use this to change the behaviour in our nightlies and run benchmarks on both `elastic/logs` and `elastic/security` using value `arrays`.

…tic#682) This PR introduces a new track parameter, `synthetic_source_keep` which is used to control the behaviour of synthetic source for all field types. It can have values `none`, `arrays` or `all` (`all` not usable when set at index level). See elastic/elasticsearch#112706 to understand the effect of each value. Later on we will use this to change the behaviour in our nightlies and run benchmarks on both `elastic/logs` and `elastic/security` using value `arrays`.

#684) This PR introduces a new track parameter, `synthetic_source_keep` which is used to control the behaviour of synthetic source for all field types. It can have values `none`, `arrays` or `all` (`all` not usable when set at index level). See elastic/elasticsearch#112706 to understand the effect of each value. Later on we will use this to change the behaviour in our nightlies and run benchmarks on both `elastic/logs` and `elastic/security` using value `arrays`.

kkrik-es added 2 commits September 10, 2024 16:48

Configure keeping source in FieldMapper

3ece000

Merge branch 'main' into synthetic-source/keep-field-param

eb2be1a

kkrik-es added >enhancement Team:StorageEngine :StorageEngine/Mapping The storage related side of mappings labels Sep 10, 2024

kkrik-es self-assigned this Sep 10, 2024

elasticsearchmachine added the v8.16.0 label Sep 10, 2024

kkrik-es and others added 3 commits September 10, 2024 17:00

Update docs/changelog/112706.yaml

5d5566e

revert unintended

1a2e4af

Merge remote-tracking branch 'origin/synthetic-source/keep-field-para…

9f72215

…m' into synthetic-source/keep-field-param

kkrik-es commented Sep 10, 2024

View reviewed changes

kkrik-es added 2 commits September 10, 2024 17:21

spotless

9135045

fix tests

9aed8b1

kkrik-es requested review from lkts and martijnvg September 10, 2024 17:59

kkrik-es marked this pull request as ready for review September 10, 2024 17:59

fix tests

8746f59

lkts requested changes Sep 10, 2024

View reviewed changes

kkrik-es added 4 commits September 10, 2024 22:09

fix tests

f9a677e

Merge branch 'main' into synthetic-source/keep-field-param

885f975

# Conflicts: # test/framework/src/main/java/org/elasticsearch/logsdb/datageneration/datasource/DefaultMappingParametersHandler.java

address comments

a29d601

spotless

2043bf3

kkrik-es requested a review from lkts September 11, 2024 12:00

lkts reviewed Sep 11, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/index/mapper/StringStoredFieldFieldLoader.java Show resolved Hide resolved

mark-vieira added v9.0.0 and removed v8.16.0 labels Sep 11, 2024

Merge branch 'main' into synthetic-source/keep-field-param

be782bc

spotless

e026593

salvatore-campagna reviewed Sep 12, 2024

View reviewed changes

lkts approved these changes Sep 12, 2024

View reviewed changes

lkts mentioned this pull request Sep 12, 2024

Fix StandardVersusLogsIndexModeRandomDataChallengeRestIT to actually use logsdb #112830

Closed

kkrik-es mentioned this pull request Sep 13, 2024

Fix synthetic source field names for multi-fields #112850

Merged

martijnvg approved these changes Sep 13, 2024

View reviewed changes

kkrik-es added 5 commits September 13, 2024 15:06

sync

e47623c

disable keeping array source in randomized tests

887e822

Merge branch 'main' into synthetic-source/keep-field-param

fa15f55

# Conflicts: # modules/data-streams/src/javaRestTest/java/org/elasticsearch/datastreams/logsdb/qa/StandardVersusLogsIndexModeRandomDataChallengeRestIT.java

enable in randomized tests

2afddca

kkrik-es added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) and removed auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Sep 19, 2024

check for conflicting ignored values on second pass

64658f3

kkrik-es commented Sep 19, 2024

View reviewed changes

kkrik-es added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 19, 2024

salvatore-campagna approved these changes Sep 19, 2024

View reviewed changes

elasticsearchmachine merged commit e244216 into elastic:main Sep 19, 2024

kkrik-es deleted the synthetic-source/keep-field-param branch September 19, 2024 13:29

kkrik-es mentioned this pull request Sep 19, 2024

[8.x] Configure keeping source in FieldMapper (#112706) #113181

Merged

salvatore-campagna mentioned this pull request Sep 30, 2024

Include a variable to control synthetic_source_keep parameter elastic/rally-tracks#682

Merged

kkrik-es mentioned this pull request Sep 30, 2024

Add object param for keeping synthetic source #113690

Merged

Conversation

kkrik-es commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Sep 10, 2024

Uh oh!

lkts left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkrik-es Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salvatore-campagna Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salvatore-campagna Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salvatore-campagna commented Sep 12, 2024

Uh oh!

kkrik-es commented Sep 12, 2024

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkrik-es commented Sep 10, 2024 •

edited

Loading

kkrik-es Sep 10, 2024 •

edited

Loading

salvatore-campagna Sep 13, 2024 •

edited

Loading

salvatore-campagna Sep 13, 2024 •

edited

Loading

kkrik-es Sep 16, 2024 •

edited

Loading

kkrik-es Sep 19, 2024 •

edited

Loading