From e6d087be5f710f1152123288e07e2482030b7abf Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 20 May 2020 10:52:58 +0100 Subject: [PATCH 1/7] Remove Mapper.updateFieldType() --- .../mapper/SearchAsYouTypeFieldMapper.java | 10 ----- .../join/mapper/ParentJoinFieldMapper.java | 13 ------ .../percolator/PercolatorFieldMapper.java | 24 ----------- .../index/mapper/DocumentMapper.java | 13 ------ .../index/mapper/DocumentParser.java | 4 -- .../index/mapper/FieldAliasMapper.java | 5 --- .../index/mapper/FieldMapper.java | 42 ------------------- .../elasticsearch/index/mapper/Mapper.java | 6 --- .../index/mapper/MapperService.java | 18 +++----- .../elasticsearch/index/mapper/Mapping.java | 22 ---------- .../index/mapper/ObjectMapper.java | 22 ---------- .../index/mapper/RootObjectMapper.java | 5 --- .../index/mapper/TextFieldMapper.java | 12 ------ .../index/mapper/ExternalMapper.java | 31 -------------- 14 files changed, 6 insertions(+), 221 deletions(-) diff --git a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java index 257e01cadf60f..d47793bd09929 100644 --- a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java @@ -647,16 +647,6 @@ public SearchAsYouTypeFieldMapper(String simpleName, this.maxShingleSize = maxShingleSize; } - @Override - public FieldMapper updateFieldType(Map fullNameToFieldType) { - SearchAsYouTypeFieldMapper fieldMapper = (SearchAsYouTypeFieldMapper) super.updateFieldType(fullNameToFieldType); - fieldMapper.prefixField = (PrefixFieldMapper) fieldMapper.prefixField.updateFieldType(fullNameToFieldType); - for (int i = 0; i < fieldMapper.shingleFields.length; i++) { - fieldMapper.shingleFields[i] = (ShingleFieldMapper) fieldMapper.shingleFields[i].updateFieldType(fullNameToFieldType); - } - return fieldMapper; - } - @Override protected void parseCreateField(ParseContext context) throws IOException { final String value = context.externalValueSet() ? context.externalValue().toString() : context.parser().textOrNull(); diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java index 02a607bb27a37..6bc1c8bf7f08e 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java @@ -356,19 +356,6 @@ protected void mergeOptions(FieldMapper other, List conflicts) { uniqueFieldMapper.setFieldMapper(this); } - @Override - public FieldMapper updateFieldType(Map fullNameToFieldType) { - ParentJoinFieldMapper fieldMapper = (ParentJoinFieldMapper) super.updateFieldType(fullNameToFieldType); - final List newMappers = new ArrayList<> (); - for (ParentIdFieldMapper mapper : fieldMapper.parentIdFields) { - newMappers.add((ParentIdFieldMapper) mapper.updateFieldType(fullNameToFieldType)); - } - fieldMapper.parentIdFields = Collections.unmodifiableList(newMappers); - this.uniqueFieldMapper = (MetaJoinFieldMapper) uniqueFieldMapper.updateFieldType(fullNameToFieldType); - uniqueFieldMapper.setFieldMapper(this); - return fieldMapper; - } - @Override protected void parseCreateField(ParseContext context) throws IOException { throw new UnsupportedOperationException("parsing is implemented in parse(), this method should NEVER be called"); diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java index 6b84480078f67..0db99ca3ae87a 100644 --- a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java +++ b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java @@ -359,30 +359,6 @@ Tuple, Map>> extractTermsAndRanges(IndexRead this.rangeFieldMapper = rangeFieldMapper; } - @Override - public FieldMapper updateFieldType(Map fullNameToFieldType) { - PercolatorFieldMapper updated = (PercolatorFieldMapper) super.updateFieldType(fullNameToFieldType); - KeywordFieldMapper queryTermsUpdated = (KeywordFieldMapper) queryTermsField.updateFieldType(fullNameToFieldType); - KeywordFieldMapper extractionResultUpdated = (KeywordFieldMapper) extractionResultField.updateFieldType(fullNameToFieldType); - BinaryFieldMapper queryBuilderUpdated = (BinaryFieldMapper) queryBuilderField.updateFieldType(fullNameToFieldType); - RangeFieldMapper rangeFieldMapperUpdated = (RangeFieldMapper) rangeFieldMapper.updateFieldType(fullNameToFieldType); - NumberFieldMapper msmFieldMapperUpdated = (NumberFieldMapper) minimumShouldMatchFieldMapper.updateFieldType(fullNameToFieldType); - - if (updated == this && queryTermsUpdated == queryTermsField && extractionResultUpdated == extractionResultField - && queryBuilderUpdated == queryBuilderField && rangeFieldMapperUpdated == rangeFieldMapper) { - return this; - } - if (updated == this) { - updated = (PercolatorFieldMapper) updated.clone(); - } - updated.queryTermsField = queryTermsUpdated; - updated.extractionResultField = extractionResultUpdated; - updated.queryBuilderField = queryBuilderUpdated; - updated.rangeFieldMapper = rangeFieldMapperUpdated; - updated.minimumShouldMatchFieldMapper = msmFieldMapperUpdated; - return updated; - } - @Override public void parse(ParseContext context) throws IOException { QueryShardContext queryShardContext = this.queryShardContext.get(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java index baa5fe5b85931..01d25da00685c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java @@ -307,19 +307,6 @@ public DocumentMapper merge(Mapping mapping) { return new DocumentMapper(mapperService, merged); } - /** - * Recursively update sub field types. - */ - 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); - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { return mapping.toXContent(builder, params); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index fc4b477472f4e..5a2a287f08748 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -812,10 +812,6 @@ private static void parseDynamicValue(final ParseContext context, ObjectMapper p 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)); - } context.addDynamicMapper(mapper); parseObjectOrField(context, mapper); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java index 5a1e28f9e2179..cc33f51982509 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java @@ -74,11 +74,6 @@ public Mapper merge(Mapper mergeWith) { return mergeWith; } - @Override - public Mapper updateFieldType(Map fullNameToFieldType) { - return this; - } - @Override public Iterator iterator() { return Collections.emptyIterator(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java index 00415a30956a2..77a17114518c3 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -413,27 +413,6 @@ private void mergeSharedOptions(FieldMapper mergeWith, List conflicts) { */ protected abstract void mergeOptions(FieldMapper other, List conflicts); - @Override - public FieldMapper updateFieldType(Map fullNameToFieldType) { - final MappedFieldType newFieldType = fullNameToFieldType.get(fieldType.name()); - if (newFieldType == null) { - // this field does not exist in the mappings yet - // this can happen if this mapper represents a mapping update - return this; - } else if (fieldType.getClass() != newFieldType.getClass()) { - throw new IllegalStateException("Mixing up field types: " + - fieldType.getClass() + " != " + newFieldType.getClass() + " on field " + fieldType.name()); - } - MultiFields updatedMultiFields = multiFields.updateFieldType(fullNameToFieldType); - if (fieldType == newFieldType && multiFields == updatedMultiFields) { - return this; // no change - } - FieldMapper updated = clone(); - updated.fieldType = newFieldType; - updated.multiFields = updatedMultiFields; - return updated; - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(simpleName()); @@ -637,27 +616,6 @@ public MultiFields merge(MultiFields mergeWith) { return new MultiFields(mappers); } - public MultiFields updateFieldType(Map fullNameToFieldType) { - ImmutableOpenMap.Builder newMappersBuilder = null; - - for (ObjectCursor cursor : mappers.values()) { - FieldMapper updated = cursor.value.updateFieldType(fullNameToFieldType); - if (updated != cursor.value) { - if (newMappersBuilder == null) { - newMappersBuilder = ImmutableOpenMap.builder(mappers); - } - newMappersBuilder.put(updated.simpleName(), updated); - } - } - - if (newMappersBuilder == null) { - return this; - } - - ImmutableOpenMap mappers = newMappersBuilder.build(); - return new MultiFields(mappers); - } - public Iterator iterator() { return StreamSupport.stream(mappers.values().spliterator(), false).map((p) -> (Mapper)p.value).iterator(); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java index 054a7725b8db1..7b432031238c6 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java @@ -172,10 +172,4 @@ public final String simpleName() { * Both {@code this} and {@code mergeWith} will be left unmodified. */ public abstract Mapper merge(Mapper mergeWith); - /** - * Update the field type of this mapper. This is necessary because some mapping updates - * can modify mappings across several types. This method must return a copy of the mapper - * so that the current mapper is not modified. - */ - public abstract Mapper updateFieldType(Map fullNameToFieldType); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index fe4f4311ca707..365f7d37dbfb8 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -63,6 +63,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.function.BooleanSupplier; import java.util.function.Function; @@ -402,13 +403,6 @@ private synchronized DocumentMapper internalMerge(DocumentMapper mapper, MergeRe } checkIndexSortCompatibility(indexSettings.getIndexSortConfig(), hasNested); - if (newMapper != null) { - DocumentMapper updatedDocumentMapper = newMapper.updateFieldType(fieldTypes.fullNameToFieldType); - if (updatedDocumentMapper != newMapper) { - newMapper = updatedDocumentMapper; - } - } - if (reason == MergeReason.MAPPING_UPDATE_PREFLIGHT) { return newMapper; } @@ -420,15 +414,13 @@ private synchronized DocumentMapper internalMerge(DocumentMapper mapper, MergeRe } // commit the change - if (newMapper != null) { - this.mapper = newMapper; - } + this.mapper = newMapper; this.fieldTypes = fieldTypes; this.hasNested = hasNested; this.fullPathObjectMappers = fullPathObjectMappers; assert assertMappersShareSameFieldType(); - assert newMapper == null || assertSerialization(newMapper); + assert assertSerialization(newMapper); return newMapper; } @@ -439,7 +431,9 @@ private boolean assertMappersShareSameFieldType() { Collections.addAll(fieldMappers, mapper.mapping().metadataMappers); MapperUtils.collect(mapper.root(), new ArrayList<>(), fieldMappers, new ArrayList<>()); for (FieldMapper fieldMapper : fieldMappers) { - assert fieldMapper.fieldType() == fieldTypes.get(fieldMapper.name()) : fieldMapper.name(); + assert Objects.equals(fieldMapper.fieldType(), fieldTypes.get(fieldMapper.name())) : + "Fieldtype mismatch: " + fieldMapper.name() + " " + fieldMapper.fieldType() + + " != " + fieldTypes.get(fieldMapper.name()); } } return true; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java index df98e356938e8..0cc730c33cdf6 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -104,28 +104,6 @@ public Mapping merge(Mapping mergeWith) { return new Mapping(indexCreated, mergedRoot, mergedMetadataMappers.values().toArray(new MetadataFieldMapper[0]), mergedMeta); } - /** - * Recursively update sub field types. - */ - public Mapping updateFieldType(Map 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); - if (updatedMeta == null && updatedRoot == root) { - return this; - } - return new Mapping(indexCreated, updatedRoot, updatedMeta == null ? metadataMappers : updatedMeta, meta); - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { root.toXContent(builder, params, new ToXContent() { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index c9d1b8fae75b2..412e356b111f2 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -498,28 +498,6 @@ private void checkObjectMapperParameters(final ObjectMapper mergeWith) { } } - @Override - public ObjectMapper updateFieldType(Map fullNameToFieldType) { - List updatedMappers = null; - for (Mapper mapper : this) { - Mapper updated = mapper.updateFieldType(fullNameToFieldType); - if (mapper != updated) { - if (updatedMappers == null) { - updatedMappers = new ArrayList<>(); - } - updatedMappers.add(updated); - } - } - if (updatedMappers == null) { - return this; - } - ObjectMapper updated = clone(); - for (Mapper updatedMapper : updatedMappers) { - updated.putMapper(updatedMapper); - } - return updated; - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { toXContent(builder, params, null); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 0cb6c8dd286ff..29a4d79582bf7 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -308,11 +308,6 @@ protected void doMerge(ObjectMapper mergeWith) { } } - @Override - public RootObjectMapper updateFieldType(Map fullNameToFieldType) { - return (RootObjectMapper) super.updateFieldType(fullNameToFieldType); - } - @Override protected void doXContent(XContentBuilder builder, ToXContent.Params params) throws IOException { final boolean includeDefaults = params.paramAsBoolean("include_defaults", false); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java index e95de424d5e87..b8a6c63cd1492 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java @@ -848,18 +848,6 @@ protected String contentType() { return CONTENT_TYPE; } - @Override - public FieldMapper updateFieldType(Map fullNameToFieldType) { - TextFieldMapper mapper = (TextFieldMapper) super.updateFieldType(fullNameToFieldType); - if (mapper.prefixFieldMapper != null) { - mapper.prefixFieldMapper = (PrefixFieldMapper) mapper.prefixFieldMapper.updateFieldType(fullNameToFieldType); - } - if (mapper.phraseFieldMapper != null) { - mapper.phraseFieldMapper = (PhraseFieldMapper) mapper.phraseFieldMapper.updateFieldType(fullNameToFieldType); - } - return mapper; - } - @Override protected void mergeOptions(FieldMapper other, List conflicts) { TextFieldMapper mw = (TextFieldMapper) other; diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java b/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java index e6b8f33177d37..fb730f675fbcf 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java @@ -203,37 +203,6 @@ protected void parseCreateField(ParseContext context) throws IOException { throw new UnsupportedOperationException(); } - @Override - public FieldMapper updateFieldType(Map fullNameToFieldType) { - ExternalMapper update = (ExternalMapper) super.updateFieldType(fullNameToFieldType); - MultiFields multiFieldsUpdate = multiFields.updateFieldType(fullNameToFieldType); - BinaryFieldMapper binMapperUpdate = (BinaryFieldMapper) binMapper.updateFieldType(fullNameToFieldType); - BooleanFieldMapper boolMapperUpdate = (BooleanFieldMapper) boolMapper.updateFieldType(fullNameToFieldType); - GeoPointFieldMapper pointMapperUpdate = (GeoPointFieldMapper) pointMapper.updateFieldType(fullNameToFieldType); - AbstractShapeGeometryFieldMapper shapeMapperUpdate = - (AbstractShapeGeometryFieldMapper) shapeMapper.updateFieldType(fullNameToFieldType); - TextFieldMapper stringMapperUpdate = (TextFieldMapper) stringMapper.updateFieldType(fullNameToFieldType); - if (update == this - && multiFieldsUpdate == multiFields - && binMapperUpdate == binMapper - && boolMapperUpdate == boolMapper - && pointMapperUpdate == pointMapper - && shapeMapperUpdate == shapeMapper - && stringMapperUpdate == stringMapper) { - return this; - } - if (update == this) { - update = (ExternalMapper) clone(); - } - update.multiFields = multiFieldsUpdate; - update.binMapper = binMapperUpdate; - update.boolMapper = boolMapperUpdate; - update.pointMapper = pointMapperUpdate; - update.shapeMapper = shapeMapperUpdate; - update.stringMapper = stringMapperUpdate; - return update; - } - @Override public Iterator iterator() { return Iterators.concat(super.iterator(), Arrays.asList(binMapper, boolMapper, pointMapper, shapeMapper, stringMapper).iterator()); From b66c60eba70c0cdfdb8b1dc6a358c19940346eab Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 20 May 2020 13:01:57 +0100 Subject: [PATCH 2/7] MetaJoinMapperField uses a fieldname lookup rather than holding a reference to the mapper --- .../join/mapper/MetaJoinFieldMapper.java | 25 +++++++++++-------- .../join/mapper/ParentJoinFieldMapper.java | 7 +++--- .../ChildrenToParentAggregatorTests.java | 3 ++- .../ParentToChildrenAggregatorTests.java | 3 ++- .../mapper/ParentJoinFieldMapperTests.java | 10 +++++--- .../index/mapper/MapperService.java | 4 +++ 6 files changed, 32 insertions(+), 20 deletions(-) diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/MetaJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/MetaJoinFieldMapper.java index e3e095edd0eb5..3ab36babfb33a 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/MetaJoinFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/MetaJoinFieldMapper.java @@ -59,21 +59,25 @@ static class Defaults { } static class Builder extends FieldMapper.Builder { - Builder() { + + final String joinField; + + Builder(String joinField) { super(NAME, Defaults.FIELD_TYPE, Defaults.FIELD_TYPE); builder = this; + this.joinField = joinField; } @Override public MetaJoinFieldMapper build(BuilderContext context) { fieldType.setName(NAME); - return new MetaJoinFieldMapper(name, fieldType, context.indexSettings()); + return new MetaJoinFieldMapper(name, joinField, (MetaJoinFieldType) fieldType, context.indexSettings()); } } public static class MetaJoinFieldType extends StringFieldType { - private ParentJoinFieldMapper mapper; + private String joinField; MetaJoinFieldType() {} @@ -110,8 +114,12 @@ public Object valueForDisplay(Object value) { return binaryValue.utf8ToString(); } - public ParentJoinFieldMapper getMapper() { - return mapper; + public void setJoinField(String joinField) { + this.joinField = joinField; + } + + public String getJoinField() { + return joinField; } @Override @@ -120,12 +128,10 @@ public Query existsQuery(QueryShardContext context) { } } - MetaJoinFieldMapper(String name, MappedFieldType fieldType, Settings indexSettings) { + MetaJoinFieldMapper(String name, String joinField, MetaJoinFieldType fieldType, Settings indexSettings) { super(name, fieldType, ParentIdFieldMapper.Defaults.FIELD_TYPE, indexSettings, MultiFields.empty(), CopyTo.empty()); - } + fieldType.setJoinField(joinField); - void setFieldMapper(ParentJoinFieldMapper mapper) { - fieldType().mapper = mapper; } @Override @@ -140,7 +146,6 @@ protected MetaJoinFieldMapper clone() { @Override protected void mergeOptions(FieldMapper other, List conflicts) { - } @Override diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java index 6bc1c8bf7f08e..5fbf7f4544282 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java @@ -85,7 +85,8 @@ public static class Defaults { public static ParentJoinFieldMapper getMapper(MapperService service) { MetaJoinFieldMapper.MetaJoinFieldType fieldType = (MetaJoinFieldMapper.MetaJoinFieldType) service.fieldType(MetaJoinFieldMapper.NAME); - return fieldType == null ? null : fieldType.getMapper(); + return fieldType == null ? null : + (ParentJoinFieldMapper) service.fieldMapper(fieldType.getJoinField()); } private static String getParentIdFieldName(String joinFieldName, String parentName) { @@ -160,7 +161,7 @@ public ParentJoinFieldMapper build(BuilderContext context) { }) .forEach(parentIdFields::add); checkParentFields(name(), parentIdFields); - MetaJoinFieldMapper unique = new MetaJoinFieldMapper.Builder().build(context); + MetaJoinFieldMapper unique = new MetaJoinFieldMapper.Builder(name).build(context); return new ParentJoinFieldMapper(name, fieldType, context.indexSettings(), unique, Collections.unmodifiableList(parentIdFields), eagerGlobalOrdinals); } @@ -262,7 +263,6 @@ protected ParentJoinFieldMapper(String simpleName, super(simpleName, fieldType, Defaults.FIELD_TYPE, indexSettings, MultiFields.empty(), CopyTo.empty()); this.parentIdFields = parentIdFields; this.uniqueFieldMapper = uniqueFieldMapper; - this.uniqueFieldMapper.setFieldMapper(this); this.eagerGlobalOrdinals = eagerGlobalOrdinals; } @@ -353,7 +353,6 @@ protected void mergeOptions(FieldMapper other, List conflicts) { this.eagerGlobalOrdinals = joinMergeWith.eagerGlobalOrdinals; this.parentIdFields = Collections.unmodifiableList(newParentIdFields); this.uniqueFieldMapper = (MetaJoinFieldMapper) uniqueFieldMapper.merge(joinMergeWith.uniqueFieldMapper); - uniqueFieldMapper.setFieldMapper(this); } @Override diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java index 7a7bc4e92ed9f..2750a53dec88a 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java @@ -278,8 +278,9 @@ protected MapperService mapperServiceMock() { ParentJoinFieldMapper joinFieldMapper = createJoinFieldMapper(); MapperService mapperService = mock(MapperService.class); MetaJoinFieldMapper.MetaJoinFieldType metaJoinFieldType = mock(MetaJoinFieldMapper.MetaJoinFieldType.class); - when(metaJoinFieldType.getMapper()).thenReturn(joinFieldMapper); + when(metaJoinFieldType.getJoinField()).thenReturn("join_field"); when(mapperService.fieldType("_parent_join")).thenReturn(metaJoinFieldType); + when(mapperService.fieldMapper("join_field")).thenReturn(joinFieldMapper); return mapperService; } diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java index cc302955234e5..ae2498ad84fe2 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java @@ -166,8 +166,9 @@ protected MapperService mapperServiceMock() { ParentJoinFieldMapper joinFieldMapper = createJoinFieldMapper(); MapperService mapperService = mock(MapperService.class); MetaJoinFieldMapper.MetaJoinFieldType metaJoinFieldType = mock(MetaJoinFieldMapper.MetaJoinFieldType.class); - when(metaJoinFieldType.getMapper()).thenReturn(joinFieldMapper); + when(metaJoinFieldType.getJoinField()).thenReturn("join_field"); when(mapperService.fieldType("_parent_join")).thenReturn(metaJoinFieldType); + when(mapperService.fieldMapper("join_field")).thenReturn(joinFieldMapper); return mapperService; } diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java index 0079da5d723b3..48a5e33f86d16 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java @@ -301,10 +301,11 @@ public void testUpdateRelations() throws Exception { .endObject() .endObject() .endObject().endObject()); - docMapper = indexService.mapperService().merge("type", new CompressedXContent(updateMapping), + docMapper = indexService.mapperService().merge("_doc", new CompressedXContent(updateMapping), MapperService.MergeReason.MAPPING_UPDATE); - assertTrue(docMapper.mappers().getMapper("join_field") == ParentJoinFieldMapper.getMapper(indexService.mapperService())); ParentJoinFieldMapper mapper = ParentJoinFieldMapper.getMapper(indexService.mapperService()); + assertNotNull(mapper); + assertEquals("join_field", mapper.name()); assertTrue(mapper.hasChild("child2")); assertFalse(mapper.hasParent("child2")); assertTrue(mapper.hasChild("grand_child2")); @@ -322,10 +323,11 @@ public void testUpdateRelations() throws Exception { .endObject() .endObject() .endObject().endObject()); - docMapper = indexService.mapperService().merge("type", new CompressedXContent(updateMapping), + docMapper = indexService.mapperService().merge("_doc", new CompressedXContent(updateMapping), MapperService.MergeReason.MAPPING_UPDATE); - assertTrue(docMapper.mappers().getMapper("join_field") == ParentJoinFieldMapper.getMapper(indexService.mapperService())); ParentJoinFieldMapper mapper = ParentJoinFieldMapper.getMapper(indexService.mapperService()); + assertNotNull(mapper); + assertEquals("join_field", mapper.name()); assertTrue(mapper.hasParent("other")); assertFalse(mapper.hasChild("other")); assertTrue(mapper.hasChild("child_other1")); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 365f7d37dbfb8..8599f3cac6597 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -584,6 +584,10 @@ public MappedFieldType fieldType(String fullName) { return fieldTypes.get(fullName); } + public Mapper fieldMapper(String fieldName) { + return documentMapper().mappers().getMapper(fieldName); + } + /** * Returns all the fields that match the given pattern. If the pattern is prefixed with a type * then the fields will be returned with a type prefix. From 4ca0d7f377f69fbd36b6a5e01f01d7398676ffea Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Thu, 21 May 2020 13:52:22 +0100 Subject: [PATCH 3/7] feedback --- .../join/mapper/ParentJoinFieldMapper.java | 11 ++++++++-- .../ChildrenToParentAggregatorTests.java | 8 +++++++- .../ParentToChildrenAggregatorTests.java | 8 +++++++- .../index/mapper/DocumentParser.java | 10 +--------- .../index/mapper/MapperService.java | 20 ------------------- .../aggregations/AggregatorTestCase.java | 8 +------- 6 files changed, 25 insertions(+), 40 deletions(-) diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java index 5fbf7f4544282..c7ebabcdc803b 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java @@ -34,6 +34,8 @@ import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.plain.SortedSetOrdinalsIndexFieldData; import org.elasticsearch.index.mapper.ContentPath; +import org.elasticsearch.index.mapper.DocumentFieldMappers; +import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.FieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper; @@ -85,8 +87,13 @@ public static class Defaults { public static ParentJoinFieldMapper getMapper(MapperService service) { MetaJoinFieldMapper.MetaJoinFieldType fieldType = (MetaJoinFieldMapper.MetaJoinFieldType) service.fieldType(MetaJoinFieldMapper.NAME); - return fieldType == null ? null : - (ParentJoinFieldMapper) service.fieldMapper(fieldType.getJoinField()); + if (fieldType == null) { + return null; + } + DocumentMapper mapper = service.documentMapper(); + String joinField = fieldType.getJoinField(); + DocumentFieldMappers fieldMappers = mapper.mappers(); + return (ParentJoinFieldMapper) fieldMappers.getMapper(joinField); } private static String getParentIdFieldName(String joinFieldName, String parentName) { diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java index 2750a53dec88a..dfb1d447dd63d 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java @@ -39,6 +39,8 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.Index; import org.elasticsearch.index.mapper.ContentPath; +import org.elasticsearch.index.mapper.DocumentFieldMappers; +import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.IdFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper; @@ -280,7 +282,11 @@ protected MapperService mapperServiceMock() { MetaJoinFieldMapper.MetaJoinFieldType metaJoinFieldType = mock(MetaJoinFieldMapper.MetaJoinFieldType.class); when(metaJoinFieldType.getJoinField()).thenReturn("join_field"); when(mapperService.fieldType("_parent_join")).thenReturn(metaJoinFieldType); - when(mapperService.fieldMapper("join_field")).thenReturn(joinFieldMapper); + DocumentFieldMappers fieldMappers = new DocumentFieldMappers(Collections.singleton(joinFieldMapper), + Collections.emptyList(), null, null, null); + DocumentMapper mockMapper = mock(DocumentMapper.class); + when(mockMapper.mappers()).thenReturn(fieldMappers); + when(mapperService.documentMapper()).thenReturn(mockMapper); return mapperService; } diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java index ae2498ad84fe2..fefa77c759a9b 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java @@ -39,6 +39,8 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.Index; import org.elasticsearch.index.mapper.ContentPath; +import org.elasticsearch.index.mapper.DocumentFieldMappers; +import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.IdFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper; @@ -168,7 +170,11 @@ protected MapperService mapperServiceMock() { MetaJoinFieldMapper.MetaJoinFieldType metaJoinFieldType = mock(MetaJoinFieldMapper.MetaJoinFieldType.class); when(metaJoinFieldType.getJoinField()).thenReturn("join_field"); when(mapperService.fieldType("_parent_join")).thenReturn(metaJoinFieldType); - when(mapperService.fieldMapper("join_field")).thenReturn(joinFieldMapper); + DocumentFieldMappers fieldMappers = new DocumentFieldMappers(Collections.singleton(joinFieldMapper), + Collections.emptyList(), null, null, null); + DocumentMapper mockMapper = mock(DocumentMapper.class); + when(mockMapper.mappers()).thenReturn(fieldMappers); + when(mapperService.documentMapper()).thenReturn(mockMapper); return mapperService; } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 5a2a287f08748..9803ff64cd754 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -801,16 +801,8 @@ private static void parseDynamicValue(final ParseContext context, ObjectMapper p if (dynamic == ObjectMapper.Dynamic.FALSE) { return; } - final String path = context.path().pathAsText(currentFieldName); final Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(), context.path()); - final MappedFieldType existingFieldType = context.mapperService().fieldType(path); - final Mapper.Builder builder; - if (existingFieldType != null) { - // create a builder of the same type - builder = createBuilderFromFieldType(context, existingFieldType, currentFieldName); - } else { - builder = createBuilderFromDynamicValue(context, token, currentFieldName); - } + final Mapper.Builder builder = createBuilderFromDynamicValue(context, token, currentFieldName); Mapper mapper = builder.build(builderContext); context.addDynamicMapper(mapper); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 8599f3cac6597..bae01749a0c0e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -63,7 +63,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Set; import java.util.function.BooleanSupplier; import java.util.function.Function; @@ -419,26 +418,11 @@ private synchronized DocumentMapper internalMerge(DocumentMapper mapper, MergeRe this.hasNested = hasNested; this.fullPathObjectMappers = fullPathObjectMappers; - assert assertMappersShareSameFieldType(); assert assertSerialization(newMapper); return newMapper; } - private boolean assertMappersShareSameFieldType() { - if (mapper != null) { - List fieldMappers = new ArrayList<>(); - Collections.addAll(fieldMappers, mapper.mapping().metadataMappers); - MapperUtils.collect(mapper.root(), new ArrayList<>(), fieldMappers, new ArrayList<>()); - for (FieldMapper fieldMapper : fieldMappers) { - assert Objects.equals(fieldMapper.fieldType(), fieldTypes.get(fieldMapper.name())) : - "Fieldtype mismatch: " + fieldMapper.name() + " " + fieldMapper.fieldType() - + " != " + fieldTypes.get(fieldMapper.name()); - } - } - return true; - } - private boolean assertSerialization(DocumentMapper mapper) { // capture the source now, it may change due to concurrent parsing final CompressedXContent mappingSource = mapper.mappingSource(); @@ -584,10 +568,6 @@ public MappedFieldType fieldType(String fullName) { return fieldTypes.get(fullName); } - public Mapper fieldMapper(String fieldName) { - return documentMapper().mappers().getMapper(fieldName); - } - /** * Returns all the fields that match the given pattern. If the pattern is prefixed with a type * then the fields will be returned with a type prefix. diff --git a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java index 8ca7278b6e7b6..fdd115747df50 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java @@ -55,7 +55,6 @@ import org.elasticsearch.common.lucene.index.ElasticsearchDirectoryReader; import org.elasticsearch.common.network.NetworkAddress; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.text.Text; import org.elasticsearch.common.util.BigArrays; import org.elasticsearch.common.util.MockBigArrays; import org.elasticsearch.common.util.MockPageCacheRecycler; @@ -74,7 +73,6 @@ import org.elasticsearch.index.mapper.BinaryFieldMapper; import org.elasticsearch.index.mapper.CompletionFieldMapper; import org.elasticsearch.index.mapper.ContentPath; -import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.FieldAliasMapper; import org.elasticsearch.index.mapper.FieldMapper; import org.elasticsearch.index.mapper.GeoShapeFieldMapper; @@ -295,10 +293,6 @@ public boolean shouldCache(Query query) { MapperService mapperService = mapperServiceMock(); when(mapperService.getIndexSettings()).thenReturn(indexSettings); when(mapperService.hasNested()).thenReturn(false); - DocumentMapper mapper = mock(DocumentMapper.class); - when(mapper.typeText()).thenReturn(new Text(TYPE_NAME)); - when(mapper.type()).thenReturn(TYPE_NAME); - when(mapperService.documentMapper()).thenReturn(mapper); when(searchContext.mapperService()).thenReturn(mapperService); IndexFieldDataService ifds = new IndexFieldDataService(indexSettings, new IndicesFieldDataCache(Settings.EMPTY, new IndexFieldDataCache.Listener() { @@ -495,7 +489,7 @@ protected A searchAndReduc a.preCollection(); subSearcher.search(weight, a); a.postCollection(); - InternalAggregation agg = a.buildTopLevel(); + InternalAggregation agg = a.buildTopLevel(); aggs.add(agg); InternalAggregationTestCase.assertMultiBucketConsumer(agg, shardBucketConsumer); } From de25af92b3bdce9ca6b8164f363408883fc4bbe5 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 20 May 2020 10:52:58 +0100 Subject: [PATCH 4/7] Remove Mapper.updateFieldType() --- .../mapper/SearchAsYouTypeFieldMapper.java | 10 ----- .../join/mapper/ParentJoinFieldMapper.java | 13 ------ .../percolator/PercolatorFieldMapper.java | 24 ----------- .../index/mapper/DocumentMapper.java | 13 ------ .../index/mapper/DocumentParser.java | 4 -- .../index/mapper/FieldAliasMapper.java | 5 --- .../index/mapper/FieldMapper.java | 42 ------------------- .../elasticsearch/index/mapper/Mapper.java | 6 --- .../index/mapper/MapperService.java | 18 +++----- .../elasticsearch/index/mapper/Mapping.java | 22 ---------- .../index/mapper/ObjectMapper.java | 22 ---------- .../index/mapper/RootObjectMapper.java | 5 --- .../index/mapper/TextFieldMapper.java | 12 ------ .../index/mapper/ExternalMapper.java | 31 -------------- 14 files changed, 6 insertions(+), 221 deletions(-) diff --git a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java index 257e01cadf60f..d47793bd09929 100644 --- a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java @@ -647,16 +647,6 @@ public SearchAsYouTypeFieldMapper(String simpleName, this.maxShingleSize = maxShingleSize; } - @Override - public FieldMapper updateFieldType(Map fullNameToFieldType) { - SearchAsYouTypeFieldMapper fieldMapper = (SearchAsYouTypeFieldMapper) super.updateFieldType(fullNameToFieldType); - fieldMapper.prefixField = (PrefixFieldMapper) fieldMapper.prefixField.updateFieldType(fullNameToFieldType); - for (int i = 0; i < fieldMapper.shingleFields.length; i++) { - fieldMapper.shingleFields[i] = (ShingleFieldMapper) fieldMapper.shingleFields[i].updateFieldType(fullNameToFieldType); - } - return fieldMapper; - } - @Override protected void parseCreateField(ParseContext context) throws IOException { final String value = context.externalValueSet() ? context.externalValue().toString() : context.parser().textOrNull(); diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java index 02a607bb27a37..6bc1c8bf7f08e 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java @@ -356,19 +356,6 @@ protected void mergeOptions(FieldMapper other, List conflicts) { uniqueFieldMapper.setFieldMapper(this); } - @Override - public FieldMapper updateFieldType(Map fullNameToFieldType) { - ParentJoinFieldMapper fieldMapper = (ParentJoinFieldMapper) super.updateFieldType(fullNameToFieldType); - final List newMappers = new ArrayList<> (); - for (ParentIdFieldMapper mapper : fieldMapper.parentIdFields) { - newMappers.add((ParentIdFieldMapper) mapper.updateFieldType(fullNameToFieldType)); - } - fieldMapper.parentIdFields = Collections.unmodifiableList(newMappers); - this.uniqueFieldMapper = (MetaJoinFieldMapper) uniqueFieldMapper.updateFieldType(fullNameToFieldType); - uniqueFieldMapper.setFieldMapper(this); - return fieldMapper; - } - @Override protected void parseCreateField(ParseContext context) throws IOException { throw new UnsupportedOperationException("parsing is implemented in parse(), this method should NEVER be called"); diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java index 6b84480078f67..0db99ca3ae87a 100644 --- a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java +++ b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java @@ -359,30 +359,6 @@ Tuple, Map>> extractTermsAndRanges(IndexRead this.rangeFieldMapper = rangeFieldMapper; } - @Override - public FieldMapper updateFieldType(Map fullNameToFieldType) { - PercolatorFieldMapper updated = (PercolatorFieldMapper) super.updateFieldType(fullNameToFieldType); - KeywordFieldMapper queryTermsUpdated = (KeywordFieldMapper) queryTermsField.updateFieldType(fullNameToFieldType); - KeywordFieldMapper extractionResultUpdated = (KeywordFieldMapper) extractionResultField.updateFieldType(fullNameToFieldType); - BinaryFieldMapper queryBuilderUpdated = (BinaryFieldMapper) queryBuilderField.updateFieldType(fullNameToFieldType); - RangeFieldMapper rangeFieldMapperUpdated = (RangeFieldMapper) rangeFieldMapper.updateFieldType(fullNameToFieldType); - NumberFieldMapper msmFieldMapperUpdated = (NumberFieldMapper) minimumShouldMatchFieldMapper.updateFieldType(fullNameToFieldType); - - if (updated == this && queryTermsUpdated == queryTermsField && extractionResultUpdated == extractionResultField - && queryBuilderUpdated == queryBuilderField && rangeFieldMapperUpdated == rangeFieldMapper) { - return this; - } - if (updated == this) { - updated = (PercolatorFieldMapper) updated.clone(); - } - updated.queryTermsField = queryTermsUpdated; - updated.extractionResultField = extractionResultUpdated; - updated.queryBuilderField = queryBuilderUpdated; - updated.rangeFieldMapper = rangeFieldMapperUpdated; - updated.minimumShouldMatchFieldMapper = msmFieldMapperUpdated; - return updated; - } - @Override public void parse(ParseContext context) throws IOException { QueryShardContext queryShardContext = this.queryShardContext.get(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java index baa5fe5b85931..01d25da00685c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java @@ -307,19 +307,6 @@ public DocumentMapper merge(Mapping mapping) { return new DocumentMapper(mapperService, merged); } - /** - * Recursively update sub field types. - */ - 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); - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { return mapping.toXContent(builder, params); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index fc4b477472f4e..5a2a287f08748 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -812,10 +812,6 @@ private static void parseDynamicValue(final ParseContext context, ObjectMapper p 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)); - } context.addDynamicMapper(mapper); parseObjectOrField(context, mapper); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java index 5a1e28f9e2179..cc33f51982509 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldAliasMapper.java @@ -74,11 +74,6 @@ public Mapper merge(Mapper mergeWith) { return mergeWith; } - @Override - public Mapper updateFieldType(Map fullNameToFieldType) { - return this; - } - @Override public Iterator iterator() { return Collections.emptyIterator(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java index 00415a30956a2..77a17114518c3 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -413,27 +413,6 @@ private void mergeSharedOptions(FieldMapper mergeWith, List conflicts) { */ protected abstract void mergeOptions(FieldMapper other, List conflicts); - @Override - public FieldMapper updateFieldType(Map fullNameToFieldType) { - final MappedFieldType newFieldType = fullNameToFieldType.get(fieldType.name()); - if (newFieldType == null) { - // this field does not exist in the mappings yet - // this can happen if this mapper represents a mapping update - return this; - } else if (fieldType.getClass() != newFieldType.getClass()) { - throw new IllegalStateException("Mixing up field types: " + - fieldType.getClass() + " != " + newFieldType.getClass() + " on field " + fieldType.name()); - } - MultiFields updatedMultiFields = multiFields.updateFieldType(fullNameToFieldType); - if (fieldType == newFieldType && multiFields == updatedMultiFields) { - return this; // no change - } - FieldMapper updated = clone(); - updated.fieldType = newFieldType; - updated.multiFields = updatedMultiFields; - return updated; - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(simpleName()); @@ -637,27 +616,6 @@ public MultiFields merge(MultiFields mergeWith) { return new MultiFields(mappers); } - public MultiFields updateFieldType(Map fullNameToFieldType) { - ImmutableOpenMap.Builder newMappersBuilder = null; - - for (ObjectCursor cursor : mappers.values()) { - FieldMapper updated = cursor.value.updateFieldType(fullNameToFieldType); - if (updated != cursor.value) { - if (newMappersBuilder == null) { - newMappersBuilder = ImmutableOpenMap.builder(mappers); - } - newMappersBuilder.put(updated.simpleName(), updated); - } - } - - if (newMappersBuilder == null) { - return this; - } - - ImmutableOpenMap mappers = newMappersBuilder.build(); - return new MultiFields(mappers); - } - public Iterator iterator() { return StreamSupport.stream(mappers.values().spliterator(), false).map((p) -> (Mapper)p.value).iterator(); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java index 054a7725b8db1..7b432031238c6 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java @@ -172,10 +172,4 @@ public final String simpleName() { * Both {@code this} and {@code mergeWith} will be left unmodified. */ public abstract Mapper merge(Mapper mergeWith); - /** - * Update the field type of this mapper. This is necessary because some mapping updates - * can modify mappings across several types. This method must return a copy of the mapper - * so that the current mapper is not modified. - */ - public abstract Mapper updateFieldType(Map fullNameToFieldType); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index fe4f4311ca707..365f7d37dbfb8 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -63,6 +63,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.function.BooleanSupplier; import java.util.function.Function; @@ -402,13 +403,6 @@ private synchronized DocumentMapper internalMerge(DocumentMapper mapper, MergeRe } checkIndexSortCompatibility(indexSettings.getIndexSortConfig(), hasNested); - if (newMapper != null) { - DocumentMapper updatedDocumentMapper = newMapper.updateFieldType(fieldTypes.fullNameToFieldType); - if (updatedDocumentMapper != newMapper) { - newMapper = updatedDocumentMapper; - } - } - if (reason == MergeReason.MAPPING_UPDATE_PREFLIGHT) { return newMapper; } @@ -420,15 +414,13 @@ private synchronized DocumentMapper internalMerge(DocumentMapper mapper, MergeRe } // commit the change - if (newMapper != null) { - this.mapper = newMapper; - } + this.mapper = newMapper; this.fieldTypes = fieldTypes; this.hasNested = hasNested; this.fullPathObjectMappers = fullPathObjectMappers; assert assertMappersShareSameFieldType(); - assert newMapper == null || assertSerialization(newMapper); + assert assertSerialization(newMapper); return newMapper; } @@ -439,7 +431,9 @@ private boolean assertMappersShareSameFieldType() { Collections.addAll(fieldMappers, mapper.mapping().metadataMappers); MapperUtils.collect(mapper.root(), new ArrayList<>(), fieldMappers, new ArrayList<>()); for (FieldMapper fieldMapper : fieldMappers) { - assert fieldMapper.fieldType() == fieldTypes.get(fieldMapper.name()) : fieldMapper.name(); + assert Objects.equals(fieldMapper.fieldType(), fieldTypes.get(fieldMapper.name())) : + "Fieldtype mismatch: " + fieldMapper.name() + " " + fieldMapper.fieldType() + + " != " + fieldTypes.get(fieldMapper.name()); } } return true; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java index df98e356938e8..0cc730c33cdf6 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -104,28 +104,6 @@ public Mapping merge(Mapping mergeWith) { return new Mapping(indexCreated, mergedRoot, mergedMetadataMappers.values().toArray(new MetadataFieldMapper[0]), mergedMeta); } - /** - * Recursively update sub field types. - */ - public Mapping updateFieldType(Map 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); - if (updatedMeta == null && updatedRoot == root) { - return this; - } - return new Mapping(indexCreated, updatedRoot, updatedMeta == null ? metadataMappers : updatedMeta, meta); - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { root.toXContent(builder, params, new ToXContent() { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index c9d1b8fae75b2..412e356b111f2 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -498,28 +498,6 @@ private void checkObjectMapperParameters(final ObjectMapper mergeWith) { } } - @Override - public ObjectMapper updateFieldType(Map fullNameToFieldType) { - List updatedMappers = null; - for (Mapper mapper : this) { - Mapper updated = mapper.updateFieldType(fullNameToFieldType); - if (mapper != updated) { - if (updatedMappers == null) { - updatedMappers = new ArrayList<>(); - } - updatedMappers.add(updated); - } - } - if (updatedMappers == null) { - return this; - } - ObjectMapper updated = clone(); - for (Mapper updatedMapper : updatedMappers) { - updated.putMapper(updatedMapper); - } - return updated; - } - @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { toXContent(builder, params, null); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 0cb6c8dd286ff..29a4d79582bf7 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -308,11 +308,6 @@ protected void doMerge(ObjectMapper mergeWith) { } } - @Override - public RootObjectMapper updateFieldType(Map fullNameToFieldType) { - return (RootObjectMapper) super.updateFieldType(fullNameToFieldType); - } - @Override protected void doXContent(XContentBuilder builder, ToXContent.Params params) throws IOException { final boolean includeDefaults = params.paramAsBoolean("include_defaults", false); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java index e95de424d5e87..b8a6c63cd1492 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java @@ -848,18 +848,6 @@ protected String contentType() { return CONTENT_TYPE; } - @Override - public FieldMapper updateFieldType(Map fullNameToFieldType) { - TextFieldMapper mapper = (TextFieldMapper) super.updateFieldType(fullNameToFieldType); - if (mapper.prefixFieldMapper != null) { - mapper.prefixFieldMapper = (PrefixFieldMapper) mapper.prefixFieldMapper.updateFieldType(fullNameToFieldType); - } - if (mapper.phraseFieldMapper != null) { - mapper.phraseFieldMapper = (PhraseFieldMapper) mapper.phraseFieldMapper.updateFieldType(fullNameToFieldType); - } - return mapper; - } - @Override protected void mergeOptions(FieldMapper other, List conflicts) { TextFieldMapper mw = (TextFieldMapper) other; diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java b/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java index e6b8f33177d37..fb730f675fbcf 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java @@ -203,37 +203,6 @@ protected void parseCreateField(ParseContext context) throws IOException { throw new UnsupportedOperationException(); } - @Override - public FieldMapper updateFieldType(Map fullNameToFieldType) { - ExternalMapper update = (ExternalMapper) super.updateFieldType(fullNameToFieldType); - MultiFields multiFieldsUpdate = multiFields.updateFieldType(fullNameToFieldType); - BinaryFieldMapper binMapperUpdate = (BinaryFieldMapper) binMapper.updateFieldType(fullNameToFieldType); - BooleanFieldMapper boolMapperUpdate = (BooleanFieldMapper) boolMapper.updateFieldType(fullNameToFieldType); - GeoPointFieldMapper pointMapperUpdate = (GeoPointFieldMapper) pointMapper.updateFieldType(fullNameToFieldType); - AbstractShapeGeometryFieldMapper shapeMapperUpdate = - (AbstractShapeGeometryFieldMapper) shapeMapper.updateFieldType(fullNameToFieldType); - TextFieldMapper stringMapperUpdate = (TextFieldMapper) stringMapper.updateFieldType(fullNameToFieldType); - if (update == this - && multiFieldsUpdate == multiFields - && binMapperUpdate == binMapper - && boolMapperUpdate == boolMapper - && pointMapperUpdate == pointMapper - && shapeMapperUpdate == shapeMapper - && stringMapperUpdate == stringMapper) { - return this; - } - if (update == this) { - update = (ExternalMapper) clone(); - } - update.multiFields = multiFieldsUpdate; - update.binMapper = binMapperUpdate; - update.boolMapper = boolMapperUpdate; - update.pointMapper = pointMapperUpdate; - update.shapeMapper = shapeMapperUpdate; - update.stringMapper = stringMapperUpdate; - return update; - } - @Override public Iterator iterator() { return Iterators.concat(super.iterator(), Arrays.asList(binMapper, boolMapper, pointMapper, shapeMapper, stringMapper).iterator()); From 71abe8e5df8f8853c59ebf541856846d6e7991ef Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 20 May 2020 13:01:57 +0100 Subject: [PATCH 5/7] MetaJoinMapperField uses a fieldname lookup rather than holding a reference to the mapper --- .../join/mapper/MetaJoinFieldMapper.java | 25 +++++++++++-------- .../join/mapper/ParentJoinFieldMapper.java | 7 +++--- .../ChildrenToParentAggregatorTests.java | 3 ++- .../ParentToChildrenAggregatorTests.java | 3 ++- .../mapper/ParentJoinFieldMapperTests.java | 10 +++++--- .../index/mapper/MapperService.java | 4 +++ 6 files changed, 32 insertions(+), 20 deletions(-) diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/MetaJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/MetaJoinFieldMapper.java index e3e095edd0eb5..3ab36babfb33a 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/MetaJoinFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/MetaJoinFieldMapper.java @@ -59,21 +59,25 @@ static class Defaults { } static class Builder extends FieldMapper.Builder { - Builder() { + + final String joinField; + + Builder(String joinField) { super(NAME, Defaults.FIELD_TYPE, Defaults.FIELD_TYPE); builder = this; + this.joinField = joinField; } @Override public MetaJoinFieldMapper build(BuilderContext context) { fieldType.setName(NAME); - return new MetaJoinFieldMapper(name, fieldType, context.indexSettings()); + return new MetaJoinFieldMapper(name, joinField, (MetaJoinFieldType) fieldType, context.indexSettings()); } } public static class MetaJoinFieldType extends StringFieldType { - private ParentJoinFieldMapper mapper; + private String joinField; MetaJoinFieldType() {} @@ -110,8 +114,12 @@ public Object valueForDisplay(Object value) { return binaryValue.utf8ToString(); } - public ParentJoinFieldMapper getMapper() { - return mapper; + public void setJoinField(String joinField) { + this.joinField = joinField; + } + + public String getJoinField() { + return joinField; } @Override @@ -120,12 +128,10 @@ public Query existsQuery(QueryShardContext context) { } } - MetaJoinFieldMapper(String name, MappedFieldType fieldType, Settings indexSettings) { + MetaJoinFieldMapper(String name, String joinField, MetaJoinFieldType fieldType, Settings indexSettings) { super(name, fieldType, ParentIdFieldMapper.Defaults.FIELD_TYPE, indexSettings, MultiFields.empty(), CopyTo.empty()); - } + fieldType.setJoinField(joinField); - void setFieldMapper(ParentJoinFieldMapper mapper) { - fieldType().mapper = mapper; } @Override @@ -140,7 +146,6 @@ protected MetaJoinFieldMapper clone() { @Override protected void mergeOptions(FieldMapper other, List conflicts) { - } @Override diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java index 6bc1c8bf7f08e..5fbf7f4544282 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java @@ -85,7 +85,8 @@ public static class Defaults { public static ParentJoinFieldMapper getMapper(MapperService service) { MetaJoinFieldMapper.MetaJoinFieldType fieldType = (MetaJoinFieldMapper.MetaJoinFieldType) service.fieldType(MetaJoinFieldMapper.NAME); - return fieldType == null ? null : fieldType.getMapper(); + return fieldType == null ? null : + (ParentJoinFieldMapper) service.fieldMapper(fieldType.getJoinField()); } private static String getParentIdFieldName(String joinFieldName, String parentName) { @@ -160,7 +161,7 @@ public ParentJoinFieldMapper build(BuilderContext context) { }) .forEach(parentIdFields::add); checkParentFields(name(), parentIdFields); - MetaJoinFieldMapper unique = new MetaJoinFieldMapper.Builder().build(context); + MetaJoinFieldMapper unique = new MetaJoinFieldMapper.Builder(name).build(context); return new ParentJoinFieldMapper(name, fieldType, context.indexSettings(), unique, Collections.unmodifiableList(parentIdFields), eagerGlobalOrdinals); } @@ -262,7 +263,6 @@ protected ParentJoinFieldMapper(String simpleName, super(simpleName, fieldType, Defaults.FIELD_TYPE, indexSettings, MultiFields.empty(), CopyTo.empty()); this.parentIdFields = parentIdFields; this.uniqueFieldMapper = uniqueFieldMapper; - this.uniqueFieldMapper.setFieldMapper(this); this.eagerGlobalOrdinals = eagerGlobalOrdinals; } @@ -353,7 +353,6 @@ protected void mergeOptions(FieldMapper other, List conflicts) { this.eagerGlobalOrdinals = joinMergeWith.eagerGlobalOrdinals; this.parentIdFields = Collections.unmodifiableList(newParentIdFields); this.uniqueFieldMapper = (MetaJoinFieldMapper) uniqueFieldMapper.merge(joinMergeWith.uniqueFieldMapper); - uniqueFieldMapper.setFieldMapper(this); } @Override diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java index 7a7bc4e92ed9f..2750a53dec88a 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java @@ -278,8 +278,9 @@ protected MapperService mapperServiceMock() { ParentJoinFieldMapper joinFieldMapper = createJoinFieldMapper(); MapperService mapperService = mock(MapperService.class); MetaJoinFieldMapper.MetaJoinFieldType metaJoinFieldType = mock(MetaJoinFieldMapper.MetaJoinFieldType.class); - when(metaJoinFieldType.getMapper()).thenReturn(joinFieldMapper); + when(metaJoinFieldType.getJoinField()).thenReturn("join_field"); when(mapperService.fieldType("_parent_join")).thenReturn(metaJoinFieldType); + when(mapperService.fieldMapper("join_field")).thenReturn(joinFieldMapper); return mapperService; } diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java index cc302955234e5..ae2498ad84fe2 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java @@ -166,8 +166,9 @@ protected MapperService mapperServiceMock() { ParentJoinFieldMapper joinFieldMapper = createJoinFieldMapper(); MapperService mapperService = mock(MapperService.class); MetaJoinFieldMapper.MetaJoinFieldType metaJoinFieldType = mock(MetaJoinFieldMapper.MetaJoinFieldType.class); - when(metaJoinFieldType.getMapper()).thenReturn(joinFieldMapper); + when(metaJoinFieldType.getJoinField()).thenReturn("join_field"); when(mapperService.fieldType("_parent_join")).thenReturn(metaJoinFieldType); + when(mapperService.fieldMapper("join_field")).thenReturn(joinFieldMapper); return mapperService; } diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java index 0079da5d723b3..48a5e33f86d16 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java @@ -301,10 +301,11 @@ public void testUpdateRelations() throws Exception { .endObject() .endObject() .endObject().endObject()); - docMapper = indexService.mapperService().merge("type", new CompressedXContent(updateMapping), + docMapper = indexService.mapperService().merge("_doc", new CompressedXContent(updateMapping), MapperService.MergeReason.MAPPING_UPDATE); - assertTrue(docMapper.mappers().getMapper("join_field") == ParentJoinFieldMapper.getMapper(indexService.mapperService())); ParentJoinFieldMapper mapper = ParentJoinFieldMapper.getMapper(indexService.mapperService()); + assertNotNull(mapper); + assertEquals("join_field", mapper.name()); assertTrue(mapper.hasChild("child2")); assertFalse(mapper.hasParent("child2")); assertTrue(mapper.hasChild("grand_child2")); @@ -322,10 +323,11 @@ public void testUpdateRelations() throws Exception { .endObject() .endObject() .endObject().endObject()); - docMapper = indexService.mapperService().merge("type", new CompressedXContent(updateMapping), + docMapper = indexService.mapperService().merge("_doc", new CompressedXContent(updateMapping), MapperService.MergeReason.MAPPING_UPDATE); - assertTrue(docMapper.mappers().getMapper("join_field") == ParentJoinFieldMapper.getMapper(indexService.mapperService())); ParentJoinFieldMapper mapper = ParentJoinFieldMapper.getMapper(indexService.mapperService()); + assertNotNull(mapper); + assertEquals("join_field", mapper.name()); assertTrue(mapper.hasParent("other")); assertFalse(mapper.hasChild("other")); assertTrue(mapper.hasChild("child_other1")); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 365f7d37dbfb8..8599f3cac6597 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -584,6 +584,10 @@ public MappedFieldType fieldType(String fullName) { return fieldTypes.get(fullName); } + public Mapper fieldMapper(String fieldName) { + return documentMapper().mappers().getMapper(fieldName); + } + /** * Returns all the fields that match the given pattern. If the pattern is prefixed with a type * then the fields will be returned with a type prefix. From b710a50163e3f887ca4dc6097ea3a3c57f29461f Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Thu, 21 May 2020 13:52:22 +0100 Subject: [PATCH 6/7] feedback --- .../join/mapper/ParentJoinFieldMapper.java | 11 ++++++++-- .../ChildrenToParentAggregatorTests.java | 8 +++++++- .../ParentToChildrenAggregatorTests.java | 8 +++++++- .../index/mapper/DocumentParser.java | 10 +--------- .../index/mapper/MapperService.java | 20 ------------------- .../aggregations/AggregatorTestCase.java | 8 +------- 6 files changed, 25 insertions(+), 40 deletions(-) diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java index 5fbf7f4544282..c7ebabcdc803b 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java @@ -34,6 +34,8 @@ import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.plain.SortedSetOrdinalsIndexFieldData; import org.elasticsearch.index.mapper.ContentPath; +import org.elasticsearch.index.mapper.DocumentFieldMappers; +import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.FieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper; @@ -85,8 +87,13 @@ public static class Defaults { public static ParentJoinFieldMapper getMapper(MapperService service) { MetaJoinFieldMapper.MetaJoinFieldType fieldType = (MetaJoinFieldMapper.MetaJoinFieldType) service.fieldType(MetaJoinFieldMapper.NAME); - return fieldType == null ? null : - (ParentJoinFieldMapper) service.fieldMapper(fieldType.getJoinField()); + if (fieldType == null) { + return null; + } + DocumentMapper mapper = service.documentMapper(); + String joinField = fieldType.getJoinField(); + DocumentFieldMappers fieldMappers = mapper.mappers(); + return (ParentJoinFieldMapper) fieldMappers.getMapper(joinField); } private static String getParentIdFieldName(String joinFieldName, String parentName) { diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java index 2750a53dec88a..dfb1d447dd63d 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java @@ -39,6 +39,8 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.Index; import org.elasticsearch.index.mapper.ContentPath; +import org.elasticsearch.index.mapper.DocumentFieldMappers; +import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.IdFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper; @@ -280,7 +282,11 @@ protected MapperService mapperServiceMock() { MetaJoinFieldMapper.MetaJoinFieldType metaJoinFieldType = mock(MetaJoinFieldMapper.MetaJoinFieldType.class); when(metaJoinFieldType.getJoinField()).thenReturn("join_field"); when(mapperService.fieldType("_parent_join")).thenReturn(metaJoinFieldType); - when(mapperService.fieldMapper("join_field")).thenReturn(joinFieldMapper); + DocumentFieldMappers fieldMappers = new DocumentFieldMappers(Collections.singleton(joinFieldMapper), + Collections.emptyList(), null, null, null); + DocumentMapper mockMapper = mock(DocumentMapper.class); + when(mockMapper.mappers()).thenReturn(fieldMappers); + when(mapperService.documentMapper()).thenReturn(mockMapper); return mapperService; } diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java index ae2498ad84fe2..fefa77c759a9b 100644 --- a/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java +++ b/modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java @@ -39,6 +39,8 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.Index; import org.elasticsearch.index.mapper.ContentPath; +import org.elasticsearch.index.mapper.DocumentFieldMappers; +import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.IdFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper; @@ -168,7 +170,11 @@ protected MapperService mapperServiceMock() { MetaJoinFieldMapper.MetaJoinFieldType metaJoinFieldType = mock(MetaJoinFieldMapper.MetaJoinFieldType.class); when(metaJoinFieldType.getJoinField()).thenReturn("join_field"); when(mapperService.fieldType("_parent_join")).thenReturn(metaJoinFieldType); - when(mapperService.fieldMapper("join_field")).thenReturn(joinFieldMapper); + DocumentFieldMappers fieldMappers = new DocumentFieldMappers(Collections.singleton(joinFieldMapper), + Collections.emptyList(), null, null, null); + DocumentMapper mockMapper = mock(DocumentMapper.class); + when(mockMapper.mappers()).thenReturn(fieldMappers); + when(mapperService.documentMapper()).thenReturn(mockMapper); return mapperService; } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 5a2a287f08748..9803ff64cd754 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -801,16 +801,8 @@ private static void parseDynamicValue(final ParseContext context, ObjectMapper p if (dynamic == ObjectMapper.Dynamic.FALSE) { return; } - final String path = context.path().pathAsText(currentFieldName); final Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(), context.path()); - final MappedFieldType existingFieldType = context.mapperService().fieldType(path); - final Mapper.Builder builder; - if (existingFieldType != null) { - // create a builder of the same type - builder = createBuilderFromFieldType(context, existingFieldType, currentFieldName); - } else { - builder = createBuilderFromDynamicValue(context, token, currentFieldName); - } + final Mapper.Builder builder = createBuilderFromDynamicValue(context, token, currentFieldName); Mapper mapper = builder.build(builderContext); context.addDynamicMapper(mapper); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 8599f3cac6597..bae01749a0c0e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -63,7 +63,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Set; import java.util.function.BooleanSupplier; import java.util.function.Function; @@ -419,26 +418,11 @@ private synchronized DocumentMapper internalMerge(DocumentMapper mapper, MergeRe this.hasNested = hasNested; this.fullPathObjectMappers = fullPathObjectMappers; - assert assertMappersShareSameFieldType(); assert assertSerialization(newMapper); return newMapper; } - private boolean assertMappersShareSameFieldType() { - if (mapper != null) { - List fieldMappers = new ArrayList<>(); - Collections.addAll(fieldMappers, mapper.mapping().metadataMappers); - MapperUtils.collect(mapper.root(), new ArrayList<>(), fieldMappers, new ArrayList<>()); - for (FieldMapper fieldMapper : fieldMappers) { - assert Objects.equals(fieldMapper.fieldType(), fieldTypes.get(fieldMapper.name())) : - "Fieldtype mismatch: " + fieldMapper.name() + " " + fieldMapper.fieldType() - + " != " + fieldTypes.get(fieldMapper.name()); - } - } - return true; - } - private boolean assertSerialization(DocumentMapper mapper) { // capture the source now, it may change due to concurrent parsing final CompressedXContent mappingSource = mapper.mappingSource(); @@ -584,10 +568,6 @@ public MappedFieldType fieldType(String fullName) { return fieldTypes.get(fullName); } - public Mapper fieldMapper(String fieldName) { - return documentMapper().mappers().getMapper(fieldName); - } - /** * Returns all the fields that match the given pattern. If the pattern is prefixed with a type * then the fields will be returned with a type prefix. diff --git a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java index 8ca7278b6e7b6..fdd115747df50 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java @@ -55,7 +55,6 @@ import org.elasticsearch.common.lucene.index.ElasticsearchDirectoryReader; import org.elasticsearch.common.network.NetworkAddress; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.text.Text; import org.elasticsearch.common.util.BigArrays; import org.elasticsearch.common.util.MockBigArrays; import org.elasticsearch.common.util.MockPageCacheRecycler; @@ -74,7 +73,6 @@ import org.elasticsearch.index.mapper.BinaryFieldMapper; import org.elasticsearch.index.mapper.CompletionFieldMapper; import org.elasticsearch.index.mapper.ContentPath; -import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.FieldAliasMapper; import org.elasticsearch.index.mapper.FieldMapper; import org.elasticsearch.index.mapper.GeoShapeFieldMapper; @@ -295,10 +293,6 @@ public boolean shouldCache(Query query) { MapperService mapperService = mapperServiceMock(); when(mapperService.getIndexSettings()).thenReturn(indexSettings); when(mapperService.hasNested()).thenReturn(false); - DocumentMapper mapper = mock(DocumentMapper.class); - when(mapper.typeText()).thenReturn(new Text(TYPE_NAME)); - when(mapper.type()).thenReturn(TYPE_NAME); - when(mapperService.documentMapper()).thenReturn(mapper); when(searchContext.mapperService()).thenReturn(mapperService); IndexFieldDataService ifds = new IndexFieldDataService(indexSettings, new IndicesFieldDataCache(Settings.EMPTY, new IndexFieldDataCache.Listener() { @@ -495,7 +489,7 @@ protected A searchAndReduc a.preCollection(); subSearcher.search(weight, a); a.postCollection(); - InternalAggregation agg = a.buildTopLevel(); + InternalAggregation agg = a.buildTopLevel(); aggs.add(agg); InternalAggregationTestCase.assertMultiBucketConsumer(agg, shardBucketConsumer); } From fdae94408f77a79a31f730e798fb1a68e641052f Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Thu, 21 May 2020 18:22:56 +0100 Subject: [PATCH 7/7] remove unused method --- .../index/mapper/DocumentParser.java | 50 ------------------- 1 file changed, 50 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 9803ff64cd754..a1cda9f3e4990 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -32,14 +32,11 @@ import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.mapper.DynamicTemplate.XContentFieldType; -import org.elasticsearch.index.mapper.KeywordFieldMapper.KeywordFieldType; -import org.elasticsearch.index.mapper.TextFieldMapper.TextFieldType; import java.io.IOException; import java.time.format.DateTimeParseException; import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.Iterator; import java.util.List; @@ -619,53 +616,6 @@ private static void parseNullValue(ParseContext context, ObjectMapper parentMapp } } - private static Mapper.Builder createBuilderFromFieldType(final ParseContext context, - MappedFieldType fieldType, String currentFieldName) { - Mapper.Builder builder = null; - if (fieldType instanceof TextFieldType) { - builder = context.root().findTemplateBuilder(context, currentFieldName, "text", XContentFieldType.STRING); - if (builder == null) { - builder = new TextFieldMapper.Builder(currentFieldName) - .addMultiField(new KeywordFieldMapper.Builder("keyword").ignoreAbove(256)); - } - } else if (fieldType instanceof KeywordFieldType) { - builder = context.root().findTemplateBuilder(context, currentFieldName, "keyword", XContentFieldType.STRING); - } else { - switch (fieldType.typeName()) { - case DateFieldMapper.CONTENT_TYPE: - builder = context.root().findTemplateBuilder(context, currentFieldName, XContentFieldType.DATE); - break; - case "long": - builder = context.root().findTemplateBuilder(context, currentFieldName, "long", XContentFieldType.LONG); - break; - case "double": - builder = context.root().findTemplateBuilder(context, currentFieldName, "double", XContentFieldType.DOUBLE); - break; - case "integer": - builder = context.root().findTemplateBuilder(context, currentFieldName, "integer", XContentFieldType.LONG); - break; - case "float": - builder = context.root().findTemplateBuilder(context, currentFieldName, "float", XContentFieldType.DOUBLE); - break; - case BooleanFieldMapper.CONTENT_TYPE: - builder = context.root().findTemplateBuilder(context, currentFieldName, "boolean", XContentFieldType.BOOLEAN); - break; - default: - break; - } - } - if (builder == null) { - Mapper.TypeParser.ParserContext parserContext = context.docMapperParser().parserContext(); - Mapper.TypeParser typeParser = parserContext.typeParser(fieldType.typeName()); - if (typeParser == null) { - throw new MapperParsingException("Cannot generate dynamic mappings of type [" + fieldType.typeName() - + "] for [" + currentFieldName + "]"); - } - builder = typeParser.parse(currentFieldName, new HashMap<>(), parserContext); - } - return builder; - } - private static Mapper.Builder newLongBuilder(String name) { return new NumberFieldMapper.Builder(name, NumberFieldMapper.NumberType.LONG); }