Text fields are stored by default in TSDB indices #106338

lkts · 2024-03-14T00:10:33Z

Synthetic source requires text fields to be stored or have keyword
sub-field that supports synthetic source. If there are no keyword fields
users currently have to explicitly set 'store' to 'true' or get a
validation exception. This is not the best experience. It is quite
likely that setting store to true is the correct thing to do but
users still get an error and need to investigate it. With this change if
store setting is not specified in such context it will be set to
true by default. Setting it explicitly to false results in the
exception.

Closes #97039

Synthetic source requires text fields to be stored or have keyword sub-field that supports synthetic source. If there are no keyword fields users currently have to explicitly set 'store' to 'true' or get a validation exception. This is not the best experience. It is quite likely that setting `store` to `true` is the correct thing to do but users still get an error and need to investigate it. With this change if `store` setting is not specified in such context it will be set to `true` by default. Setting it explicitly to `false` results in the exception. Closes elastic#97039

github-actions · 2024-03-14T00:10:47Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2024-03-14T00:12:58Z

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

elasticsearchmachine · 2024-03-14T00:12:59Z

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

martijnvg

Thanks! I left a comment about a potential different direction we may want to take here.

docs/reference/mapping/types/text.asciidoc

martijnvg · 2024-03-14T11:16:13Z

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

