From b9600c78918579181164a03ba25579c8ea3b3f9e Mon Sep 17 00:00:00 2001 From: Yannick Welsch Date: Thu, 15 Dec 2016 09:20:28 +0100 Subject: [PATCH] Only update DocumentMapper if field type changes (#22165) Merging mappings ensures that fields are used consistently across mapping types. Disabling norms for a specific field in one mapping type for example also disables norms for the same field in other mapping types of that index. The logic that ensures this while merging mappings currently always creates a fresh document mapper for all existing mapping types, even if no change occurred. Creating such a fresh document mapper does not come for free though as it involves recompressing the source. Making a mapping change to one type of an index with 100 types will thus re-serialize and recompress all 100 types, independent of any changes made to those types. This commit fixes the update logic to only create a new DocumentMapper if a field type actually changes. --- .../index/mapper/DocumentMapper.java | 5 ++ .../index/mapper/FieldTypeLookup.java | 2 +- .../elasticsearch/index/mapper/Mapping.java | 18 +++++-- .../index/mapper/FieldTypeLookupTests.java | 7 +-- .../index/mapper/MapperServiceTests.java | 52 ++++++++++++++----- 5 files changed, 64 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java index c7ee704de4f..fbe82a70bd7 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java @@ -327,6 +327,11 @@ public class DocumentMapper implements ToXContent { */ public DocumentMapper updateFieldType(Map fullNameToFieldType) { Mapping updated = this.mapping.updateFieldType(fullNameToFieldType); + if (updated == this.mapping) { + // no change + return this; + } + assert updated == updated.updateFieldType(fullNameToFieldType) : "updateFieldType operation is not idempotent"; return new DocumentMapper(mapperService, updated); } diff --git a/core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java b/core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java index 5f6fddf09ef..d05077fe5cf 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java @@ -93,7 +93,7 @@ class FieldTypeLookup implements Iterable { // is the update even legal? checkCompatibility(type, fieldMapper, updateAllTypes); - if (fieldType != fullNameFieldType) { + if (fieldType.equals(fullNameFieldType) == false) { fullName = fullName.copyAndPut(fieldType.name(), fieldMapper.fieldType()); } diff --git a/core/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/core/src/main/java/org/elasticsearch/index/mapper/Mapping.java index 0b92dbe4517..9cb3ea7a57e 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -104,12 +104,22 @@ public final class Mapping implements ToXContent { * Recursively update sub field types. */ public Mapping updateFieldType(Map fullNameToFieldType) { - final MetadataFieldMapper[] updatedMeta = Arrays.copyOf(metadataMappers, metadataMappers.length); - for (int i = 0; i < updatedMeta.length; ++i) { - updatedMeta[i] = (MetadataFieldMapper) updatedMeta[i].updateFieldType(fullNameToFieldType); + MetadataFieldMapper[] updatedMeta = null; + for (int i = 0; i < metadataMappers.length; ++i) { + MetadataFieldMapper currentFieldMapper = metadataMappers[i]; + MetadataFieldMapper updatedFieldMapper = (MetadataFieldMapper) currentFieldMapper.updateFieldType(fullNameToFieldType); + if (updatedFieldMapper != currentFieldMapper) { + if (updatedMeta == null) { + updatedMeta = Arrays.copyOf(metadataMappers, metadataMappers.length); + } + updatedMeta[i] = updatedFieldMapper; + } } RootObjectMapper updatedRoot = root.updateFieldType(fullNameToFieldType); - return new Mapping(indexCreated, updatedRoot, updatedMeta, meta); + if (updatedMeta == null && updatedRoot == root) { + return this; + } + return new Mapping(indexCreated, updatedRoot, updatedMeta == null ? metadataMappers : updatedMeta, meta); } @Override diff --git a/core/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java b/core/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java index 80e5df4081c..1091e313416 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java @@ -72,11 +72,12 @@ public class FieldTypeLookupTests extends ESTestCase { MockFieldMapper f = new MockFieldMapper("foo"); MockFieldMapper f2 = new MockFieldMapper("foo"); FieldTypeLookup lookup = new FieldTypeLookup(); - lookup = lookup.copyAndAddAll("type1", newList(f), randomBoolean()); - FieldTypeLookup lookup2 = lookup.copyAndAddAll("type2", newList(f2), randomBoolean()); + lookup = lookup.copyAndAddAll("type1", newList(f), true); + FieldTypeLookup lookup2 = lookup.copyAndAddAll("type2", newList(f2), true); - assertSame(f2.fieldType(), lookup2.get("foo")); assertEquals(1, size(lookup2.iterator())); + assertSame(f.fieldType(), lookup2.get("foo")); + assertEquals(f2.fieldType(), lookup2.get("foo")); } public void testAddExistingIndexName() { diff --git a/core/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/core/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index a6270dfc953..87afdedf89d 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -19,16 +19,6 @@ package org.elasticsearch.index.mapper; -import java.io.IOException; -import java.io.UncheckedIOException; -import java.util.Arrays; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; -import java.util.concurrent.ExecutionException; -import java.util.function.Function; - import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.settings.Settings; @@ -39,8 +29,17 @@ import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.index.mapper.NumberFieldMapper.NumberFieldType; import org.elasticsearch.test.ESSingleNodeTestCase; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.concurrent.ExecutionException; +import java.util.function.Function; + import static org.hamcrest.CoreMatchers.containsString; -import static org.hamcrest.Matchers.hasToString; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.startsWith; @@ -169,7 +168,6 @@ public class MapperServiceTests extends ESSingleNodeTestCase { assertThat(mapperService.unmappedFieldType("string"), instanceOf(KeywordFieldType.class)); } - public void testMergeWithMap() throws Throwable { IndexService indexService1 = createIndex("index1"); MapperService mapperService = indexService1.mapperService(); @@ -187,4 +185,34 @@ public class MapperServiceTests extends ESSingleNodeTestCase { () -> mapperService.merge(mappings, false)); assertThat(e.getMessage(), startsWith("Failed to parse mapping [type1]: ")); } + + public void testOtherDocumentMappersOnlyUpdatedWhenChangingFieldType() throws IOException { + IndexService indexService = createIndex("test"); + + CompressedXContent simpleMapping = new CompressedXContent(XContentFactory.jsonBuilder().startObject() + .startObject("properties") + .startObject("field") + .field("type", "text") + .endObject() + .endObject().endObject().bytes()); + + indexService.mapperService().merge("type1", simpleMapping, MergeReason.MAPPING_UPDATE, true); + DocumentMapper documentMapper = indexService.mapperService().documentMapper("type1"); + + indexService.mapperService().merge("type2", simpleMapping, MergeReason.MAPPING_UPDATE, true); + assertSame(indexService.mapperService().documentMapper("type1"), documentMapper); + + CompressedXContent normsDisabledMapping = new CompressedXContent(XContentFactory.jsonBuilder().startObject() + .startObject("properties") + .startObject("field") + .field("type", "text") + .startObject("norms") + .field("enabled", false) + .endObject() + .endObject() + .endObject().endObject().bytes()); + + indexService.mapperService().merge("type3", normsDisabledMapping, MergeReason.MAPPING_UPDATE, true); + assertNotSame(indexService.mapperService().documentMapper("type1"), documentMapper); + } }