Construct dynamic updates directly via object builders #81449

romseygeek · 2021-12-07T15:24:34Z

Currently, dynamic updates are built in the DocumentParser using a
stack of possibly-dynamic object mappers. This logic, spread across
a number of static methods, frequently assumes that the parents of
a mapper can be found by splitting its name on dots, an assumption
that will fail to hold once we allow objects to hold fields that have dots
in their names.

As a pre-requisite for the dots in field names effort, this commit refactors
the construction of dynamic updates into object mapper builders. Now,
to build an update, we start with a new dynamic root builder, and then
call addUpdate on it with each dynamically built mapper in turn. The
builder will examine the mapper and see if it can just add it to its own
set of mappers directly; and if not, it will retrieve or build an appropriate
intermediate object mapper and recursively call addUpdate on it with
the original mapper.

As a side-effect of this change, ObjectMapper itself no longer updates
its map of child mappers except during construction via merging, and so
we can safely replace CopyOnWriteHashMap here.

…-fields

…te-without-merging

elasticmachine · 2021-12-07T15:24:38Z

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

romseygeek · 2021-12-08T10:09:56Z

@elasticmachine update branch

nik9000 · 2021-12-08T14:44:25Z

As a side-effect of this change, ObjectMapper itself no longer updates
its map of child mappers except during construction via merging, and so
we can safely replace CopyOnWriteHashMap here.

That sounds lovely.

…te-without-merging

cbuescher

I did a first pass of this and left a few notes mostly for my own understanding. I didn't really go deep into the old logic in DocumentParser#createDynamicUpdate since it seems to be replaced by a completely different bottom-up-approach, but instead focused on that and the changes in the builders, which make sense to me so far. I still would like to give this a second pass and take another look at the test changes and would also appreciate a second pair of eyes here since this changes a lot of complex logic where I hope we cover most of the edge cases in tests already.

cbuescher · 2021-12-17T11:25:51Z

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

    protected volatile Dynamic dynamic;

-    protected volatile CopyOnWriteHashMap<String, Mapper> mappers;
+    protected Map<String, Mapper> mappers;


Just curious to understand why this changed...

Ah, I see, probably because its not mutable any more now? Could the map be it be final then?

Unfortunately not, it gets updated in doMerge(). Which is a whole 'nother can of worms.

cbuescher · 2021-12-17T11:30:28Z

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

+            this.runtimeFields.put(runtimeField.name(), runtimeField);
+            return this;
+        }
+
        public RootObjectMapper.Builder setRuntime(Map<String, RuntimeField> runtimeFields) {


nit: if this method now adds all input fields, could we rename it to "addRuntime" to make it clearer this doesn't overwrite the existing map?

Good idea, I'll update.

cbuescher · 2021-12-17T13:27:58Z

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

+                String fullChildName = prefix == null ? childName : prefix + "." + childName;
+                ObjectMapper.Builder childBuilder = findChild(childName, fullChildName, context);
+                childBuilder.addDynamic(name.substring(firstDotIndex + 1), fullChildName, mapper, context);
+                mappersBuilders.add(childBuilder);


I'm probably confused by the recursion here, but would it make sense to add the new childBuilder before the recursive step? Or maybe it doesn't matter, I now see that "findChild" only looks up previous dynamic updates in 'context.getObjectMapper(fullChildName)' and that wouldn't be affected by the mappersBuilders here? Just a bit confused, maybe you can explain.

Yes, findChild doesn't actually consult the existing mappersBuilders at all. In fact, I think the final branch of findChild, where we just return a new builder, won't ever be called now, because of #79922 - a dynamically built mapper will always have a parent that is either in the existing mappings, or has also been dynamically added. Maybe worth keeping it as a safety fall-back though.

In terms of adding the childBuilder to mappersBuilders before/after the recursion, it doesn't actually matter. The recursive step knows nothing about its parent (apart from using the passed-in prefix to build a full child name for lookup purposes), and the childBuilder itself is mutable so we can add a new mapper to it either before or after it has been added to mappersBuilders, it's the same object with the same result.

I think the thing the feels weird to me is that we're merging stuff from the tree of updates in the context and the tree under the object itself. It's, like, how you have to do it, I think. I left a note about the method itself below. Maybe I've found something? If I have maybe it'd give us something to pry on to simplify this.

It's not quite under the object itself, it's under a new, empty, objectbuilder that has been generated by the main object. I've tried to explain the recursion more fully in a comment below.

Interestingly, if I remove the third branch and throw an exception, it causes failures in tests that are checking how fields starting or ending with a dot behave. I'll see if I can pull field name validation out into a separate PR.

cbuescher

Did a second pass with a few more questions, other than that LGTM. Still worth checking from another point of view I think.

cbuescher · 2021-12-17T16:35:29Z

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

@@ -87,6 +86,64 @@ public Builder add(Mapper.Builder builder) {
            return this;
        }

+        public Builder addMappers(Map<String, Mapper> mappers) {


Looks like this can be package private, seems only be used by ObjectMapperMergeTests

cbuescher · 2021-12-17T16:54:12Z

server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java

+            b.endObject();
+            b.field("field", 10);
+            b.endObject();
+        }));