+            //
+            // If 'store' parameter was explicitly provided we'll let it pass and
+            // fail in TextFieldMapper#syntheticFieldLoader later if needed.
+            if (store.isSet() == false


Ideally we would do default values and validation via the Parameter class in the Builder constructor in the file. Something like this:

this.store = Parameter.storeParam(m -> ((TextFieldMapper) m).store, () -> syntheticSource).addValidator(value -> { if (syntheticSource && value == false) { throw new IllegalArgumentException("If synthetic source is enabled, then store cannot be set to false"); } });

The issue is that now we can't see the details we care about here in the multi fields (MultiFields.Builder only keep a map with a Function per field name around).

Note that this way does require more plumbing and additional changes.

If we like to do this approach we may need to make changes to MultiFields.Builder, so that we know whether there is a keyword field (either stored or doesn't have normalizer and ignore above) as multi field.

I tried this approach and there is a problem. It is not possible to have syntheticSource flag with proposed impl from gist in MappingParserContext because it will return if synthetic source is enabled for currently existing mapping or empty mapping in case of index creation. mappingLookup will only be updated as a last step after parsing and building new mapping.

Value that we need is here https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java#L147 and it is not available to field mapper builders since builders are created in RootObjectMapper#parse which happens before that.

We could try:

Creating metadata field mappers before RootObjectMapper. I assume that there is a good reason for that ordering so this is unlikely a solution.

Support storing text fields by default only for TSDB indices (and any future index modes that will have synthethic source always enabled) instead of for any index with synthetic source enabled. This works since we know source is synthetic even without parsing the mapping. I think we are mostly interested in this use case anyway so this may be a good solution.

Keep initial implementation from this PR.

Thanks for writing down this analysis. It is kind of a pain that we know synthetic source is used after the builders are created. I don't think it is worth changing the construction of mapping builders, since it most likely is a tricky change.

Regarding option 2, we know if index mode has been set to time series that synthetic source is enabled, and this will also be the case for the new logging index mode. Given that index mode comes from index setting, we know at the time mapping builders are created what the index mode has been set to. So we can use that as well for determining the default value of the store attribute. So let's do option 2?

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

elasticsearchmachine · 2024-03-14T14:04:12Z

Pinging @elastic/es-search (Team:Search)

lkts · 2024-03-21T00:27:07Z

I have pushed change to use option 2 from the discussion above.

One issue i discovered is that it is not possible to implement validation using Parameter#addValidator API. The problem is that we need to provide full name of the field in the error message e.g. k8s.pod.agent.name (as we currently do). It is not possible to get this information in constructor of TextFieldMapper.Builder, this information is only exposed during build() via MapperBuilderContext#buildFullName. We can only report field name as name in this case which is obviously confusing. Additionally, performing validation in constructor changes the error returned to user from illegal_argument_exception to illegal_argument_exception nested inside mapper_parsing_exception. I don't think this is a problem by itself but it would mean that errors are different for TSDB indices and non TSDB indices and that seems undesirable.

As a result i removed validation logic and i am relying on existing code for that.

martijnvg

One issue i discovered is that it is not possible to implement validation using Parameter#addValidator API. The problem is that we need to provide full name of the field in the error message e.g. k8s.pod.agent.name (as we currently do). It is not possible to get this information in constructor of TextFieldMapper.Builder, this information is only exposed during build() via MapperBuilderContext#buildFullName. We can only report field name as name in this case which is obviously confusing.

I agree this part is confusion, so +1 to rely on the currently validation if store is specifically disabled.

This change is looking good, a left a few more (minor) comments.

martijnvg · 2024-03-21T08:04:01Z

docs/reference/mapping/types/text.asciidoc

-    the <<mapping-source-field,`_source`>> field. Accepts `true` or `false`
-    (default).
+    the <<mapping-source-field,`_source`>> field. Accepts `true` or `false` (default).
+    To support <<synthetic-source,synthetic `_source`>> this parameter


Maybe rename To support <<synthetic-source,synthetic _source>> this parameter to If <<synthetic-source,synthetic _source>> is enabled, then this parameter?

martijnvg · 2024-03-21T08:13:31Z

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

@@ -283,12 +283,23 @@ public static class Builder extends FieldMapper.Builder {

        final TextParams.Analyzers analyzers;

-        public Builder(String name, IndexAnalyzers indexAnalyzers) {
-            this(name, IndexVersion.current(), indexAnalyzers);
+        public Builder(String name, IndexAnalyzers indexAnalyzers, boolean isSyntheticSourceAlwaysUsed) {


Maybe rename to synthetcSourceEnabledViaIndexMode? To indicate that synthetic source is enabled because the index mode enables it.

I was trying to express that and your suggestion is so much more clear!

martijnvg · 2024-03-21T08:16:54Z

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

@@ -1258,7 +1265,7 @@ public Map<String, NamedAnalyzer> indexAnalyzers() {

    @Override
    public FieldMapper.Builder getMergeBuilder() {
-        return new Builder(simpleName(), indexCreatedVersion, indexAnalyzers).init(this);
+        return new Builder(simpleName(), indexCreatedVersion, indexAnalyzers, false).init(this);


Always providing false here is incorrect. It could disable synthetic source at some point. I think we need record in TextFieldMapper class, that synthetic source was enabled, so the right value can be provided here.

I don't think it can actually disable it since the default value should be already set correctly before that and init will pick it up. I do agree it is more clear to store it though.

server/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java

server/src/test/java/org/elasticsearch/index/fielddata/AbstractFieldDataTestCase.java

server/src/test/java/org/elasticsearch/index/fielddata/FilterFieldDataTests.java

lkts · 2024-03-21T22:42:47Z

CI uncovered another interesting problem. store parameter was not serialized here

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

Line 1423 in 2b67444

b.store.toXContent(builder, includeDefaults);

because includeDefaults seems to be always false. During downsampling new index is created which is not a TSDB index and since store value was not serialized it defaults to false. We successfully transfer synthetic source setting from original index and as a result we now have synthetic source and text field that is not stored.

I have fixed serialization but i honestly don't know if it is a proper solution. I don't really like the state of "defaults can be interpreted differently in different contexts".

martijnvg · 2024-03-22T14:21:21Z

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

+                // we want to serialize 'true' value even if it is default
+                // since defaults differ for different index modes
+                // and can be interpreted differently is different context
+                .setSerializerCheck((includeDefaults, isConfigured, value) -> includeDefaults || isConfigured || value);


A downsampled index should have index.mode=time_series. What exact failure occurs if this check is missing? Maybe we need to change something in TransportDownsampleAction instead.

Downsample index is created using temporary index service using a static predefined subset of index settings. All other settings are later copied over from source index. As discovered in elastic#106338 this causes context like index mode to be missing during initial index creation process. This PR adds index mode and related required settings to initial set of index settings in order to have access to this information during initial create index operation.

* Set index mode earlier for new downsample index Downsample index is created using temporary index service using a static predefined subset of index settings. All other settings are later copied over from source index. As discovered in #106338 this causes context like index mode to be missing during initial index creation process. This PR adds index mode and related required settings to initial set of index settings in order to have access to this information during initial create index operation.

lkts · 2024-03-26T16:30:11Z

@elasticmachine update branch

This reverts commit a0bce15.

martijnvg

Left two comments. LGTM otherwise.

martijnvg · 2024-03-26T17:40:08Z

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

@@ -249,6 +251,61 @@ public void testDefaults() throws IOException {
        assertEquals(DocValuesType.NONE, fieldType.docValuesType());
    }

+    public void testStoreParameterDefaults() throws IOException {


I think this test needs to be adjusted with the serialization logic removed to address the CI failure?

martijnvg · 2024-03-26T17:43:10Z

docs/reference/mapping/types/text.asciidoc

-    the <<mapping-source-field,`_source`>> field. Accepts `true` or `false`
-    (default).
+    the <<mapping-source-field,`_source`>> field. Accepts `true` or `false` (default).
+    If <<synthetic-source,synthetic _source>> is enabled, then this


Maybe remove If <<synthetic-source,synthetic _source>> is enabled part? The store param defaults to true for tsdb indices, when there is no suitable multi field of type keyword .

Woops, thanks. This is leftover.

lkts requested a review from martijnvg March 14, 2024 00:10

elasticsearchmachine added v8.14.0 needs:triage Requires assignment of a team area label labels Mar 14, 2024

lkts added the Team:StorageEngine label Mar 14, 2024

elasticsearchmachine removed the Team:StorageEngine label Mar 14, 2024

lkts added >enhancement :StorageEngine/TSDB You know, for Metrics Team:StorageEngine and removed needs:triage Requires assignment of a team area label labels Mar 14, 2024

Update docs/changelog/106338.yaml

8b72975

martijnvg reviewed Mar 14, 2024

View reviewed changes

martijnvg added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Mar 14, 2024

elasticsearchmachine added the Team:Search Meta label for search team label Mar 14, 2024

lkts added 3 commits March 20, 2024 17:26

Adjust documentation

053883b

Adjust documentation to the new approach

cd8441b

Configure default value using Parameter APIs

a0715d2

Revert unnecessary change

5f3cba4

martijnvg reviewed Mar 21, 2024

View reviewed changes

lkts changed the title ~~Text fields are stored by default with synthetic source~~ Text fields are stored by default in TSDB indices Mar 21, 2024

lkts and others added 5 commits March 21, 2024 11:35

Update 106338.yaml

8b47538

Address feedback

aeb2b70

Revert unnecessary change

ee885aa

Spotless

dbb72e2

Fix serialization of store parameter

a0bce15

martijnvg reviewed Mar 22, 2024

View reviewed changes

lkts mentioned this pull request Mar 25, 2024

Set index mode earlier for new downsample index #106728

Merged

elasticmachine and others added 3 commits March 26, 2024 17:30

Merge branch 'main' into fix/97039

4a5c382

Revert "Fix serialization of store parameter"

2e3518c

This reverts commit a0bce15.

Fix test

db40668

martijnvg approved these changes Mar 26, 2024

View reviewed changes

Adjust docs

156c869

lkts merged commit 9e6b893 into elastic:main Mar 26, 2024

lkts deleted the fix/97039 branch March 26, 2024 20:37

lkts mentioned this pull request Apr 16, 2024

Binary fields should have doc values enabled by default for index modes using synthetic source #107554

Closed

felixbarny mentioned this pull request Aug 6, 2024

Synthetic Source #86603

Closed

50 tasks

tommyers-elastic mentioned this pull request Oct 7, 2024

Synthetic Source Issue with Text Fields #114235

Closed

Text fields are stored by default in TSDB indices #106338

Text fields are stored by default in TSDB indices #106338

Uh oh!

Conversation

lkts commented Mar 14, 2024

Uh oh!

github-actions bot commented Mar 14, 2024

Uh oh!

elasticsearchmachine commented Mar 14, 2024

Uh oh!

elasticsearchmachine commented Mar 14, 2024

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkts Mar 14, 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!

Uh oh!

elasticsearchmachine commented Mar 14, 2024

Uh oh!

lkts commented Mar 21, 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!

Uh oh!

Uh oh!

Uh oh!

lkts commented Mar 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkts commented Mar 26, 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!

Uh oh!

lkts Mar 14, 2024 •

edited

Loading