Merge pull request #15633 from jpountz/fix/dynamic_mappings

Improve cross-type dynamic mapping updates.

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(),