Handle runtime subfields when shadowing dynamic mappings #75595
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
romseygeek
added
>enhancement
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
|
@elasticmachine update branch |
cbuescher
approved these changes
Jul 22, 2021
| @@ -308,7 +308,7 @@ public void testDualingQueries() throws IOException { | |||
| String source = "{\"foo\": " + values + "}"; | |||
| XContentParser parser = createParser(JsonXContent.jsonXContent, source); | |||
| SourceToParse sourceToParse = new SourceToParse("test", "test", new BytesArray(source), XContentType.JSON); | |||
| DocumentParserContext ctx = new TestDocumentParserContext(null, null, null, null, sourceToParse) { | |||
| DocumentParserContext ctx = new TestDocumentParserContext(MappingLookup.EMPTY, null, null, null, sourceToParse) { | |||
There was a problem hiding this comment.
There is another 0-argument ctor in TestDocumentParserContext that seems unused, maybe we should delete it or otherwise also use MappingLookup.EMPTY as first argument there to avoid suprises later.
There was a problem hiding this comment.
Good catch - that's actually causing some percolator failures, will switch it to always use MappingLookup.EMPTY
| if (fieldType != null) { | ||
| RuntimeField runtimeField = context.root().getRuntimeField(fieldPath); | ||
| if (runtimeField != null) { | ||
| assert fieldType.hasDocValues() == false && fieldType.isAggregatable() && fieldType.isSearchable(); |
There was a problem hiding this comment.
I don't know if its useful to keep this assertion, no strong opinion, just curious if you deleted it for some specific reason.
There was a problem hiding this comment.
I think it was an extra check that this was definitely a runtime field. I don't think it's necessary any longer because we now know where the shadowing logic is pulling fields from.
romseygeek
added
the
auto-backport
💔 Backport failed
To backport manually run: |
romseygeek
added a commit
that referenced
this pull request
Jul 22, 2021
In #75454 we changed our dynamic shadowing logic to check that an unmapped field was truly shadowed by a runtime field before returning no-op mappers. However, this does not handle the case where the runtime field can have multiple subfields, as will be true for the upcoming composite field type. We instead need to check that the field in question would not be shadowed by any field type returned by any runtime field. This commit abstracts this logic into a new isShadowed() method on DocumentParserContext, which uses a set of runtime field type names built from the mapping lookup at construction time. It also simplifies the no-op mapper slightly by making it a singleton object, as we don't need to preserve field names here.
ywangd
pushed a commit
to ywangd/elasticsearch
that referenced
this pull request
Jul 30, 2021
In elastic#75454 we changed our dynamic shadowing logic to check that an unmapped field was truly shadowed by a runtime field before returning no-op mappers. However, this does not handle the case where the runtime field can have multiple subfields, as will be true for the upcoming composite field type. We instead need to check that the field in question would not be shadowed by any field type returned by any runtime field. This commit abstracts this logic into a new isShadowed() method on DocumentParserContext, which uses a set of runtime field type names built from the mapping lookup at construction time. It also simplifies the no-op mapper slightly by making it a singleton object, as we don't need to preserve field names here.
romseygeek
added a commit
that referenced
this pull request
Aug 3, 2021
#75595 added better checks for fields shadowed by runtime fields, so that we don't index data that would never be searched. However, the shadow lookup was being rebuilt for every document, which has caused a noticeable regression in indexing times. This commit reworks things so that this lookup is built once per mapping update and lives on MappingLookup.
javanna
added a commit
that referenced
this pull request
Mar 29, 2022
With #75454 we have fixed a buggy behaviour that was causing us to treat multi-fields as if they were runtime fields when retrieving mappers for the fields encountered while parsing incoming documents. As a follow-up #75595 introduced MappingLookup#isShadowed as a primitive to check if a field is a runtime field which covers also for subfields of the runtime composite field which the previous check did not. With recent changes around expanding dots in field names, the situation where we previously ended up confusing a multi-field for a runtime field no longer manifests.This allows us to potentially remove isShadowed and go back to the original logic + assertion: if we could not resolve the field to a mapper by name, and we can resolve it to a field type by name, such field must be a runtime field. This is maybe not as elegant as isShadowed but I found its naming a bit confusing and I was hoping we can avoid maintaining a specialized method around this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #75454 we changed our dynamic shadowing logic to check that an unmapped
field was truly shadowed by a runtime field before returning no-op mappers. However,
this does not handle the case where the runtime field can have multiple subfields, as
will be true for the upcoming composite field type. We instead need to check that
the field in question would not be shadowed by any field type returned by any
runtime field.
This commit abstracts this logic into a new
isShadowed()method onDocumentParserContext, which uses a set of runtime field type names built from
the mapping lookup at construction time. It also simplifies the no-op mapper
slightly by making it a singleton object, as we don't need to preserve field names
here.