Add NestedPathFieldMapper to store nested path information #51100

romseygeek · 2020-01-16T13:45:37Z

Currently nested documents repurpose the _type field to store their nested paths.
This commit adds a dedicated _nested_path field instead, which decouples this
information from types and will allow the removal of the _type field entirely further
down the line. To preserve backwards compatibility, references to this field are
mediated via methods that take an index settings object, and indexes created before
8x still use the _type field.

Relates to #41059
Closes #24362

…han type field

elasticmachine · 2020-01-16T13:45:39Z

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

elasticmachine · 2020-01-16T13:45:41Z

Pinging @elastic/es-distributed (:Distributed/CRUD)

romseygeek · 2020-01-16T13:46:26Z

Note: this supersedes #50312, which removed the type field entirely; this defers that removal to a later commit

…ng updates due to the _type field being overloaded

romseygeek · 2020-01-16T14:57:51Z

@elasticmachine run elasticsearch-ci/2

jimczi

I did a first pass and added some comments. Overall it looks good to me.

jimczi · 2020-01-17T09:17:07Z

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

+
+        @Override
+        public IndexFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName) {
+            Function<MapperService, String> typeFunction = mapperService -> mapperService.documentMapper().type();


should we disallow fielddata on this field ? It's an implementation details and I am not sure that we want to expose it in aggs or sorting ?

Maybe keep it only for indices created before 8 to handle bwc ?

I'll remove it - indices created before 8 still use TypeFieldMapper, so BWC is handled that way

I pushed 7ea0da8

jimczi · 2020-01-17T09:18:56Z

server/src/main/java/org/elasticsearch/indices/IndicesModule.java

@@ -157,6 +158,7 @@ public IndicesModule(List<MapperPlugin> mapperPlugins) {
        builtInMetadataMappers.put(IndexFieldMapper.NAME, new IndexFieldMapper.TypeParser());
        builtInMetadataMappers.put(SourceFieldMapper.NAME, new SourceFieldMapper.TypeParser());
        builtInMetadataMappers.put(TypeFieldMapper.NAME, new TypeFieldMapper.TypeParser());


Can you switch to the NestedPathFieldMapper.TypeParser in this pr since you handle bwc in the new field type ?

We may be able to in future, but I think we need to keep TypeFieldMapper around for the moment because it's still used for type queries, etc.

Ok I was confused because there are some logic to handle bwc in NestedPathFieldMapper.

…d-field-mapper

jimczi · 2020-01-17T15:14:48Z

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

-        this.nestedTypePathAsString = "__" + fullPath;
-        this.nestedTypePathAsBytes = new BytesRef(nestedTypePathAsString);
-        this.nestedTypeFilter = new TermQuery(new Term(TypeFieldMapper.NAME, nestedTypePathAsBytes));
+        this.nestedTypePath = "__" + fullPath;


For indices created in v8 we don't need to add any prefix since we use a dedicated field ?

++ I pushed 7ea0da8

jimczi · 2020-01-17T15:15:58Z

server/src/main/java/org/elasticsearch/indices/IndicesModule.java

@@ -157,6 +158,7 @@ public IndicesModule(List<MapperPlugin> mapperPlugins) {
        builtInMetadataMappers.put(IndexFieldMapper.NAME, new IndexFieldMapper.TypeParser());
        builtInMetadataMappers.put(SourceFieldMapper.NAME, new SourceFieldMapper.TypeParser());
        builtInMetadataMappers.put(TypeFieldMapper.NAME, new TypeFieldMapper.TypeParser());


Ok I was confused because there are some logic to handle bwc in NestedPathFieldMapper.

jimczi · 2020-01-17T15:21:43Z

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

+        fields.add(new Field(fieldType().name(), MapperService.SINGLE_MAPPING_NAME, fieldType()));
+        if (fieldType().hasDocValues()) {
+            fields.add(new SortedSetDocValuesField(fieldType().name(), new BytesRef(MapperService.SINGLE_MAPPING_NAME)));
+        }


I don't think we need to index root document in this field. We use the nested path to find nested documents but we don't use this field to detect root documents so this indexing would be redundant ?

++ I pushed 7ea0da8

romseygeek · 2020-01-20T10:58:42Z

@elasticmachine update branch

romseygeek · 2020-01-20T13:17:04Z

@elasticmachine run elasticsearch-ci/2

jimczi

I left some minor comments, LGTM otherwise.

jimczi · 2020-01-22T14:17:11Z

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

@@ -118,7 +118,7 @@
    //TODO this needs to be cleaned up: _timestamp and _ttl are not supported anymore, _field_names, _seq_no, _version and _source are
    //also missing, not sure if on purpose. See IndicesModule#getMetadataMappers
    private static final String[] SORTED_META_FIELDS = new String[]{
-        "_id", IgnoredFieldMapper.NAME, "_index", "_routing", "_size", "_timestamp", "_ttl", "_type"
+        "_id", IgnoredFieldMapper.NAME, "_index", "_routing", "_size", "_timestamp", "_ttl", "_type", "_nested_path"


I think you need to preserve the sort order ?

++, will fix. I had a look at fixing the TODO above, but it's not all that straightforward unfortunately.

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

…d-field-mapper

…apper' into types-removal/nested-field-mapper

Add new NestedPathFieldMapper, migrate nested docs to use it rather t…

35a8c50

…han type field

romseygeek added :Search Foundations/Mapping Index mappings, including merging and defining field types :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >refactoring v8.0.0 labels Jan 16, 2020

romseygeek self-assigned this Jan 16, 2020

romseygeek mentioned this pull request Jan 16, 2020

Don't use _type field for nested paths #50312

Closed

Don't add nested field mapper for pre-8 indexes, as this breaks mappi…

89c13b7

…ng updates due to the _type field being overloaded

romseygeek added 3 commits January 16, 2020 15:30

test different built-in mapper counts on different versions

894bb52

doh

39d2d1a

Adjust isMetaDataField test to take version into account

0fff0cf

jimczi reviewed Jan 17, 2020

View reviewed changes

romseygeek added 3 commits January 17, 2020 09:44

Merge remote-tracking branch 'origin/master' into types-removal/neste…

81c36c7

…d-field-mapper

Merge remote-tracking branch 'origin/master' into types-removal/neste…

bba4d6a

…d-field-mapper

No fielddata on nested path

7ea0da8

jimczi reviewed Jan 17, 2020

View reviewed changes

feedback

0302900

Merge branch 'master' into types-removal/nested-field-mapper

e59a329

jimczi approved these changes Jan 22, 2020

View reviewed changes

romseygeek added 5 commits January 22, 2020 14:42

Merge remote-tracking branch 'origin/master' into types-removal/neste…

befa0ec

…d-field-mapper

Fix sort order

b90105d

Merge remote-tracking branch 'romseygeek/types-removal/nested-field-m…

7d21c65

…apper' into types-removal/nested-field-mapper

compilation

ad8ac4d

fieldtype

bafb96b

romseygeek merged commit 1dc9dd4 into elastic:master Jan 22, 2020

romseygeek deleted the types-removal/nested-field-mapper branch January 22, 2020 16:31

jpountz mentioned this pull request Mar 11, 2020

Types removal in 8.0 #41059

Closed

66 tasks

pgomulka mentioned this pull request Mar 25, 2020

Types removal - compatible changes #54160

Closed

66 tasks

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

Add NestedPathFieldMapper to store nested path information #51100

Add NestedPathFieldMapper to store nested path information #51100

Uh oh!

Conversation

romseygeek commented Jan 16, 2020

Uh oh!

elasticmachine commented Jan 16, 2020

Uh oh!

elasticmachine commented Jan 16, 2020

Uh oh!

romseygeek commented Jan 16, 2020

Uh oh!

romseygeek commented Jan 16, 2020

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Jan 20, 2020

Uh oh!

romseygeek commented Jan 20, 2020

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!