Skip to content

IndexTemplateMetaData should only store a single mapping #50753

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

romseygeek
Copy link
Contributor

Index templates can only store a single mapping, but getMappings() returns a map
of mapping type to mappings. This commit changes the signature of getMappings() to
just return the mappings.

In addition, index templates stored in the cluster state will now always convert their type
to _doc, and templates added via PutIndexTemplateRequest will also convert their
type.

@romseygeek romseygeek added :Search Foundations/Mapping Index mappings, including merging and defining field types :Data Management/Indices APIs APIs to create and manage indices and templates >refactoring v8.0.0 labels Jan 8, 2020
@romseygeek romseygeek self-assigned this Jan 8, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Mapping)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Indices APIs)

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @romseygeek, for now I would like to clarify some things

private final ImmutableOpenMap<String, CompressedXContent> mappings;

private final ImmutableOpenMap<String, AliasMetaData> aliases;

public IndexTemplateMetaData(String name, int order, Integer version,
List<String> patterns, Settings settings,
ImmutableOpenMap<String, CompressedXContent> mappings,
CompressedXContent mappings,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not very clear if we expect CompressedXContent mappings to contain the type or not? From line 98, it looks like it should not contain type, but then how IndexTemplateMetaData is built fromXContent and builder it looks like mappings already contain a type?
Or we can expect both cases?

@@ -281,13 +302,13 @@ public Builder settings(Settings settings) {
return this;
}

public Builder putMapping(String mappingType, CompressedXContent mappingSource) {
mappings.put(mappingType, mappingSource);
public Builder setMapping(CompressedXContent mappingSource) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here it is not very clear if we expect mappingSource to contain a type or not. I saw examples of both

Map<String, Object> mappingSource =
MapBuilder.<String, Object>newMapBuilder().put(mappingType, parser.mapOrdered()).map();
builder.putMapping(mappingType, Strings.toString(XContentFactory.jsonBuilder().map(mappingSource)));
builder.setMapping(Strings.toString(XContentFactory.jsonBuilder().map(mappingSource)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here looks like setMapping expects mapping with a type

@@ -94,7 +95,8 @@ public IndexTemplateMetaData(String name, int order, Integer version,
this.version = version;
this.patterns = patterns;
this.settings = settings;
this.mappings = mappings;
this.mappings = ImmutableOpenMap.<String, CompressedXContent>builder()
.fPut(MapperService.SINGLE_MAPPING_NAME, mappings).build();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not very clear, why we can't have mappings just as CompressedXContent and have this.mappings = mappings?

@romseygeek
Copy link
Contributor Author

I've opened #52961 as a replacement here, which just changes the method signature of getMappings() and leaves the internal representations in place, which is a lot simpler.

@romseygeek romseygeek closed this Mar 2, 2020
@romseygeek romseygeek deleted the types-removal/index-templates branch March 2, 2020 10:03
@romseygeek romseygeek removed the v8.0.0 label Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Indices APIs APIs to create and manage indices and templates >refactoring :Search Foundations/Mapping Index mappings, including merging and defining field types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants