Improve cross-type dynamic mapping updates.

Today when dynamically mapping a field that is already defined in another type,
we use the regular dynamic mapping logic and try to copy some settings to avoid
introducing conflicts. However this is quite fragile as we don't deal with every
existing setting. This proposes a different approach that will just reuse the
shared field type.

Close #15568

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

@@ -596,40 +596,22 @@ class DocumentParser implements Closeable {
         if (dynamic == ObjectMapper.Dynamic.FALSE) {
             return null;
         }
+        final String path = context.path().pathAsText(currentFieldName);
         final Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings(), context.path());
-        final MappedFieldType existingFieldType = context.mapperService().fullName(context.path().pathAsText(currentFieldName));
+        final MappedFieldType existingFieldType = context.mapperService().fullName(path);
         Mapper.Builder builder = null;
         if (existingFieldType != null) {
             // create a builder of the same type
             builder = createBuilderFromFieldType(context, existingFieldType, currentFieldName);
-            if (builder != null) {
-                // best-effort to not introduce a conflict
-                if (builder instanceof StringFieldMapper.Builder) {
-                    StringFieldMapper.Builder stringBuilder = (StringFieldMapper.Builder) builder;
-                    stringBuilder.fieldDataSettings(existingFieldType.fieldDataType().getSettings());
-                    stringBuilder.store(existingFieldType.stored());
-                    stringBuilder.indexOptions(existingFieldType.indexOptions());
-                    stringBuilder.tokenized(existingFieldType.tokenized());
-                    stringBuilder.omitNorms(existingFieldType.omitNorms());
-                    stringBuilder.docValues(existingFieldType.hasDocValues());
-                    stringBuilder.indexAnalyzer(existingFieldType.indexAnalyzer());
-                    stringBuilder.searchAnalyzer(existingFieldType.searchAnalyzer());
-                } else if (builder instanceof NumberFieldMapper.Builder) {
-                    NumberFieldMapper.Builder<?,?> numberBuilder = (NumberFieldMapper.Builder<?, ?>) builder;
-                    numberBuilder.fieldDataSettings(existingFieldType.fieldDataType().getSettings());
-                    numberBuilder.store(existingFieldType.stored());
-                    numberBuilder.indexOptions(existingFieldType.indexOptions());
-                    numberBuilder.tokenized(existingFieldType.tokenized());
-                    numberBuilder.omitNorms(existingFieldType.omitNorms());
-                    numberBuilder.docValues(existingFieldType.hasDocValues());
-                    numberBuilder.precisionStep(existingFieldType.numericPrecisionStep());
-                }
-            }
         }
         if (builder == null) {
             builder = createBuilderFromDynamicValue(context, token, currentFieldName);
         }
         Mapper mapper = builder.build(builderContext);
+        if (existingFieldType != null) {
+            // try to not introduce a conflict
+            mapper = mapper.updateFieldType(Collections.singletonMap(path, existingFieldType));
+        }
 
         mapper = parseAndMergeUpdate(mapper, context);
 

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