In your opinion, does changing these tests from directly testing DocumentParser.createDynamicUpdate() to parsing full json objects also change their coverage? As far as I understand, we previously sorted all mapper updates from a parse alphabetically (thus the sorting in the test setups). Now we don't need to do that anymore due to the new recursive strategy. However, I wonder if my re-ordering things in the json we might create a different update order and maybe different results/edge cases? I'm not sure this can happen so just wondering if that might be a concern.

The main difference I think is if we have two incompatible dynamic mappers for the same field - let's say you have something that looks like this:

{ "foo" : { "bar" : 1 }, "foo.bar" : "baz" }

We'll generate two mappings for 'foo.bar' here, one long and one text+keyword. Previously they would have been merged together and thrown an exception; now one will overwrite the other, but we'll still get an exception when the document is re-parsed if the long update is seen second and therefore 'wins'. I think this is OK though?

This also opens up possibilities for automatic widening of dynamic types - we could in the future have some kind of merge detection that says if you have a long mapper and a text mapper, you should always accept the text mapper when you combine the two in a dynamic context.

…te-without-merging

romseygeek · 2022-01-10T17:29:41Z

Another update, taking advantage of the changes in #82359.

This does now in fact cause a change in behaviour, due to #28948. Previously if you send in a document that looked like {"a.":{"b":{"c":"d"}}} you'd get an error about ambiguous object paths, but only if there wasn't already an object mapped called 'a' in your mappings. Due to how paths get split up and combined, the extra '.' on the end of a. would only matter when things were smooshed together to build a dynamic mapper; during parsing the trailing dots were stripped off by a call to String.split() and ignored. With the new way of building dynamic mappings, these trailing dots are always ignored - you'll only get an error if you send in something like {"a..b":{"c":"d"}}.

Given all the weird corner cases that #28948 causes I wonder if we should address it as part of this? It's an easy change to make in the document parsing code, although it may be a bit trickier as part of mapper parsing.

…te-without-merging

romseygeek · 2022-01-12T14:13:26Z

@elasticmachine run elasticsearch-ci/bwc

romseygeek · 2022-01-18T09:42:26Z

@elasticmachine update branch

nik9000

LGTM. I still don't fully understand it, but I'm closer to understand it than before. And a lot closer to understanding it than the old code. And we have tests.

romseygeek · 2022-01-25T11:04:56Z

@elasticmachine update branch

