Report synthetic source status in MapperBuilderContext #91400
Conversation
|
Pinging @elastic/es-search (Team:Search) |
|
|
||
| if (metadataFieldMapper instanceof SourceFieldMapper sfm) { | ||
| isSourceSynthetic = sfm.isSynthetic(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This bit is why I didn't go this way when I was first looking at synthetic source stuff. But what you've done kind of makes sense. Metadata fields can't really care about whether or not they are synthetic.
There was a problem hiding this comment.
Right, this only applies to fields that come from the source. Which, thinking about it, some metadata fields can, like _doc_count. And looking more closely, I think _doc_count doesn't support synthetic source at all at the moment, but will just silently fail to return a value when the source is reconstructed. So we should probably do something about that...
There was a problem hiding this comment.
Yeah. I was thinking maybe metadata fields need to get a version that throws an exception if you check if synthetic source is enabled. Or maybe get an object that doesn't have that method. Not sure which is more sane.
There was a problem hiding this comment.
@romseygeek and I talked in another channel and figured out that the metadata field _doc_count needs to know if synthetic source is enabled. Well, it will when I add support for synthetic _source to it. Which I should do! So I think we should merge this as is and I'll add support for doc count on top of it, messing with this code right here to make the synthetic _source-ness available to metadata mappers.
| @@ -85,7 +85,7 @@ protected Collection<Class<? extends Plugin>> getPlugins() { | |||
|
|
|||
| public <IFD extends IndexFieldData<?>> IFD getForField(String type, String fieldName, boolean docValues) { | |||
| final MappedFieldType fieldType; | |||
| final MapperBuilderContext context = MapperBuilderContext.ROOT; | |||
| final MapperBuilderContext context = MapperBuilderContext.root(false); | |||
There was a problem hiding this comment.
I was going to say this test will need to be modified to deal with synthetic source fields - but looking at it more I'm kind of confused by this test. I kind of wonder what it's for. It looks like it's testing field data abstractions without needing the whole mapped field type thing. That feels a like maybe not something worth doing. Those aren't too hard to make. We do it all the time in aggs land.
There was a problem hiding this comment.
Yeah, it's a bit of an odd short-cut but it's used in quite a few places so I don't think it's worth touching in this PR
| @@ -50,4 +55,8 @@ public String buildFullName(String name) { | |||
| } | |||
| return path + "." + name; | |||
| } | |||
|
|
|||
| public boolean isSourceSynthetic() { | |||
There was a problem hiding this comment.
Should this throw an exception if called for metadata fields?
There was a problem hiding this comment.
We decided offline to support metadata fields in a follow-up. _doc_count doesn't currently support synthetic source but it will need to.
We currently work out whether or not a mapper should be storing additional
values for synthetic source by looking at the DocumentParserContext. However,
this value does not change for the lifetime of the mapper - it is defined by
metadata on the root mapper and is immutable - and DocumentParserContext
feels like the wrong place for this information as it holds context specific
to the document being parsed.
This commit moves synthetic source status information from DocumentParserContext
to MapperBuilderContext instead. Mappers which need this information retrieve
it at build time and hold it on final fields.