@@ -363,6 +363,8 @@ public abstract class FieldMapper extends Mapper implements Cloneable {
         final MappedFieldType newFieldType = fullNameToFieldType.get(fieldType.name());
         if (newFieldType == null) {
             throw new IllegalStateException();
+        } else if (fieldType.getClass() != newFieldType.getClass()) {
+            throw new IllegalStateException("Mixing up field types: " + fieldType.getClass() + " != " + newFieldType.getClass());
         }
         MultiFields updatedMultiFields = multiFields.updateFieldType(fullNameToFieldType);
         if (fieldType == newFieldType && multiFields == updatedMultiFields) {

core/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java

@@ -18,6 +18,7 @@
  */
 package org.elasticsearch.index.mapper;
 
+import org.apache.lucene.index.IndexOptions;
 import org.elasticsearch.Version;
 import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsResponse;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
@@ -30,8 +31,13 @@ import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.index.IndexService;
+import org.elasticsearch.index.mapper.core.DateFieldMapper;
+import org.elasticsearch.index.mapper.core.DateFieldMapper.DateFieldType;
+import org.elasticsearch.index.mapper.core.DoubleFieldMapper;
 import org.elasticsearch.index.mapper.core.FloatFieldMapper;
 import org.elasticsearch.index.mapper.core.IntegerFieldMapper;
+import org.elasticsearch.index.mapper.core.LongFieldMapper;
+import org.elasticsearch.index.mapper.core.LongFieldMapper.LongFieldType;
 import org.elasticsearch.index.mapper.core.StringFieldMapper;
 import org.elasticsearch.test.ESSingleNodeTestCase;
 
@@ -367,17 +373,52 @@ public class DynamicMappingTests extends ESSingleNodeTestCase {
     }
 
     public void testReuseExistingMappings() throws IOException, Exception {
-        IndexService indexService = createIndex("test", Settings.EMPTY, "type", "my_field1", "type=string,store=yes", "my_field2", "type=integer,precision_step=10");
+        IndexService indexService = createIndex("test", Settings.EMPTY, "type",
+                "my_field1", "type=string,store=yes",
+                "my_field2", "type=integer,precision_step=10",
+                "my_field3", "type=long,doc_values=false",
+                "my_field4", "type=float,index_options=freqs",
+                "my_field5", "type=double,precision_step=14",
+                "my_field6", "type=date,doc_values=false");
 
         // Even if the dynamic type of our new field is long, we already have a mapping for the same field
         // of type string so it should be mapped as a string
         DocumentMapper newMapper = indexService.mapperService().documentMapperWithAutoCreate("type2").getDocumentMapper();
         Mapper update = parse(newMapper, indexService.mapperService().documentMapperParser(),
-                XContentFactory.jsonBuilder().startObject().field("my_field1", 42).endObject());
+                XContentFactory.jsonBuilder().startObject()
+                    .field("my_field1", 42)
+                    .field("my_field2", 43)
+                    .field("my_field3", 44)
+                    .field("my_field4", 45)
+                    .field("my_field5", 46)
+                    .field("my_field6", 47)
+                .endObject());
         Mapper myField1Mapper = null;
+        Mapper myField2Mapper = null;
+        Mapper myField3Mapper = null;
+        Mapper myField4Mapper = null;
+        Mapper myField5Mapper = null;
+        Mapper myField6Mapper = null;
         for (Mapper m : update) {
-            if (m.name().equals("my_field1")) {
+            switch (m.name()) {
+            case "my_field1":
                 myField1Mapper = m;
+                break;
+            case "my_field2":
+                myField2Mapper = m;
+                break;
+            case "my_field3":
+                myField3Mapper = m;
+                break;
+            case "my_field4":
+                myField4Mapper = m;
+                break;
+            case "my_field5":
+                myField5Mapper = m;
+                break;
+            case "my_field6":
+                myField6Mapper = m;
+                break;
             }
         }
         assertNotNull(myField1Mapper);
@@ -388,20 +429,28 @@ public class DynamicMappingTests extends ESSingleNodeTestCase {
 
         // Even if dynamic mappings would map a numeric field as a long, here it should map it as a integer
         // since we already have a mapping of type integer
-        update = parse(newMapper, indexService.mapperService().documentMapperParser(),
-                XContentFactory.jsonBuilder().startObject().field("my_field2", 42).endObject());
-        Mapper myField2Mapper = null;
-        for (Mapper m : update) {
-            if (m.name().equals("my_field2")) {
-                myField2Mapper = m;
-            }
-        }
         assertNotNull(myField2Mapper);
         // same type
         assertTrue(myField2Mapper instanceof IntegerFieldMapper);
         // and same option
         assertEquals(10, ((IntegerFieldMapper) myField2Mapper).fieldType().numericPrecisionStep());
 
+        assertNotNull(myField3Mapper);
+        assertTrue(myField3Mapper instanceof LongFieldMapper);
+        assertFalse(((LongFieldType) ((LongFieldMapper) myField3Mapper).fieldType()).hasDocValues());
+
+        assertNotNull(myField4Mapper);
+        assertTrue(myField4Mapper instanceof FloatFieldMapper);
+        assertEquals(IndexOptions.DOCS_AND_FREQS, ((FieldMapper) myField4Mapper).fieldType().indexOptions());
+
+        assertNotNull(myField5Mapper);
+        assertTrue(myField5Mapper instanceof DoubleFieldMapper);
+        assertEquals(14, ((DoubleFieldMapper) myField5Mapper).fieldType().numericPrecisionStep());
+
+        assertNotNull(myField6Mapper);
+        assertTrue(myField6Mapper instanceof DateFieldMapper);
+        assertFalse(((DateFieldType) ((DateFieldMapper) myField6Mapper).fieldType()).hasDocValues());
+
         // This can't work
         try {
             parse(newMapper, indexService.mapperService().documentMapperParser(),