* upstream/master: (762 commits) [DOCS] Add note to that log4j customization is outside the support scope (elastic#82668) Batch Index Settings Update Requests (elastic#82896) [DOCS] Delete pipeline containing stored script (elastic#83102) Try again to fix changelog areas after reorg (elastic#83100) Bind to non-localhost for transport in some cases (elastic#82973) [DOCS] Reuse multi-level `join` warning (elastic#82976) Remove unnecessary CopyOnWriteHashMap class (elastic#83040) Adjust changelog categories after reorg (elastic#83087) [DOCS] Fix typo in `action.destructive_requires_name` breaking change (elastic#83085) Stack Monitoring: Add Enterprise Search monitoring index templates (elastic#82743) [DOCS] Fix stored script example snippet (elastic#83056) [DOCS] Re-add network traffic para to `term` query (elastic#83047) [DOCS] Rename example stored script (elastic#83054) [ML][DOCS] Add Trained model APIs to the REST APIs index (elastic#82791) [ML] Update running process when global calendar changes (elastic#83044) [Transform] Fix condition on which the transform stops processing buckets (elastic#82852) [DOCS] Fixes field names in ML sum functions. (elastic#83048) [ML] fix NLP tokenization never_split handling around punctuation (elastic#82982) Construct dynamic updates directly via object builders (elastic#81449) Emit trace.id into audit logs (elastic#82849) ... # Conflicts: # client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java # client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/ILMDocumentationIT.java # server/src/main/java/org/elasticsearch/action/admin/indices/rollover/Condition.java # server/src/test/java/org/elasticsearch/action/admin/indices/rollover/ConditionTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/RolloverActionTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java

When we apply dynamic mappings, we iterate over all the dynamic mappers retrieved from the DocumentParserContext, which are registered while parsing the document, and for each field look up their parent (going back multiple levels if necessary), and add it to the dynamic mapping update with the added/modified sub-field. Dynamic mappers that are iterated through consist of a flat set of mappers, which can be both object mappers or field mappers. Most times, the object mappers from such set have no sub-fields as they are mapped as a result of parsing a document where the object appears for the first time, which also has sub-fields that are going to be added to the set of dynamic mappers themselves once they are parsed. There are scenarios though where a dynamic template matches an object, and defines its structure including its subobjects as well as sub-fields. In that case the dynamically mapped object may hold sub-fields as well as define non-default values for dynamic, enabled or subobjects. The described situation was not well covered in tests so far, and is currently affected by a bug introduced with elastic#81449. When an object mapper is dynamically mapped, it is added to the map of dynamic object mappers, which makes it discoverable for sub-fields that will need to be added to it later, as well as to the set of dynamic mappers so that it becomes part of the mappings in case the document being parsed defines no sub-fields for it. What is missing is to recursively add its sub-fields to the dynamic object mappers. As a result of missing this step, intermediate objects that were dynamically mapped are not made discoverable which causes a cryptic "Missing intermediate object" error. This commit fixed the problem by recursively registering inner sub-objects to the dynamic mappers whenever an object mapper is added to the dynamic mappers. It also changes the "missing intermediate object" error to make it more evident that it's an internal error and not a user error: it is now an IllegalStateException instead of an IllegalArgumentException. Closes elastic#87513

…87622) When we apply dynamic mappings, we iterate over all the dynamic mappers retrieved from the DocumentParserContext, which are registered while parsing the document, and for each field look up their parent (going back multiple levels if necessary), and add it to the dynamic mapping update with the added/modified sub-field. Dynamic mappers that are iterated through consist of a flat set of mappers, which can be both object mappers or field mappers. Most times, the object mappers from such set have no sub-fields as they are mapped as a result of parsing a document where the object appears for the first time, which also has sub-fields that are going to be added to the set of dynamic mappers themselves once they are parsed. There are scenarios though where a dynamic template matches an object, and defines its structure including its subobjects as well as sub-fields. In that case the dynamically mapped object may hold sub-fields as well as define non-default values for dynamic, enabled or subobjects. The described situation was not well covered in tests so far, and is currently affected by a bug introduced with #81449. When an object mapper is dynamically mapped, it is added to the map of dynamic object mappers, which makes it discoverable for sub-fields that will need to be added to it later, as well as to the set of dynamic mappers so that it becomes part of the mappings in case the document being parsed defines no sub-fields for it. What is missing is to recursively add its sub-fields to the dynamic object mappers. As a result of missing this step, intermediate objects that were dynamically mapped are not made discoverable which causes a cryptic "Missing intermediate object" error. This commit fixed the problem by recursively registering inner sub-objects to the dynamic mappers whenever an object mapper is added to the dynamic mappers. It also changes the "missing intermediate object" error to make it more evident that it's an internal error and not a user error: it is now an IllegalStateException instead of an IllegalArgumentException. Closes #87513

