Apply runtime mappings directly to MappingLookup #73572
Conversation
javanna
added
>enhancement
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
| this.indexTimeScriptMappers = mappingLookup.indexTimeScriptMappers; | ||
| this.fieldMappers = mappingLookup.fieldMappers; | ||
| this.objectMappers = mappingLookup.objectMappers; | ||
| this.fieldTypeLookup = new FieldTypeLookup(mappingLookup.fieldTypeLookup, runtimeMappings); |
There was a problem hiding this comment.
This is one way to achieve what we want (not duplicating code). I am happy with it code-wise. I do wonder if it's too heavy to make a copy of MappingLookup and FieldTypeLookup on each shard. I could not find a way to save work and share more between shards, even if we know that the runtime mappings will be the same on all shards for the same search request, because we parse them on the shards, hence we apply them there after parsing.
If we are worried about this, we could take a different approach, perhaps less elegant: avoid making copies of these two objects and rather expose new methods on them that allow to overlay runtime fields on the fly, a bit like search execution used to previously do. I am not too sure how this would look but I thought that it would not be ideal code-wise. Another idea I considered was using a search FieldTypeLookup within SearchExecutionContext: lightweight but in would introduce a new abstraction in SearchExecutionContext, would require to make FieldTypeLookup public etc.
I am curious to know what you think!
javanna
force-pushed
the
enhancement/search_exec_context_runtime_mappings
branch
from
84713aa to
1f4b299
Compare
SearchExecutionContext currently holds the runtime mappings and each field type lookup, or calls to getMatchingFieldNames or getMatchingFieldTypes, needs to have the runtime mappings applied first, as runtime mappings have the precedence when they have the same name as fields that come from the index mappings (runtime or not). This caused code duplication, as the logic in those three methods is exactly the same that we already have in FieldTypeLookup. Also, we recently enhanced FieldTypeLookup to apply dynamic field types provided with the runtime section, and the same needs to be then also applied to SearchExecutionContext so that it supports dynamic runtime fields as well. To address this problem, SearchExecutionContext has now access to a copy of MappingLookup with the runtime mappings applied to it, which is exactly the same MappingLookup as the original one, besides the FieldTypeLookup which is a copy of the original one with the runtime mappings applied on top.
javanna
force-pushed
the
enhancement/search_exec_context_runtime_mappings
branch
from
1f4b299 to
244f736
Compare
|
heads up I am investigating alternatives: I started with #73590 and I am looking into decreasing getMatchingFieldTypes usages or something along those lines, in the hope that we only have to apply the runtime mappings logic to getMatchingFieldNames and getFieldType. That may affect the approach we take trying to avoid code duplication. Copying all of the field type lookup does not seem like a great trade-off. |
|
Closing this, we will take a different and less invasive approach. We were able to already reduce the code duplication between FieldTypeLookup and SearchExecutionContext to one method instead of two: getMatchingFieldNames. |
SearchExecutionContext currently holds the runtime mappings and each field type lookup, or calls to getMatchingFieldNames or getMatchingFieldTypes, needs to have the runtime mappings applied first, as runtime mappings have the precedence when they have the same name as fields that come from the index mappings (runtime or not).
This caused code duplication, as the logic in those three methods is exactly the same that we already have in FieldTypeLookup. Also, we recently enhanced FieldTypeLookup to apply dynamic field types provided with the runtime section, and the same needs to be then also applied to SearchExecutionContext so that it supports dynamic runtime fields as well.
To address this problem, SearchExecutionContext has now access to a copy of MappingLookup with the runtime mappings applied to it, which is exactly the same MappingLookup as the original one, besides the FieldTypeLookup which is a copy of the original one with the runtime mappings applied on top.