Make FieldTypeLookup immutable (#58162) (#58411)

FieldTypeLookup maps field names to their MappedFieldTypes. In the past, due to
the presence of multiple mapping types within a single index, this had to be updated
in-place because a mapping update might only affect one type. However, now that
we only have a single type per index, we can completely rebuild the FieldTypeLookup
on each update, removing lots of concurrency worries.

server/src/main/java/org/elasticsearch/index/mapper/DynamicKeyFieldTypeLookup.java

@@ -19,9 +19,6 @@
 
 package org.elasticsearch.index.mapper;
 
-import org.elasticsearch.common.collect.CopyOnWriteHashMap;
-
-import java.util.Collections;
 import java.util.Iterator;
 import java.util.Map;
 
@@ -36,7 +33,7 @@ import java.util.Map;
  * Flattened object fields live in the 'mapper-flattened' module.
  */
 class DynamicKeyFieldTypeLookup {
-    private final CopyOnWriteHashMap<String, DynamicKeyFieldMapper> mappers;
+    private final Map<String, DynamicKeyFieldMapper> mappers;
     private final Map<String, String> aliasToConcreteName;
 
     /**
@@ -45,25 +42,11 @@ class DynamicKeyFieldTypeLookup {
      */
     private final int maxKeyDepth;
 
-    DynamicKeyFieldTypeLookup() {
+    DynamicKeyFieldTypeLookup(Map<String, DynamicKeyFieldMapper> newMappers,
-        this.mappers = new CopyOnWriteHashMap<>();
+                              Map<String, String> aliasToConcreteName) {
-        this.aliasToConcreteName = Collections.emptyMap();
+        this.mappers = newMappers;
-        this.maxKeyDepth = 0;
-    }
-
-    private DynamicKeyFieldTypeLookup(CopyOnWriteHashMap<String, DynamicKeyFieldMapper> mappers,
-                                      Map<String, String> aliasToConcreteName,
-                                      int maxKeyDepth) {
-        this.mappers = mappers;
         this.aliasToConcreteName = aliasToConcreteName;
-        this.maxKeyDepth = maxKeyDepth;
+        this.maxKeyDepth = getMaxKeyDepth(mappers, aliasToConcreteName);
-    }
-
-    DynamicKeyFieldTypeLookup copyAndAddAll(Map<String, DynamicKeyFieldMapper> newMappers,
-                                            Map<String, String> aliasToConcreteName) {
-        CopyOnWriteHashMap<String, DynamicKeyFieldMapper> combinedMappers = this.mappers.copyAndPutAll(newMappers);
-        int maxKeyDepth = getMaxKeyDepth(combinedMappers, aliasToConcreteName);
-        return new DynamicKeyFieldTypeLookup(combinedMappers, aliasToConcreteName, maxKeyDepth);
     }
 
     /**

server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java

@@ -19,16 +19,15 @@
 
 package org.elasticsearch.index.mapper;
 
-import org.elasticsearch.common.collect.CopyOnWriteHashMap;
 import org.elasticsearch.common.collect.Iterators;
 import org.elasticsearch.common.regex.Regex;
 
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 
 /**
@@ -36,52 +35,23 @@ import java.util.Set;
  */
 class FieldTypeLookup implements Iterable<MappedFieldType> {
 
-    final CopyOnWriteHashMap<String, MappedFieldType> fullNameToFieldType;
+    private final Map<String, MappedFieldType> fullNameToFieldType = new HashMap<>();
-    private final CopyOnWriteHashMap<String, String> aliasToConcreteName;
+    private final Map<String, String> aliasToConcreteName = new HashMap<>();
     private final DynamicKeyFieldTypeLookup dynamicKeyLookup;
 
-
     FieldTypeLookup() {
-        fullNameToFieldType = new CopyOnWriteHashMap<>();
+        this(Collections.emptyList(), Collections.emptyList());
-        aliasToConcreteName = new CopyOnWriteHashMap<>();
-        dynamicKeyLookup = new DynamicKeyFieldTypeLookup();
     }
 
-    private FieldTypeLookup(CopyOnWriteHashMap<String, MappedFieldType> fullNameToFieldType,
+    FieldTypeLookup(Collection<FieldMapper> fieldMappers,
-                            CopyOnWriteHashMap<String, String> aliasToConcreteName,
+                    Collection<FieldAliasMapper> fieldAliasMappers) {
-                            DynamicKeyFieldTypeLookup dynamicKeyLookup) {
-        this.fullNameToFieldType = fullNameToFieldType;
-        this.aliasToConcreteName = aliasToConcreteName;
-        this.dynamicKeyLookup = dynamicKeyLookup;
-    }
 
-    /**
-     * Return a new instance that contains the union of this instance and the field types
-     * from the provided mappers. If a field already exists, its field type will be updated
-     * to use the new type from the given field mapper. Similarly if an alias already
-     * exists, it will be updated to reference the field type from the new mapper.
-     */
-    public FieldTypeLookup copyAndAddAll(String type,
-                                         Collection<FieldMapper> fieldMappers,
-                                         Collection<FieldAliasMapper> fieldAliasMappers) {
-        Objects.requireNonNull(type, "type must not be null");
-        if (MapperService.DEFAULT_MAPPING.equals(type)) {
-            throw new IllegalArgumentException("Default mappings should not be added to the lookup");
-        }
-
-        CopyOnWriteHashMap<String, MappedFieldType> fullName = this.fullNameToFieldType;
-        CopyOnWriteHashMap<String, String> aliases = this.aliasToConcreteName;
         Map<String, DynamicKeyFieldMapper> dynamicKeyMappers = new HashMap<>();
 
         for (FieldMapper fieldMapper : fieldMappers) {
             String fieldName = fieldMapper.name();
             MappedFieldType fieldType = fieldMapper.fieldType();
-            MappedFieldType fullNameFieldType = fullName.get(fieldType.name());
+            fullNameToFieldType.put(fieldType.name(), fieldType);
-
-            if (!Objects.equals(fieldType, fullNameFieldType)) {
-                fullName = fullName.copyAndPut(fieldType.name(), fieldType);
-            }
-
             if (fieldMapper instanceof DynamicKeyFieldMapper) {
                 dynamicKeyMappers.put(fieldName, (DynamicKeyFieldMapper) fieldMapper);
             }
@@ -90,11 +60,10 @@ class FieldTypeLookup implements Iterable<MappedFieldType> {
         for (FieldAliasMapper fieldAliasMapper : fieldAliasMappers) {
             String aliasName = fieldAliasMapper.name();
             String path = fieldAliasMapper.path();
-            aliases = aliases.copyAndPut(aliasName, path);
+            aliasToConcreteName.put(aliasName, path);
         }
 
-        DynamicKeyFieldTypeLookup newDynamicKeyLookup = this.dynamicKeyLookup.copyAndAddAll(dynamicKeyMappers, aliases);
+        this.dynamicKeyLookup = new DynamicKeyFieldTypeLookup(dynamicKeyMappers, aliasToConcreteName);
-        return new FieldTypeLookup(fullName, aliases, newDynamicKeyLookup);
     }
 
     /**

server/src/main/java/org/elasticsearch/index/mapper/MapperMergeValidator.java

@@ -38,12 +38,10 @@ class MapperMergeValidator {
      * @param objectMappers The newly added object mappers.
      * @param fieldMappers The newly added field mappers.
      * @param fieldAliasMappers The newly added field alias mappers.
-     * @param fieldTypes Any existing field and field alias mappers, collected into a lookup structure.
      */
     public static void validateNewMappers(Collection<ObjectMapper> objectMappers,
                                           Collection<FieldMapper> fieldMappers,
-                                          Collection<FieldAliasMapper> fieldAliasMappers,
+                                          Collection<FieldAliasMapper> fieldAliasMappers) {
-                                          FieldTypeLookup fieldTypes) {
         Set<String> objectFullNames = new HashSet<>();
         for (ObjectMapper objectMapper : objectMappers) {
             String fullPath = objectMapper.fullPath();

server/src/main/java/org/elasticsearch/index/mapper/MapperService.java

@@ -459,7 +459,6 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
                                                                    MergeReason reason) {
         boolean hasNested = this.hasNested;
         Map<String, ObjectMapper> fullPathObjectMappers = this.fullPathObjectMappers;
-        FieldTypeLookup fieldTypes = this.fieldTypes;
 
         Map<String, DocumentMapper> results = new LinkedHashMap<>(2);
 
@@ -474,6 +473,7 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
         }
 
         DocumentMapper newMapper = null;
+        FieldTypeLookup newFieldTypes = null;
         if (mapper != null) {
             // check naming
             validateTypeName(mapper.type());
@@ -494,11 +494,11 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
             Collections.addAll(fieldMappers, metadataMappers);
             MapperUtils.collect(newMapper.mapping().root(), objectMappers, fieldMappers, fieldAliasMappers);
 
-            MapperMergeValidator.validateNewMappers(objectMappers, fieldMappers, fieldAliasMappers, fieldTypes);
+            MapperMergeValidator.validateNewMappers(objectMappers, fieldMappers, fieldAliasMappers);
             checkPartitionedIndexConstraints(newMapper);
 
             // update lookup data-structures
-            fieldTypes = fieldTypes.copyAndAddAll(newMapper.type(), fieldMappers, fieldAliasMappers);
+            newFieldTypes = new FieldTypeLookup(fieldMappers, fieldAliasMappers);
 
             for (ObjectMapper objectMapper : objectMappers) {
                 if (fullPathObjectMappers == this.fullPathObjectMappers) {
@@ -513,9 +513,9 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
             }
 
             MapperMergeValidator.validateFieldReferences(fieldMappers, fieldAliasMappers,
-                fullPathObjectMappers, fieldTypes);
+                fullPathObjectMappers, newFieldTypes);
 
-            ContextMapping.validateContextPaths(indexSettings.getIndexVersionCreated(), fieldMappers, fieldTypes::get);
+            ContextMapping.validateContextPaths(indexSettings.getIndexVersionCreated(), fieldMappers, newFieldTypes::get);
 
             if (reason == MergeReason.MAPPING_UPDATE || reason == MergeReason.MAPPING_UPDATE_PREFLIGHT) {
                 // this check will only be performed on the master node when there is
@@ -563,8 +563,8 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
         }
         if (newMapper != null) {
             this.mapper = newMapper;
+            this.fieldTypes = newFieldTypes;
         }
-        this.fieldTypes = fieldTypes;
         this.hasNested = hasNested;
         this.fullPathObjectMappers = fullPathObjectMappers;
 

server/src/test/java/org/elasticsearch/index/mapper/FieldTypeLookupTests.java

@@ -23,8 +23,8 @@ import org.elasticsearch.test.ESTestCase;
 
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Iterator;
-import java.util.List;
 
 import static java.util.Collections.emptyList;
 
@@ -41,100 +41,24 @@ public class FieldTypeLookupTests extends ESTestCase {
         assertFalse(itr.hasNext());
     }
 
-    public void testDefaultMapping() {
-        FieldTypeLookup lookup = new FieldTypeLookup();
-        try {
-            lookup.copyAndAddAll(MapperService.DEFAULT_MAPPING, emptyList(), emptyList());
-            fail();
-        } catch (IllegalArgumentException expected) {
-            assertEquals("Default mappings should not be added to the lookup", expected.getMessage());
-        }
-    }
-
     public void testAddNewField() {
-        FieldTypeLookup lookup = new FieldTypeLookup();
         MockFieldMapper f = new MockFieldMapper("foo");
-        FieldTypeLookup lookup2 = lookup.copyAndAddAll("type", newList(f), emptyList());
+        FieldTypeLookup lookup = new FieldTypeLookup(Collections.singletonList(f), emptyList());
-        assertNull(lookup.get("foo"));
         assertNull(lookup.get("bar"));
-        assertEquals(f.fieldType(), lookup2.get("foo"));
+        assertEquals(f.fieldType(), lookup.get("foo"));
-        assertNull(lookup.get("bar"));
+        assertEquals(1, size(lookup.iterator()));
-        assertEquals(1, size(lookup2.iterator()));
-    }
-
-    public void testAddExistingField() {
-        MockFieldMapper f = new MockFieldMapper("foo");
-        MockFieldMapper f2 = new MockFieldMapper("foo");
-        FieldTypeLookup lookup = new FieldTypeLookup();
-        lookup = lookup.copyAndAddAll("type1", newList(f), emptyList());
-        FieldTypeLookup lookup2 = lookup.copyAndAddAll("type2", newList(f2), emptyList());
-
-        assertEquals(1, size(lookup2.iterator()));
-        assertSame(f.fieldType(), lookup2.get("foo"));
-        assertEquals(f2.fieldType(), lookup2.get("foo"));
     }
 
     public void testAddFieldAlias() {
         MockFieldMapper field = new MockFieldMapper("foo");
         FieldAliasMapper alias = new FieldAliasMapper("alias", "alias", "foo");
 
-        FieldTypeLookup lookup = new FieldTypeLookup();
+        FieldTypeLookup lookup = new FieldTypeLookup(Collections.singletonList(field), Collections.singletonList(alias));
-        lookup = lookup.copyAndAddAll("type", newList(field), newList(alias));
 
         MappedFieldType aliasType = lookup.get("alias");
         assertEquals(field.fieldType(), aliasType);
     }
 
-    public void testUpdateFieldAlias() {
-        // Add an alias 'alias' to the concrete field 'foo'.
-        MockFieldMapper.FakeFieldType fieldType1 = new MockFieldMapper.FakeFieldType("foo");
-        MockFieldMapper field1 = new MockFieldMapper(fieldType1);
-        FieldAliasMapper alias1 = new FieldAliasMapper("alias", "alias", "foo");
-
-        FieldTypeLookup lookup = new FieldTypeLookup();
-        lookup = lookup.copyAndAddAll("type", newList(field1), newList(alias1));
-
-        // Check that the alias refers to 'foo'.
-        MappedFieldType aliasType1 = lookup.get("alias");
-        assertEquals(fieldType1, aliasType1);
-
-        // Update the alias to refer to a new concrete field 'bar'.
-        MockFieldMapper.FakeFieldType fieldType2 = new MockFieldMapper.FakeFieldType("bar");
-        MockFieldMapper field2 = new MockFieldMapper(fieldType2);
-
-        FieldAliasMapper alias2 = new FieldAliasMapper("alias", "alias", "bar");
-        lookup = lookup.copyAndAddAll("type", newList(field2), newList(alias2));
-
-        // Check that the alias now refers to 'bar'.
-        MappedFieldType aliasType2 = lookup.get("alias");
-        assertEquals(fieldType2, aliasType2);
-    }
-
-    public void testUpdateConcreteFieldWithAlias() {
-        // Add an alias 'alias' to the concrete field 'foo'.
-        FieldAliasMapper alias1 = new FieldAliasMapper("alias", "alias", "foo");
-        MockFieldMapper.FakeFieldType fieldType1 = new MockFieldMapper.FakeFieldType("foo");
-        fieldType1.setBoost(1.0f);
-        MockFieldMapper field1 = new MockFieldMapper(fieldType1);
-
-        FieldTypeLookup lookup = new FieldTypeLookup();
-        lookup = lookup.copyAndAddAll("type", newList(field1), newList(alias1));
-
-        // Check that the alias maps to this field type.
-        MappedFieldType aliasType1 = lookup.get("alias");
-        assertEquals(fieldType1, aliasType1);
-
-        // Update the boost for field 'foo'.
-        MockFieldMapper.FakeFieldType fieldType2 = new MockFieldMapper.FakeFieldType("foo");
-        fieldType2.setBoost(2.0f);
-        MockFieldMapper field2 = new MockFieldMapper(fieldType2);
-        lookup = lookup.copyAndAddAll("type", newList(field2), emptyList());
-
-        // Check that the alias maps to the new field type.
-        MappedFieldType aliasType2 = lookup.get("alias");
-        assertEquals(fieldType2, aliasType2);
-    }
-
     public void testSimpleMatchToFullName() {
         MockFieldMapper field1 = new MockFieldMapper("foo");
         MockFieldMapper field2 = new MockFieldMapper("bar");
@@ -142,10 +66,7 @@ public class FieldTypeLookupTests extends ESTestCase {
         FieldAliasMapper alias1 = new FieldAliasMapper("food", "food", "foo");
         FieldAliasMapper alias2 = new FieldAliasMapper("barometer", "barometer", "bar");
 
-        FieldTypeLookup lookup = new FieldTypeLookup();
+        FieldTypeLookup lookup = new FieldTypeLookup(Arrays.asList(field1, field2), Arrays.asList(alias1, alias2));
-        lookup = lookup.copyAndAddAll("type",
-            newList(field1, field2),
-            newList(alias1, alias2));
 
         Collection<String> names = lookup.simpleMatchToFullName("b*");
 
@@ -158,8 +79,7 @@ public class FieldTypeLookupTests extends ESTestCase {
 
     public void testIteratorImmutable() {
         MockFieldMapper f1 = new MockFieldMapper("foo");
-        FieldTypeLookup lookup = new FieldTypeLookup();
+        FieldTypeLookup lookup = new FieldTypeLookup(Collections.singletonList(f1), emptyList());
-        lookup = lookup.copyAndAddAll("type", newList(f1), emptyList());
 
         try {
             Iterator<MappedFieldType> itr = lookup.iterator();
@@ -172,14 +92,6 @@ public class FieldTypeLookupTests extends ESTestCase {
         }
     }
 
-    private static List<FieldMapper> newList(FieldMapper... mapper) {
-        return Arrays.asList(mapper);
-    }
-
-    private static List<FieldAliasMapper> newList(FieldAliasMapper... mapper) {
-        return Arrays.asList(mapper);
-    }
-
     private int size(Iterator<MappedFieldType> iterator) {
         if (iterator == null) {
             throw new NullPointerException("iterator");

server/src/test/java/org/elasticsearch/index/mapper/MapperMergeValidatorTests.java

@@ -40,8 +40,7 @@ public class MapperMergeValidatorTests extends ESTestCase {
             MapperMergeValidator.validateNewMappers(
                 singletonList(objectMapper),
                 emptyList(),
-                singletonList(aliasMapper),
+                singletonList(aliasMapper)));
-                new FieldTypeLookup()));
         assertEquals("Field [some.path] is defined both as an object and a field.", e.getMessage());
     }
 
@@ -54,8 +53,7 @@ public class MapperMergeValidatorTests extends ESTestCase {
             MapperMergeValidator.validateNewMappers(
                 emptyList(),
                 Arrays.asList(field, invalidField),
-                singletonList(invalidAlias),
+                singletonList(invalidAlias)));
-                new FieldTypeLookup()));
 
         assertEquals("Field [invalid] is defined both as an alias and a concrete field.", e.getMessage());
     }
@@ -69,8 +67,7 @@ public class MapperMergeValidatorTests extends ESTestCase {
             MapperMergeValidator.validateNewMappers(
                 emptyList(),
                 singletonList(field),
-                Arrays.asList(alias, invalidAlias),
+                Arrays.asList(alias, invalidAlias)));
-                new FieldTypeLookup()));
 
         assertEquals("Invalid [path] value [alias] for field alias [invalid-alias]: an alias" +
             " cannot refer to another alias.", e.getMessage());
@@ -83,8 +80,7 @@ public class MapperMergeValidatorTests extends ESTestCase {
             MapperMergeValidator.validateNewMappers(
                 emptyList(),
                 emptyList(),
-                singletonList(invalidAlias),
+                singletonList(invalidAlias)));
-                new FieldTypeLookup()));
 
         assertEquals("Invalid [path] value [invalid-alias] for field alias [invalid-alias]: an alias" +
             " cannot refer to itself.", e.getMessage());
@@ -97,8 +93,7 @@ public class MapperMergeValidatorTests extends ESTestCase {
             MapperMergeValidator.validateNewMappers(
                 emptyList(),
                 emptyList(),
-                singletonList(invalidAlias),
+                singletonList(invalidAlias)));
-                new FieldTypeLookup()));
 
         assertEquals("Invalid [path] value [non-existent] for field alias [invalid-alias]: an alias" +
             " must refer to an existing field in the mappings.", e.getMessage());

x-pack/plugin/mapper-flattened/src/test/java/org/elasticsearch/index/mapper/FlatObjectFieldLookupTests.java

@@ -40,8 +40,7 @@ public class FlatObjectFieldLookupTests extends ESTestCase {
         String fieldName = "object1.object2.field";
         FlatObjectFieldMapper mapper = createFlatObjectMapper(fieldName);
 
-        FieldTypeLookup lookup = new FieldTypeLookup()
+        FieldTypeLookup lookup = new FieldTypeLookup(singletonList(mapper), emptyList());
-            .copyAndAddAll("type", singletonList(mapper), emptyList());
         assertEquals(mapper.fieldType(), lookup.get(fieldName));
 
         String objectKey = "key1.key2";
@@ -62,8 +61,7 @@ public class FlatObjectFieldLookupTests extends ESTestCase {
         String aliasName = "alias";
         FieldAliasMapper alias = new FieldAliasMapper(aliasName, aliasName, fieldName);
 
-        FieldTypeLookup lookup = new FieldTypeLookup()
+        FieldTypeLookup lookup = new FieldTypeLookup(singletonList(mapper), singletonList(alias));
-            .copyAndAddAll("type", singletonList(mapper), singletonList(alias));
         assertEquals(mapper.fieldType(), lookup.get(aliasName));
 
         String objectKey = "key1.key2";
@@ -86,12 +84,11 @@ public class FlatObjectFieldLookupTests extends ESTestCase {
         FlatObjectFieldMapper mapper2 = createFlatObjectMapper(field2);
         FlatObjectFieldMapper mapper3 = createFlatObjectMapper(field3);
 
-        FieldTypeLookup lookup = new FieldTypeLookup()
+        FieldTypeLookup lookup = new FieldTypeLookup(Arrays.asList(mapper1, mapper2), emptyList());
-            .copyAndAddAll("type", Arrays.asList(mapper1, mapper2), emptyList());
         assertNotNull(lookup.get(field1 + ".some.key"));
         assertNotNull(lookup.get(field2 + ".some.key"));
 
-        lookup = lookup.copyAndAddAll("type", singletonList(mapper3), emptyList());
+        lookup = new FieldTypeLookup(Arrays.asList(mapper1, mapper2, mapper3), emptyList());
         assertNotNull(lookup.get(field1 + ".some.key"));
         assertNotNull(lookup.get(field2 + ".some.key"));
         assertNotNull(lookup.get(field3 + ".some.key"));
@@ -128,8 +125,7 @@ public class FlatObjectFieldLookupTests extends ESTestCase {
         MockFieldMapper mapper = new MockFieldMapper("foo");
         FlatObjectFieldMapper flatObjectMapper = createFlatObjectMapper("object1.object2.field");
 
-        FieldTypeLookup lookup = new FieldTypeLookup()
+        FieldTypeLookup lookup = new FieldTypeLookup(Arrays.asList(mapper, flatObjectMapper), emptyList());
-            .copyAndAddAll("type", Arrays.asList(mapper, flatObjectMapper), emptyList());
 
         Set<String> fieldNames = new HashSet<>();
         for (MappedFieldType fieldType : lookup) {