Remove Mapper.updateFieldType() #57151
Conversation
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
romseygeek
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
|
This is a backport commit for #56986 - there were some test failures on the plain backport, so I've opened a PR to let CI make sure everything works. |
| @@ -436,106 +436,6 @@ public void testComplexArray() throws Exception { | |||
| .endObject().endObject().endObject()), serialize(update)); | |||
| } | |||
|
|
|||
| public void testReuseExistingMappings() throws IOException, Exception { | |||
There was a problem hiding this comment.
This was testing dead code - we would never update a mapping against a separate type, because the update mappings code already ensures that all updates use the same type name.
There was a problem hiding this comment.
It would be good to match what we have on master (it's easier to reason about, and makes backport conflicts less likely). On master we've modified the test to focus on a single type -- we could apply the same change here.
romseygeek
changed the title
| @@ -559,29 +547,16 @@ static void validateTypeName(String type) { | |||
| } | |||
| if (newMapper != null) { | |||
| this.mapper = newMapper; | |||
| this.fieldTypes = fieldTypes; | |||
There was a problem hiding this comment.
Same question here about whether this change is important.
| } | ||
|
|
||
| // make structures immutable | ||
| results = Collections.unmodifiableMap(results); |
There was a problem hiding this comment.
I'm wondering if this change is important, where we no longer wrap in an unmodifiable map. If not we could restore it to make the PR more targeted/ precise.
| @@ -436,106 +436,6 @@ public void testComplexArray() throws Exception { | |||
| .endObject().endObject().endObject()), serialize(update)); | |||
| } | |||
|
|
|||
| public void testReuseExistingMappings() throws IOException, Exception { | |||
There was a problem hiding this comment.
It would be good to match what we have on master (it's easier to reason about, and makes backport conflicts less likely). On master we've modified the test to focus on a single type -- we could apply the same change here.
|
Thanks @jtibshirani! |
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