Merge mappings for composable index templates #58521

jtibshirani · 2020-06-25T00:04:02Z

This PR implements recursive mapping merging for composable index templates.

When creating an index, we perform the following:

Add each component template mapping in order, merging each one in after the
last.
Merge in the index template mappings (if present).
Merge in the mappings on the index request itself (if present).

Some principles:

All 'structural' changes are disallowed (but everything else is fine). An
object mapper can never be changed between type: object and type: nested. A
field mapper can never be changed to an object mapper, and vice versa.
Generally, each section is merged recursively. This includes object
mappings, as well as root options like dynamic_templates and meta. Once we
reach 'leaf components' like field definitions, they always overwrite an
existing one instead of being merged.

The PR is pretty large, but each commit can be reviewed individually.

Relates to #53101.

The differences from standard mapping merge: * For metadata fields like _source, the whole field definition is replaced instead of merged. * Dynamic templates are appended to the end of the existing list. Any duplicate definitions overwrite existing ones. * The _meta object is deep-merged, so any existing keys are replaced.

elasticmachine · 2020-06-25T17:34:17Z

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

elasticmachine · 2020-06-25T17:34:19Z

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

server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java

dakrone

Thanks for working on this Julie! I left a couple of comments with (small!) substance and then a bunch of minor nits. I am not an expert in our mapping code so it would be good to have a second opinion also

server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java

server/src/main/java/org/elasticsearch/index/mapper/MapperService.java

server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java

jtibshirani · 2020-06-26T15:05:15Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java


        if (request.dataStreamName() != null) {
            DataStream dataStream = currentState.metadata().dataStreams().get(request.dataStreamName());
            if (dataStream != null) {
-                dataStream.getTimeStampField().insertTimestampFieldMapping(mappings);
+                mappings.add(dataStream.getTimeStampField().getTimestampFieldMapping());


After chatting with the core-features team, I now realize that we should apply request.mappings() after this datastream mapping. I'll fix this.

Actually @dakrone is that right? If we applied request.mappings() after the datastream timestamp mapping, it could result in an index with an inconsistent timestamp mapping. So perhaps we always need to apply the datastream timestamp mapping last.

I think the request.mappings() part last, but we need to validate that the composite mappings are valid for a data stream, that way a user could override part of the mappings (say for instance a custom format for the date), but couldn't do anything bad (like change it to a keyword for instance)

I'm going to keep it as-is for this PR, since it just tries to replicate the existing behavior. We can discuss in a follow-up if we want to change the order.

jimczi

The changes looks great. I like how it is integrated into the mapping merge, it makes the logic very easy to follow. I left one question and it would be nice to have a single place where the full merging logic is described.

server/src/main/java/org/elasticsearch/index/mapper/Mapping.java

jimczi · 2020-06-29T14:54:09Z

server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java

+            }
+        } else if (isEnabled() != mergeWith.isEnabled()) {
+            throw new MapperException("the [enabled] parameter can't be updated for the object mapping [" + name() + "]");
+        }



I wonder if we should nullify the underlying mappers if enabled is false ? It's not related to this pr specifically but we don't need to keep children when the parent is disabled ?

As you mentioned, I think this is general question. Even without this PR you can define an object mapper like this:

"object": { "type": "object", "enabled": false, "properties": { "field": { "type": "keyword" } } }

For this PR I plan to stick with the existing behavior, but we could have a separate discussion on whether we want to remove this flexibility.

Agreed, I opened #58702 to discuss this specifically.

Great thanks!

dakrone

LGTM, thanks Julie!

jtibshirani · 2020-06-29T20:57:37Z

@elasticmachine elasticsearch-ci/1

…pping

These were added in elastic#58521 but were incorrect -- at this point the mapping is not necessarily wrapped in the '_doc' type. The assertion likely didn't trip before because it's common for mappings to have only a top-level 'properties' key.

These were added in #58521 but were incorrect -- at this point the mapping is not necessarily wrapped in the '_doc' type. The assertion likely didn't trip before because it's common for mappings to have only a top-level 'properties' key.