…lastic#87622) When we apply dynamic mappings, we iterate over all the dynamic mappers retrieved from the DocumentParserContext, which are registered while parsing the document, and for each field look up their parent (going back multiple levels if necessary), and add it to the dynamic mapping update with the added/modified sub-field. Dynamic mappers that are iterated through consist of a flat set of mappers, which can be both object mappers or field mappers. Most times, the object mappers from such set have no sub-fields as they are mapped as a result of parsing a document where the object appears for the first time, which also has sub-fields that are going to be added to the set of dynamic mappers themselves once they are parsed. There are scenarios though where a dynamic template matches an object, and defines its structure including its subobjects as well as sub-fields. In that case the dynamically mapped object may hold sub-fields as well as define non-default values for dynamic, enabled or subobjects. The described situation was not well covered in tests so far, and is currently affected by a bug introduced with elastic#81449. When an object mapper is dynamically mapped, it is added to the map of dynamic object mappers, which makes it discoverable for sub-fields that will need to be added to it later, as well as to the set of dynamic mappers so that it becomes part of the mappings in case the document being parsed defines no sub-fields for it. What is missing is to recursively add its sub-fields to the dynamic object mappers. As a result of missing this step, intermediate objects that were dynamically mapped are not made discoverable which causes a cryptic "Missing intermediate object" error. This commit fixed the problem by recursively registering inner sub-objects to the dynamic mappers whenever an object mapper is added to the dynamic mappers. It also changes the "missing intermediate object" error to make it more evident that it's an internal error and not a user error: it is now an IllegalStateException instead of an IllegalArgumentException. Closes elastic#87513

…87622) When we apply dynamic mappings, we iterate over all the dynamic mappers retrieved from the DocumentParserContext, which are registered while parsing the document, and for each field look up their parent (going back multiple levels if necessary), and add it to the dynamic mapping update with the added/modified sub-field. Dynamic mappers that are iterated through consist of a flat set of mappers, which can be both object mappers or field mappers. Most times, the object mappers from such set have no sub-fields as they are mapped as a result of parsing a document where the object appears for the first time, which also has sub-fields that are going to be added to the set of dynamic mappers themselves once they are parsed. There are scenarios though where a dynamic template matches an object, and defines its structure including its subobjects as well as sub-fields. In that case the dynamically mapped object may hold sub-fields as well as define non-default values for dynamic, enabled or subobjects. The described situation was not well covered in tests so far, and is currently affected by a bug introduced with #81449. When an object mapper is dynamically mapped, it is added to the map of dynamic object mappers, which makes it discoverable for sub-fields that will need to be added to it later, as well as to the set of dynamic mappers so that it becomes part of the mappings in case the document being parsed defines no sub-fields for it. What is missing is to recursively add its sub-fields to the dynamic object mappers. As a result of missing this step, intermediate objects that were dynamically mapped are not made discoverable which causes a cryptic "Missing intermediate object" error. This commit fixed the problem by recursively registering inner sub-objects to the dynamic mappers whenever an object mapper is added to the dynamic mappers. It also changes the "missing intermediate object" error to make it more evident that it's an internal error and not a user error: it is now an IllegalStateException instead of an IllegalArgumentException. Closes #87513

romseygeek added 13 commits October 7, 2021 10:49

WIP

f54f87a

Merge remote-tracking branch 'origin/master' into mapper/dots-in-leaf…

578179d

…-fields

Add 'flatten' option to ObjectMapper

28233e3

Merge remote-tracking branch 'origin/master' into mapper/dots-in-leaf…

2b8b94f

…-fields

tidying up

75ca57c

tidy up

4ee36b4

Merge remote-tracking branch 'origin/master' into mapper/dots-in-leaf…

81a3613

…-fields

Merge remote-tracking branch 'origin/master' into mapper/dots-in-leaf…

4a47404

…-fields

Merge remote-tracking branch 'origin/master' into mapper/dynamic-upda…

4e977bf

…te-without-merging

Merge remote-tracking branch 'origin/master' into mapper/dynamic-upda…

f328c83

…te-without-merging

spotless

