Split Mapping parsing from DocumentMapper construction #68593
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DocumentMapperParser parses xcontent mappings into a DocumentMapper, through DocumentMapper.Builder. Most of the work is done to construct a Mapping instance, that then gets provided to the DocumentMapper. Moving forward, we would like to rely more on Mapping and less on the entire DocumentMapper, but currently it is cumbersome to create Mapping without creating an instance of DocumentMapper. This commit removes DocumentMapperParser and DocumentMapper.Builder in favor of a new class called MappingParser that given xcontent mappings returns a Mapping instance, which can be used on its own or provided to DocumentMapper at its construction time. This will help with further refactorings as well as to possibly have more tests that don't rely on the entire MapperService/DocumentMapper but rather only on the needed components.
javanna
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
javanna
commented
Feb 5, 2021
| import org.elasticsearch.common.xcontent.XContentFactory; | ||
| import org.elasticsearch.common.xcontent.XContentHelper; | ||
| import org.elasticsearch.index.mapper.MapperService.MergeReason; | ||
| import static java.util.Collections.unmodifiableMap; |
There was a problem hiding this comment.
these should go back to normal , now correcting a previous wrong change made by myself :)
romseygeek
approved these changes
Feb 8, 2021
| private final Supplier<Mapper.TypeParser.ParserContext> parserContextSupplier; | ||
| private final RootObjectMapper.TypeParser rootObjectTypeParser = new RootObjectMapper.TypeParser(); | ||
| private final Supplier<Map<Class<? extends MetadataFieldMapper>, MetadataFieldMapper>> metadataMappersSupplier; | ||
| private final Map<String, MetadataFieldMapper.TypeParser> rootTypeParsers; |
There was a problem hiding this comment.
I know they were already called this, but can we rename this map to metadataTypeParsers? I always get confused when reading this about why we're using these outside the root.
| } | ||
|
|
||
| private static String getRemainingFields(Map<?, ?> map) { | ||
| StringBuilder remainingFields = new StringBuilder(); |
There was a problem hiding this comment.
Does this do anything significantly different to Objects.toString(map)?
There was a problem hiding this comment.
only different parentheses and separator. I am removing this method, hopefully not too many tests will fail as a result.
There was a problem hiding this comment.
I backed off this change because it ends up changing ever so slightly an error message which causes having to update tests, and all this is really not the goal for this PR. We can also make such change separately.
javanna
force-pushed
the
refactoring/split_mapping_parsing
branch
from
9547b1a to
7799500
Compare
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Feb 8, 2021
DocumentMapperParser parses xcontent mappings into a DocumentMapper, through DocumentMapper.Builder. Most of the work is done to construct a Mapping instance, that then gets provided to the DocumentMapper. Moving forward, we would like to rely more on Mapping and less on the entire DocumentMapper, but currently it is cumbersome to create Mapping without creating an instance of DocumentMapper. This commit removes DocumentMapperParser and DocumentMapper.Builder in favor of a new class called MappingParser that given xcontent mappings returns a Mapping instance, which can be used on its own or provided to DocumentMapper at its construction time. This will help with further refactorings as well as to possibly have more tests that don't rely on the entire MapperService/DocumentMapper but rather only on the needed components.
javanna
added a commit
that referenced
this pull request
Feb 8, 2021
DocumentMapperParser parses xcontent mappings into a DocumentMapper, through DocumentMapper.Builder. Most of the work is done to construct a Mapping instance, that then gets provided to the DocumentMapper. Moving forward, we would like to rely more on Mapping and less on the entire DocumentMapper, but currently it is cumbersome to create Mapping without creating an instance of DocumentMapper. This commit removes DocumentMapperParser and DocumentMapper.Builder in favor of a new class called MappingParser that given xcontent mappings returns a Mapping instance, which can be used on its own or provided to DocumentMapper at its construction time. This will help with further refactorings as well as to possibly have more tests that don't rely on the entire MapperService/DocumentMapper but rather only on the needed components.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DocumentMapperParser parses xcontent mappings into a DocumentMapper, through DocumentMapper.Builder. Most of the work is done to construct a Mapping instance, that then gets provided to the DocumentMapper.
Moving forward, we would like to rely more on Mapping and less on the entire DocumentMapper, but currently it is cumbersome to create Mapping without creating an instance of DocumentMapper.
This commit removes DocumentMapperParser and DocumentMapper.Builder in favor of a new class called MappingParser that given xcontent mappings returns a Mapping instance, which can be used on its own or provided to DocumentMapper at its construction time. This will help with further refactorings as well as to possibly have more tests that don't rely on the entire MapperService/DocumentMapper but rather only on the needed components.