This commit adds an integration test that component templates used to form a composite template can still be updated after a cluster restart. In elastic#58643 an issue arose where mappings were causing problems because of the way we unwrap `_doc` in template mappings. This was also related to the mappings being merged manually rather than using the `MapperService` to do the merging. elastic#58643 was fixed in 7.9 and master with the elastic#58521 change, since mappings now are read and digested by the actual mapper service. This test passes for 7.x and master, and I intend to open a separate PR including this test for 7.8.1 along with a bug fix for elastic#58643. This test is to ensure we don't have any regression in the future.

This commit adds an integration test that component templates used to form a composite template can still be updated after a cluster restart. In #58643 an issue arose where mappings were causing problems because of the way we unwrap `_doc` in template mappings. This was also related to the mappings being merged manually rather than using the `MapperService` to do the merging. #58643 was fixed in 7.9 and master with the #58521 change, since mappings now are read and digested by the actual mapper service. This test passes for 7.x and master, and I intend to open a separate PR including this test for 7.8.1 along with a bug fix for #58643. This test is to ensure we don't have any regression in the future.

…ic#58883) This commit adds an integration test that component templates used to form a composite template can still be updated after a cluster restart. In elastic#58643 an issue arose where mappings were causing problems because of the way we unwrap `_doc` in template mappings. This was also related to the mappings being merged manually rather than using the `MapperService` to do the merging. elastic#58643 was fixed in 7.9 and master with the elastic#58521 change, since mappings now are read and digested by the actual mapper service. This test passes for 7.x and master, and I intend to open a separate PR including this test for 7.8.1 along with a bug fix for elastic#58643. This test is to ensure we don't have any regression in the future.

