Set default index mode for TimeSeries to null (elastic#98586)

* Default index mode null for TimeSeries If the default index mode matches the specified, we skip printing the synthetic source info in the mappings printing. This leads to confusion as it's not immediately visible (or well known) that time series indices use synthetic source by default. Leaving the default index mode to null does the trick here. We do pass the right value for time series indexes while building the mapping so there's no functional impact here. Fixes elastic#97429 * Update docs/changelog/98586.yaml * Restore other error messages. * Update source in DocumentMapper to include synthetic source. * Add version check for skipping assert
kkrik-es · Aug 23, 2023 · 8d814ff · 8d814ff
1 parent ab8bf0e
commit 8d814ff
Show file tree

Hide file tree

Showing 7 changed files with 53 additions and 12 deletions.
diff --git a/docs/changelog/98586.yaml b/docs/changelog/98586.yaml
@@ -0,0 +1,6 @@
+pr: 98586
+summary: Set default index mode for `TimeSeries` to `null`
+area: Aggregations
+type: bug
+issues:
+ - 97429
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java
@@ -10,6 +10,7 @@
 
 import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.index.IndexSettings;
+import org.elasticsearch.index.IndexVersion;
 
 import java.util.List;
 
@@ -30,16 +31,27 @@ public static DocumentMapper createEmpty(MapperService mapperService) {
         );
         MetadataFieldMapper[] metadata = mapperService.getMetadataMappers().values().toArray(new MetadataFieldMapper[0]);
         Mapping mapping = new Mapping(root, metadata, null);
-        return new DocumentMapper(mapperService.documentParser(), mapping, mapping.toCompressedXContent());
+        return new DocumentMapper(mapperService.documentParser(), mapping, mapping.toCompressedXContent(), IndexVersion.current());
     }
 
