-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Ensure dynamicMapping updates are handled in insertion order #103047
Ensure dynamicMapping updates are handled in insertion order #103047
Conversation
Pinging @elastic/es-search (Team:Search) |
Hi @benwtrent, I've created a changelog YAML for you. |
I confirmed this fix by running: #103015 The runtime is now at or below what it was in 8.10. I additionally double checked that the flame graphs were similar, and indeed they are. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Way to go @benwtrent!
💚 Backport successful
|
…#103047) The switch to holding dynamic fields in a hashmap effectively randomizes their iteration order. This can be troublesome when building the mapping update required by these updates. When iterating in an unknown order, recursing to the leaf mapper can occur many times `O(n^2)`. However, starting with insertion order, it will occur only `O(n)` times. closes: elastic#103011
#103083) The switch to holding dynamic fields in a hashmap effectively randomizes their iteration order. This can be troublesome when building the mapping update required by these updates. When iterating in an unknown order, recursing to the leaf mapper can occur many times `O(n^2)`. However, starting with insertion order, it will occur only `O(n)` times. closes: #103011
@@ -166,9 +166,9 @@ protected DocumentParserContext( | |||
mappingParserContext, | |||
source, | |||
new HashSet<>(), | |||
new HashMap<>(), | |||
new LinkedHashMap<>(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we restrict the type of the fields to LinkedHashMap
?
i.e.
- private final Map<String, List<Mapper.Builder>> dynamicMappers;
+ private final LinkedHashMap<String, List<Mapper.Builder>> dynamicMappers;
private final Set<String> newFieldsSeen;
- private final Map<String, ObjectMapper.Builder> dynamicObjectMappers;
+ private final LinkedHashMap<String, ObjectMapper.Builder> dynamicObjectMappers;
private final List<RuntimeField> dynamicRuntimeFields;
private final DocumentDimensions dimensions;
private final ObjectMapper parent;
@@ -102,9 +102,9 @@ public abstract class DocumentParserContext {
MappingParserContext mappingParserContext,
SourceToParse sourceToParse,
Set<String> ignoreFields,
- Map<String, List<Mapper.Builder>> dynamicMappers,
+ LinkedHashMap<String, List<Mapper.Builder>> dynamicMappers,
Set<String> newFieldsSeen,
- Map<String, ObjectMapper.Builder> dynamicObjectMappers,
+ LinkedHashMap<String, ObjectMapper.Builder> dynamicObjectMappers,
…lastic#103047)" This reverts commit d6e8217.
To clarify to folks digging between all the various issues, this PR didn't fully address the problem. See #103011 (comment). The real fix was in 8.11.4, and was a full revert of the originating bugged code. |
The switch to holding dynamic fields in a hashmap effectively randomizes their iteration order. This can be troublesome when building the mapping update required by these updates. When iterating in an unknown order, recursing to the leaf mapper can occur many times
O(n^2)
. However, starting with insertion order, it will occur onlyO(n)
times.closes: #103011