Skip to content
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

Stop automatically nesting mappings in index creation requests. #36924

Merged
merged 9 commits into from
Jan 4, 2019

Conversation

jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Dec 21, 2018

The call CreateIndexRequest#mapping(String type, Map<String, ?> source) currently checks if root of the map source is the type name, and if not, makes sure it is nested under the type. It looks like we have this logic because at one point, we tried to maintain the invariant that all mapping values in the request were nested under the type name.

However, since 2341825, this is no longer necessary, and in fact almost none of the other CreateIndexRequest#mapping methods perform this automatic detection + nesting. This PR is a step towards standardizing CreateIndexRequest so that mapping values are not expected to be nested under the type name.

Automatically nesting the mapping values has a few downsides:

  • It makes it difficult to introduce fully typeless mapping methods onto this class, as we move towards our goal of removing types. This is because when a typeless mapping definition is provided, it may be automatically converted to one that contains the type. In particular, this logic is making it very hard to deprecate types in the HLRC for 'create index' and 'put index template' requests.

  • It is redundant, since the type name is already stored as the key in the mappings map.

  • When using the above mapping method then serializing the request to xContent, the mappings will be doubly nested under the type name, as in this example:

    {
      "mappings": {
        "my_type": {
          "my_type": {
            "properties": { ... }
          }
        }
      }
    }
    

    Because of our leniency around mappings, these doubly-nested mappings are still able to be re-parsed. However, this situation is buggy and potentially fragile.

Note that PutIndexTemplateRequest has the same issue, and would have to be changed in an analogous way.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@jtibshirani jtibshirani changed the title In CreateIndexRequest, avoid doubly nested mappings. Avoid doubly nested mappings in index creation requests. Dec 21, 2018
@jtibshirani jtibshirani force-pushed the create-index-mappings branch 2 times, most recently from 1f536ff to 1161e5f Compare December 22, 2018 03:21
@jtibshirani jtibshirani added :Search Foundations/Mapping Index mappings, including merging and defining field types and removed :Core/Features/Java High Level REST Client labels Dec 22, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@jtibshirani jtibshirani force-pushed the create-index-mappings branch from 1161e5f to 7ff9d51 Compare January 1, 2019 23:25
Now that we unwrap mappings in DocumentMapperParser#extractMappings, it is not
necessary for the mapping definition to always be nested under the type. This
leniency around the mapping format was added in 2341825.
@jtibshirani jtibshirani force-pushed the create-index-mappings branch from 7ff9d51 to 2b395aa Compare January 2, 2019 08:29
@jtibshirani jtibshirani changed the title Avoid doubly nested mappings in index creation requests. Stop automatically nesting mappings in index creation requests. Jan 2, 2019
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Nice.

@markharwood
Copy link
Contributor

Looks like there's some test failures stemming from mixed version clusters. The stack trace suggests it's the 6.7 node attempting some internal merging of mapping definitions from 7.0 and triggers the (removed in 7.0) exception complaining about the lack of root object:

"}],"type":"mapper_parsing_exception","reason":"Failed to parse mapping [doc]: malformed mapping no root object found","caused_by":{"type":"mapper_parsing_exception","reason":"malformed mapping no root object found","stack_trace":"MapperParsingException[malformed mapping no root object found]
	at org.elasticsearch.index.mapper.DocumentMapperParser.extractMapping(DocumentMapperParser.java:179)
	at org.elasticsearch.index.mapper.DocumentMapperParser.parse(DocumentMapperParser.java:85)
	at org.elasticsearch.index.mapper.MapperService.internalMerge(MapperService.java:395)
	at org.elasticsearch.index.mapper.MapperService.merge(MapperService.java:329)
	at org.elasticsearch.cluster.metadata.MetaDataCreateIndexService$IndexCreationTask.execute(MetaDataCreateIndexService.java:452)

@jtibshirani
Copy link
Contributor Author

Thanks @jpountz for the review, and @markharwood for digging into the build failure. I've decided to pull out the 'Make sure empty mappings are accepted' part of this PR into #37089. I can then backport that PR, which should fix the BWC failures we're now seeing around empty mappings.

@jtibshirani jtibshirani force-pushed the create-index-mappings branch from bfb70b8 to 6c258ca Compare January 3, 2019 22:49
@jtibshirani
Copy link
Contributor Author

@elasticmachine run gradle build tests 1

@jtibshirani jtibshirani merged commit ac1c694 into elastic:master Jan 4, 2019
@jtibshirani jtibshirani deleted the create-index-mappings branch January 4, 2019 01:41
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Jan 4, 2019
…tic#36924)

Now that we unwrap mappings in DocumentMapperParser#extractMappings, it is not
necessary for the mapping definition to always be nested under the type. This
leniency around the mapping format was added in 2341825.
@jtibshirani
Copy link
Contributor Author

I'm going to revert this PR because of the issue uncovered in #37208, and I think we need to reconsider the overall approach. In short, the problem is that index templates store the mapping value as a string, with the mapping definition is nested under the type name. When trying to create an index with unnested mappings, we attempt to merge that map with the nested mappings from the index template, which produces an incorrect result.

I now think that instead of standardizing on unnested mappings in indices.create + indices.put_template, for 7.0 we should actually standardize on having all mappings be nested, as @markharwood suggested in #35866. My reasoning is that stored index templates nest the mappings under the type name, so it is more robust and simpler to reason about if we stick to this format across all server requests + code related to index creation.

jtibshirani added a commit that referenced this pull request Jan 8, 2019
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search Foundations/Mapping Index mappings, including merging and defining field types v6.7.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants