RuntimeField to expose more than one MappedFieldType #73900

javanna · 2021-06-08T13:29:12Z

Up until now, there was a 1:1 relationship between a RuntimeField and a MappedFieldType. Actually, the two were only recently split to support emitting multiple fields from a single runtime script. The next step is to change the signature of asMappedFieldType to make it return multiple sub-fields. The leaf fields that are supported to-date will return a collection with a single item, but the upcoming runtime "object" field will return as many subfields as are listed in its definition.

Together with this, we are introducing two consistency checks:

sub-fields exposed by a RuntimeField either have its same name, or belong to its namespace (e.g. object.subfield)
there can't be two mapped field types with the same name as part of the same runtime section, overrides happen defining the same field in two different sections (index mappings and search request)

Up until now, there was a 1:1 relationship between a RuntimeField and a MappedFieldType. Actually, the two were only recently split to support emitting multiple fields from a single runtime script. The next step is to change the signature of asMappedFieldType to make it return multiple sub-fields. The leaf fields that are supported to-date will return a collection with a single item, but the upcoming runtime "object" field will return as many subfields as are listed in its definition. Together with this, we are introducing two consistency checks: - sub-fields exposed by a RuntimeField either have its same name, or belong to its namespace (e.g. object.subfield) - there can't be two mapped field types with the same name as part of the same runtime section, overrides happen defining the same field in two different sections (index mappings and search request)

elasticmachine · 2021-06-08T13:29:15Z

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

romseygeek

LGTM - I left a question about subfield naming though which I think we need to address before committing.

romseygeek · 2021-06-08T13:32:22Z

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

-        RuntimeField runtimeField = context.root().getRuntimeField(fieldPath);
-        if (runtimeField != null) {
-            return new NoOpFieldMapper(subfields[subfields.length - 1], runtimeField.asMappedFieldType().name());
+        MappedFieldType fieldType = context.mappingLookup().getFieldType(fieldPath);


Will this work with flattened fields? I guess they should already be picked up by the parent.

I think that flattened have a mapper that would be found above?

I also think that flattened fields will be found earlier. Would you be up for adding a test for this, as it seems quite subtle?

I opened #74112

Thanks for the extra test !

romseygeek · 2021-06-08T13:42:30Z

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

+                    .collect(Collectors.toList());
+                if (names.isEmpty() == false) {
+                    throw new IllegalArgumentException("Found sub-fields with name not belonging to the parent field they are part of "
+                        + names);


This is interesting, are we sure that we want to enforce this? We've discussed in the past wanting to have subfields that are aliased out to the root so that they don't appear in their parent object's namespace. If we do enforce this then that's another reason to allow runtime aliases to refer to other runtime fields.

I was assuming that we do want to enforce this, otherwise object "foo" can emit sub-fields that don't belong to its namespace. We were trying to apply a similar assertion with the dynamic field type approach but it turned out not to be feasible due to flattened and aliases, I think.

In reality, this will not be possible if we look at how you define sub-fields (yet I think it's good to check to prevent mistakes), because the full name is generated by concatenating the name of the parent and the name of the sub-field:

"log" : { "type" : "I don't have a name yet" "script" : "...." "fields" : { "client_ip" : { "type" : "ip" } } }

Maybe I should make it an illegal state instead of a user error...

While it is possible to have users define the full path in the sub-fields, that would make the "object" name almost useless and would feel weird. Your conclusion about aliases makes sense to me, although it is very easy to make an alias runtime field with a super small script?

I made this an IllegalStateException

Using IllegalStateException or an assertion here makes sense to me, as it'd indicate a bug in our code (or plugin authors' code). Not relevant to this PR, but I noticed we don't assert this for FieldMapper#iterator... maybe we should.

romseygeek

Thanks @javanna

Up until now, there was a 1:1 relationship between a RuntimeField and a MappedFieldType. Actually, the two were only recently split to support emitting multiple fields from a single runtime script. The next step is to change the signature of asMappedFieldType to make it return multiple sub-fields. The leaf fields that are supported to-date will return a collection with a single item, but the upcoming runtime "object" field will return as many subfields as are listed in its definition. Together with this, we are introducing two consistency checks: - sub-fields exposed by a RuntimeField either have its same name, or belong to its namespace (e.g. object.subfield) - there can't be two mapped field types with the same name as part of the same runtime section, overrides happen defining the same field in two different sections (index mappings and search request)

jtibshirani · 2021-06-08T18:19:01Z

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

-        RuntimeField runtimeField = context.root().getRuntimeField(fieldPath);
-        if (runtimeField != null) {
-            return new NoOpFieldMapper(subfields[subfields.length - 1], runtimeField.asMappedFieldType().name());
+        MappedFieldType fieldType = context.mappingLookup().getFieldType(fieldPath);


I also think that flattened fields will be found earlier. Would you be up for adding a test for this, as it seems quite subtle?

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

jtibshirani · 2021-06-08T18:50:11Z

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

+                    .collect(Collectors.toList());
+                if (names.isEmpty() == false) {
+                    throw new IllegalArgumentException("Found sub-fields with name not belonging to the parent field they are part of "
+                        + names);


Using IllegalStateException or an assertion here makes sense to me, as it'd indicate a bug in our code (or plugin authors' code). Not relevant to this PR, but I noticed we don't assert this for FieldMapper#iterator... maybe we should.

jtibshirani · 2021-06-08T18:51:00Z

Sorry for the late review, I had it open for a long time without refreshing the page!

javanna added :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v8.0.0 v7.14.0 labels Jun 8, 2021

javanna requested review from romseygeek and jtibshirani June 8, 2021 13:29

elasticmachine added the Team:Search Meta label for search team label Jun 8, 2021

iter

be21d4d

romseygeek reviewed Jun 8, 2021

View reviewed changes

iter

cfb1d07

romseygeek approved these changes Jun 8, 2021

View reviewed changes

javanna merged commit 7c641ec into elastic:master Jun 8, 2021

jtibshirani reviewed Jun 8, 2021

View reviewed changes

javanna mentioned this pull request Jun 15, 2021

Add a test for parsing doc with a flattened field #74112

Merged

cbuescher mentioned this pull request Jul 19, 2021

Indexing documents trying to overwrite multi fields no longer fails #75343

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

RuntimeField to expose more than one MappedFieldType #73900

RuntimeField to expose more than one MappedFieldType #73900

javanna commented Jun 8, 2021

elasticmachine commented Jun 8, 2021

romseygeek left a comment

romseygeek Jun 8, 2021

javanna Jun 8, 2021

jtibshirani Jun 8, 2021

javanna Jun 15, 2021

jtibshirani Jun 17, 2021

romseygeek Jun 8, 2021

javanna Jun 8, 2021

javanna Jun 8, 2021

jtibshirani Jun 8, 2021

romseygeek left a comment

jtibshirani Jun 8, 2021

jtibshirani Jun 8, 2021

jtibshirani commented Jun 8, 2021

RuntimeField to expose more than one MappedFieldType #73900

RuntimeField to expose more than one MappedFieldType #73900

Conversation

javanna commented Jun 8, 2021

elasticmachine commented Jun 8, 2021

romseygeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romseygeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani commented Jun 8, 2021