From 3015eb3088c93b6833e2ec19a866fc1904b04eaf Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Wed, 23 Dec 2015 16:58:02 +0100 Subject: [PATCH] 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 --- .../index/mapper/DocumentParser.java | 30 ++------ .../index/mapper/FieldMapper.java | 2 + .../index/mapper/DynamicMappingTests.java | 71 ++++++++++++++++--- 3 files changed, 68 insertions(+), 35 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 4eb3100c99c..dd89ad37b44 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/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); diff --git a/core/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java index 3ab0ec86303..9bf58f6107f 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/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) { diff --git a/core/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java b/core/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java index d38e458248a..96e5a2fe80e 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java +++ b/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(),