Remove Mapper.updateFieldType() #56986
Conversation
romseygeek
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
…erence to the mapper
| @@ -85,7 +85,8 @@ | |||
| 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()); | |||
There was a problem hiding this comment.
ParentJoinFieldMapper was using updateFieldType() to update its linked MetaJoinFieldMapper. This requires a small bit of hackery to get round, which I hope to fix in a followup by moving the various filter factory methods from the field mapper to the field type.
| @@ -590,6 +584,10 @@ public MappedFieldType fieldType(String fullName) { | |||
| return fieldTypes.get(fullName); | |||
| } | |||
|
|
|||
| public Mapper fieldMapper(String fieldName) { | |||
There was a problem hiding this comment.
This is unfortunately necessary as a shim to get round the ParentJoinFieldMapper issue above. Hopefully removed asap.
There was a problem hiding this comment.
Could we avoid adding this new method and call this inline in ParentJoinFieldMapper? All the methods like documentMapper() are public, so it seems do-able.
There was a problem hiding this comment.
++ have reworked the mocking (which sent me down a surprisingly deep rabbit hole, but I think it's fixed now!)
| @@ -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())) : | |||
There was a problem hiding this comment.
I think we could remove this whole method assertMappersShareSameFieldType, it doesn't seem helpful now that we only have one type.
| @@ -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) { | |||
There was a problem hiding this comment.
If I'm understanding correctly, we can now assume existingFieldType will always be null, since there is only one type. In that case we could remove the logic above too, including the method createBuilderFromFieldType.
| @@ -590,6 +584,10 @@ public MappedFieldType fieldType(String fullName) { | |||
| return fieldTypes.get(fullName); | |||
| } | |||
|
|
|||
| public Mapper fieldMapper(String fieldName) { | |||
There was a problem hiding this comment.
Could we avoid adding this new method and call this inline in ParentJoinFieldMapper? All the methods like documentMapper() are public, so it seems do-able.
| DocumentMapper mapper = mock(DocumentMapper.class); | ||
| when(mapper.typeText()).thenReturn(new Text(TYPE_NAME)); | ||
| when(mapper.type()).thenReturn(TYPE_NAME); | ||
| when(mapperService.documentMapper()).thenReturn(mapper); |
There was a problem hiding this comment.
I wasted several hours trying to work out why my mocked DocumentMapper class wasn't returning the DocumentFieldMapper instance I was asking it to. This appears to be unnecessary, so I've removed it
| } else { | ||
| builder = createBuilderFromDynamicValue(context, token, currentFieldName); | ||
| } | ||
| final Mapper.Builder<?> builder = createBuilderFromDynamicValue(context, token, currentFieldName); |
There was a problem hiding this comment.
I think we can delete createBuilderFromFieldType now, since it's unused.
…erence to the mapper
…to mapper/update-field-type
|
The backport on this is a bit complicated due to default types still being present (but unused!) in 7x, so I will open a separate PR. |
When we had multiple mapping types, an update to a field in one type had to be propagated to the same field in all other types. This was done using the Mapper.updateFieldType() method, called at the end of a merge. However, now that we only have a single type per index, this method is unnecessary and can be removed. Relates to elastic#41059
When we had multiple mapping types, an update to a field in one type had to be propagated to the same field in all other types. This was done using the Mapper.updateFieldType() method, called at the end of a merge. However, now that we only have a single type per index, this method is unnecessary and can be removed. Relates to #41059 Backport of #56986
When we had multiple mapping types, an update to a field in one type had to be
propagated to the same field in all other types. This was done using the
Mapper.updateFieldType()method, called at the end of a merge. However, nowthat we only have a single type per index, this method is unnecessary and can
be removed.
Relates to #41059