0078b19

Merge remote-tracking branch 'origin/master' into mapper/dynamic-upda…

ec17691

…te-without-merging

remove copyonwritehashmap from ObjectMapper

cc75426

romseygeek added :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v8.1.0 labels Dec 7, 2021

romseygeek requested review from nik9000, jpountz and cbuescher December 7, 2021 15:24

romseygeek self-assigned this Dec 7, 2021

elasticmachine added the Team:Search Meta label for search team label Dec 7, 2021

oops

39ade03

elasticmachine and others added 2 commits December 8, 2021 02:09

Merge branch 'master' into mapper/dynamic-update-without-merging

4c362c6

spotless

087886a

Merge remote-tracking branch 'origin/master' into mapper/dynamic-upda…

824f12c

…te-without-merging

cbuescher reviewed Dec 17, 2021

View reviewed changes

romseygeek mentioned this pull request Jan 10, 2022

Move ambiguous object field name detection into DotExpandingXContentParser #82359

Merged

romseygeek added 3 commits January 10, 2022 15:29

Merge remote-tracking branch 'origin/master' into mapper/dynamic-upda…

6e94b9b

…te-without-merging

Merge remote-tracking branch 'origin/master' into mapper/dynamic-upda…

0832808

…te-without-merging

spotless

efc371f

romseygeek added 2 commits January 12, 2022 13:42

Add back check for empty field name

025be06

Merge remote-tracking branch 'origin/master' into mapper/dynamic-upda…

8dc3fcf

…te-without-merging

Merge branch 'master' into mapper/dynamic-update-without-merging

660d3db

nik9000 approved these changes Jan 18, 2022

View reviewed changes

Merge branch 'master' into mapper/dynamic-update-without-merging

be672cc

romseygeek merged commit 002f506 into elastic:master Jan 25, 2022

romseygeek deleted the mapper/dynamic-update-without-merging branch January 25, 2022 12:21

romseygeek mentioned this pull request Jan 25, 2022

Should mappings keep using CopyOnWriteHashMap? #80072

Closed

javanna mentioned this pull request Apr 28, 2022

Add 'flatten' parameter to object mappers #78997

Closed

pjhampton mentioned this pull request Jun 8, 2022

Missing intermediate objects when applying dynamic template that targets an object #87513

Closed

javanna mentioned this pull request Jun 14, 2022

Fix missing intermediate object error when applying dynamic template #87622

Merged

javanna mentioned this pull request Jun 17, 2022

[8.3] Fix missing intermediate object error when applying dynamic template #87801

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Construct dynamic updates directly via object builders #81449

Construct dynamic updates directly via object builders #81449

romseygeek commented Dec 7, 2021

elasticmachine commented Dec 7, 2021

romseygeek commented Dec 8, 2021

nik9000 commented Dec 8, 2021

cbuescher left a comment

cbuescher Dec 17, 2021

cbuescher Dec 17, 2021

romseygeek Dec 17, 2021

cbuescher Dec 17, 2021

romseygeek Dec 17, 2021

cbuescher Dec 17, 2021

romseygeek Dec 17, 2021

nik9000 Jan 4, 2022

romseygeek Jan 6, 2022

romseygeek Jan 6, 2022

cbuescher left a comment

cbuescher Dec 17, 2021

cbuescher Dec 17, 2021

romseygeek Dec 20, 2021

romseygeek commented Jan 10, 2022

romseygeek commented Jan 12, 2022

romseygeek commented Jan 18, 2022

nik9000 left a comment

romseygeek commented Jan 25, 2022

Construct dynamic updates directly via object builders #81449

Construct dynamic updates directly via object builders #81449

Conversation

romseygeek commented Dec 7, 2021

elasticmachine commented Dec 7, 2021

romseygeek commented Dec 8, 2021

nik9000 commented Dec 8, 2021

cbuescher 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

Choose a reason for hiding this comment

cbuescher 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

romseygeek commented Jan 10, 2022

romseygeek commented Jan 12, 2022

romseygeek commented Jan 18, 2022

nik9000 left a comment

Choose a reason for hiding this comment

romseygeek commented Jan 25, 2022