-    DocumentMapper(DocumentParser documentParser, Mapping mapping, CompressedXContent source) {
+    DocumentMapper(DocumentParser documentParser, Mapping mapping, CompressedXContent source, IndexVersion version) {
         this.documentParser = documentParser;
         this.type = mapping.getRoot().name();
         this.mappingLookup = MappingLookup.fromMapping(mapping);
-        this.mappingSource = source;
-        assert mapping.toCompressedXContent().equals(source)
-            : "provided source [" + source + "] differs from mapping [" + mapping.toCompressedXContent() + "]";
+        if (sourceMapper().isSynthetic()
+            && source.string().contains("\"_source\":{\"mode\":\"synthetic\"}") == false
+            && version.onOrBefore(IndexVersion.V_8_10_0)) {
+            /*
+             * Indexes built at v.8.7 were missing an explicit entry for synthetic_source.
+             * This got restored in v.8.9 (and patched in v.8.8) to avoid confusion. The change is only restricted to
+             * mapping printout, it has no functional effect as the synthetic source already applies.
+             */
+            this.mappingSource = mapping.toCompressedXContent();
+        } else {
+            assert mapping.toCompressedXContent().equals(source)
+                : "provided source [" + source + "] differs from mapping [" + mapping.toCompressedXContent() + "]";
+            this.mappingSource = source;
+        }
     }
 
     public Mapping mapping() {

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java
@@ -386,7 +386,7 @@ public DocumentMapper merge(String type, CompressedXContent mappingSource, Merge
     }
 
     private DocumentMapper newDocumentMapper(Mapping mapping, MergeReason reason, CompressedXContent mappingSource) {
-        DocumentMapper newMapper = new DocumentMapper(documentParser, mapping, mappingSource);
+        DocumentMapper newMapper = new DocumentMapper(documentParser, mapping, mappingSource, indexVersionCreated);
         newMapper.validate(indexSettings, reason != MergeReason.MAPPING_RECOVERY);
         return newMapper;
     }

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java
@@ -86,10 +86,15 @@ public static class Builder extends MetadataFieldMapper.Builder {
             .setMergeValidator(
                 (previous, current, conflicts) -> (previous.value() == current.value()) || (previous.value() && current.value() == false)
             );
+
+        /*
+         * The default mode for TimeSeries is left empty on purpose, so that mapping printings include the synthetic
+         * source mode.
+         */
         private final Parameter<Mode> mode = new Parameter<>(
             "mode",
             true,
-            () -> getIndexMode() == IndexMode.TIME_SERIES ? Mode.SYNTHETIC : null,
+            () -> null,
             (n, c, o) -> Mode.valueOf(o.toString().toUpperCase(Locale.ROOT)),
             m -> toType(m).enabled.explicit() ? null : toType(m).mode,
             (b, n, v) -> b.field(n, v.toString().toLowerCase(Locale.ROOT)),
@@ -136,10 +141,11 @@ private boolean isDefault() {
 
         @Override
         public SourceFieldMapper build() {
-            if (enabled.getValue().explicit() && mode.get() != null) {
+            if (enabled.getValue().explicit()) {
                 if (indexMode == IndexMode.TIME_SERIES) {
                     throw new MapperParsingException("Time series indices only support synthetic source");
-                } else {
+                }
+                if (mode.get() != null) {
                     throw new MapperParsingException("Cannot set both [mode] and [enabled] parameters");
                 }
             }
@@ -217,7 +223,7 @@ private SourceFieldMapper(Mode mode, Explicit<Boolean> enabled, String[] include
         this.sourceFilter = buildSourceFilter(includes, excludes);
         this.includes = includes;
         this.excludes = excludes;
-        if (this.sourceFilter != null && mode == Mode.SYNTHETIC) {
+        if (this.sourceFilter != null && (mode == Mode.SYNTHETIC || indexMode == IndexMode.TIME_SERIES)) {
             throw new IllegalArgumentException("filtering the stored _source is incompatible with synthetic source");
         }
         this.complete = stored() && sourceFilter == null;

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java
@@ -69,7 +69,7 @@ public void testAddFields() throws Exception {
         assertThat(stage1.mappers().getMapper("obj1.prop1"), nullValue());
         // but merged should
         DocumentParser documentParser = new DocumentParser(null, null, () -> DocumentParsingObserver.EMPTY_INSTANCE);
-        DocumentMapper mergedMapper = new DocumentMapper(documentParser, merged, merged.toCompressedXContent());
+        DocumentMapper mergedMapper = new DocumentMapper(documentParser, merged, merged.toCompressedXContent(), IndexVersion.current());
         assertThat(mergedMapper.mappers().getMapper("age"), notNullValue());
         assertThat(mergedMapper.mappers().getMapper("obj1.prop1"), notNullValue());
     }

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java
@@ -20,6 +20,7 @@
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.index.IndexSettings;
+import org.elasticsearch.index.IndexVersion;
 import org.elasticsearch.plugins.MapperPlugin;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.script.CompositeFieldScript;
@@ -2530,7 +2531,12 @@ same name need to be part  of the same mappings (hence the same document). If th
 
         // merge without going through toXContent and reparsing, otherwise the potential leaf path issue gets fixed on its own
         Mapping newMapping = MapperService.mergeMappings(mapperService.documentMapper(), mapping, MapperService.MergeReason.MAPPING_UPDATE);
-        DocumentMapper newDocMapper = new DocumentMapper(mapperService.documentParser(), newMapping, newMapping.toCompressedXContent());
+        DocumentMapper newDocMapper = new DocumentMapper(
+            mapperService.documentParser(),
+            newMapping,
+            newMapping.toCompressedXContent(),
+            IndexVersion.current()
+        );
         ParsedDocument doc2 = newDocMapper.parse(source("""
             {
               "foo" : {

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/SourceFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/SourceFieldMapperTests.java
@@ -12,6 +12,7 @@
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.xcontent.XContentHelper;
+import org.elasticsearch.xcontent.XContentBuilder;
 import org.elasticsearch.xcontent.XContentFactory;
 import org.elasticsearch.xcontent.XContentParser;
 import org.elasticsearch.xcontent.XContentType;
@@ -227,4 +228,14 @@ public void testSyntheticUpdates() throws Exception {
         assertFalse(mapper.enabled());
         assertFalse(mapper.isSynthetic());
     }
+
+    public void testSyntheticSourceInTimeSeries() throws IOException {
+        XContentBuilder mapping = fieldMapping(b -> {
+            b.field("type", "keyword");
+            b.field("time_series_dimension", true);
+        });
+        DocumentMapper mapper = createTimeSeriesModeDocumentMapper(mapping);
+        assertTrue(mapper.sourceMapper().isSynthetic());
+        assertEquals("{\"_source\":{\"mode\":\"synthetic\"}}", mapper.sourceMapper().toString());
+    }
 }