…58883) (#58914) This commit adds an integration test that component templates used to form a composite template can still be updated after a cluster restart. In #58643 an issue arose where mappings were causing problems because of the way we unwrap `_doc` in template mappings. This was also related to the mappings being merged manually rather than using the `MapperService` to do the merging. #58643 was fixed in 7.9 and master with the #58521 change, since mappings now are read and digested by the actual mapper service. This test passes for 7.x and master, and I intend to open a separate PR including this test for 7.8.1 along with a bug fix for #58643. This test is to ensure we don't have any regression in the future.

Due to the way that our `toXContent` of mappings works, we reduce the type to remove `_doc` from template mappings when serializing them to disk as part of cluster state. This causes problems however, as we always expected there to be a `_doc` field in the mappings. This was resolved in later versions of ES (7.9+) by elastic#58521 which uses a real mapper to handle the mappings. For 7.8.1, however, we need to ensure that we can still read the state after it has been persisted to disk. Resolves elastic#58643 Relates to elastic#58883 Relates to elastic#58521

Due to the way that our `toXContent` of mappings works, we reduce the type to remove `_doc` from template mappings when serializing them to disk as part of cluster state. This causes problems however, as we always expected there to be a `_doc` field in the mappings. This was resolved in later versions of ES (7.9+) by #58521 which uses a real mapper to handle the mappings. For 7.8.1, however, we need to ensure that we can still read the state after it has been persisted to disk. Resolves #58643 Relates to #58883 Relates to #58521

Pass merge reason to Mapping#merge.

5fa0b7d

jtibshirani force-pushed the merge-template-mapping branch from c30d286 to 6f5e142 Compare June 25, 2020 01:20

jtibshirani added 5 commits June 24, 2020 18:37

Add an 'index template' merge reason.

2e7db91

Replace leaf fields when merging objects.

f44a5f9

Overwrite enabled, include_in_parent, include_in_root.

25ed9aa

Use new merge strategy for composable templates.

5a37903

jtibshirani force-pushed the merge-template-mapping branch from 6f5e142 to 5a37903 Compare June 25, 2020 03:05

jtibshirani added :Data Management/Indices APIs APIs to create and manage indices and templates :Search Foundations/Mapping Index mappings, including merging and defining field types >feature v7.9.0 v8.0.0 labels Jun 25, 2020

jtibshirani marked this pull request as ready for review June 25, 2020 17:34

elasticmachine added the Team:Data Management Meta label for data/management team label Jun 25, 2020

elasticmachine added the Team:Search Meta label for search team label Jun 25, 2020

jtibshirani requested review from dakrone, jimczi and romseygeek June 25, 2020 17:36

jtibshirani commented Jun 25, 2020

View reviewed changes

server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java Outdated Show resolved Hide resolved

dakrone requested changes Jun 25, 2020

View reviewed changes

jtibshirani commented Jun 26, 2020

View reviewed changes

jtibshirani added 3 commits June 26, 2020 11:24

Small fixes to DataStream#getTimestampFieldMapping.

2af0fb3

Lowercase error messages and remove punctuation.

f1422bd

Move check for redundant nested includes to MapperService#merge.

ae6c79d

jimczi approved these changes Jun 29, 2020

View reviewed changes

jtibshirani requested a review from dakrone June 29, 2020 18:57

dakrone approved these changes Jun 29, 2020

View reviewed changes

Add comments explaining how _meta and metadata fields are merged.

ff6833c

Merge remote-tracking branch 'upstream/master' into merge-template-ma…

1899c28

…pping

jimczi mentioned this pull request Jun 29, 2020

Should we ignore mapping properties on disabled objects #58702

Open

jtibshirani merged commit 676893a into elastic:master Jun 29, 2020

jtibshirani deleted the merge-template-mapping branch June 29, 2020 22:00

jtibshirani mentioned this pull request Jun 30, 2020

Merge mappings for composable index templates #58709

Merged

jtibshirani mentioned this pull request Jun 30, 2020

Remove assertions that mappings have one top-level key. #58779

Merged

dakrone mentioned this pull request Jul 1, 2020

Add test for component templates updated after cluster restart #58883

Merged

dakrone mentioned this pull request Jul 2, 2020

Fix issue reading template mappings after cluster restart #58964

Merged

dakrone mentioned this pull request Jul 6, 2020

component template cannot update after ES7.8 reboot #58643

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge mappings for composable index templates #58521

Merge mappings for composable index templates #58521

jtibshirani commented Jun 25, 2020 •

edited

Loading

elasticmachine commented Jun 25, 2020

elasticmachine commented Jun 25, 2020

dakrone left a comment

jtibshirani Jun 26, 2020

jtibshirani Jun 26, 2020

dakrone Jun 26, 2020

jtibshirani Jun 29, 2020

jimczi left a comment

jimczi Jun 29, 2020

jtibshirani Jun 29, 2020 •

edited

Loading

jimczi Jun 29, 2020

jtibshirani Jun 29, 2020

dakrone left a comment

jtibshirani commented Jun 29, 2020

Merge mappings for composable index templates #58521

Merge mappings for composable index templates #58521

Conversation

jtibshirani commented Jun 25, 2020 • edited Loading

elasticmachine commented Jun 25, 2020

elasticmachine commented Jun 25, 2020

dakrone left a comment

Choose a reason for hiding this comment

jtibshirani Jun 26, 2020

Choose a reason for hiding this comment

jtibshirani Jun 26, 2020

Choose a reason for hiding this comment

dakrone Jun 26, 2020

Choose a reason for hiding this comment

jtibshirani Jun 29, 2020

Choose a reason for hiding this comment

jimczi left a comment

Choose a reason for hiding this comment

jimczi Jun 29, 2020

Choose a reason for hiding this comment

jtibshirani Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

jimczi Jun 29, 2020

Choose a reason for hiding this comment

jtibshirani Jun 29, 2020

Choose a reason for hiding this comment

dakrone left a comment

Choose a reason for hiding this comment

jtibshirani commented Jun 29, 2020

jtibshirani commented Jun 25, 2020 •

edited

Loading

jtibshirani Jun 29, 2020 •

edited

Loading