Remove type parameter from MapperService.documentMapper() #48593
Conversation
romseygeek
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
jtibshirani
left a comment
jtibshirani
left a comment
There was a problem hiding this comment.
This looks good to me, just left a small comment + question.
| .endObject().endObject().endObject()), serialize(update)); | ||
| } | ||
|
|
||
| public void testReuseExistingMappings() throws IOException, Exception { |
There was a problem hiding this comment.
This seems like a pretty useful test, and could make sense even when there is only one type. Instead of deleting it, we could validate that the update variable below is null?
There was a problem hiding this comment.
++ I re-added the test and updated it accordingly.
| }; | ||
|
|
||
| private XContentBuilder createMapping() throws Exception { | ||
| XContentBuilder xcb = XContentFactory.jsonBuilder().startObject().startObject("type1") |
There was a problem hiding this comment.
To check I'm not missing something, these changes are just for clean-up (and not required to get tests to pass)?
There was a problem hiding this comment.
I think this has bled in from a different branch (the PR for #47792) - will clean up
|
@elasticmachine run elasticsearch-ci/bwc |
|
@elasticmachine update branch |
| private DocumentMapperForType docMapper() { | ||
| return mapperService.documentMapperWithAutoCreate( | ||
| mapperService.resolveDocumentType(MapperService.SINGLE_MAPPING_NAME)); | ||
| return mapperService.documentMapperWithAutoCreate(); |
There was a problem hiding this comment.
To check my understanding, this is fine to change because the type is no longer relevant to indexing (for example we already removed type information from the translog)?
There was a problem hiding this comment.
Correct - we can't remove MapperService#resolveDocumentType yet because it's still used by DocumentMapperParser, but that's on the list...
…r-service-document-mapper
…-document-mapper' into types-removal/mapper-service-document-mapper
4 participants
We only have a single mapper per MapperService now anyway, so the extra
type parameter is surplus to requirements.
Relates to #41059