Make FieldNamesFieldMapper responsible for adding its own doc fields#71929
Make FieldNamesFieldMapper responsible for adding its own doc fields#71929romseygeek merged 5 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/es-search (Team:Search) |
javanna
left a comment
There was a problem hiding this comment.
left a couple of small comments, LGTM otherwise. Thanks, this is a good cleanup.
| } | ||
|
|
||
| protected final void createFieldNamesField(ParseContext context) { | ||
| assert fieldType().hasDocValues() == false : "_field_names should only be used when doc_values are turned off"; |
There was a problem hiding this comment.
is there a way to keep this assertion? I vaguely remember adding it while cleaning up some inconsistencies around using field_names where it would not make sense
There was a problem hiding this comment.
I've added an assertion in FieldNamesFieldMapper.postParse()
|
|
||
| }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
on simplifying this and not indexing all the inner paths, that makes sense, but should we make that change in a separate PR?
There was a problem hiding this comment.
That's a good point, I'll pull this change into a separate issue.
| } | ||
|
|
||
| @Override | ||
| public void addFieldExistsField(String field) { |
There was a problem hiding this comment.
nit: I slightly prefer the previous name, shall we call it addFieldToFieldNames ?
There was a problem hiding this comment.
I ended up with addtoFieldNames
| } | ||
|
|
||
| @Override | ||
| public Collection<String> getFieldExistsFields() { |
…71929) The FieldNamesFieldMapper is a metadata mapper defining a field that can be used for exists queries if a mapper does not use doc values or norms. Currently, data is added to it via a special method on FieldMapper that pulls the metadata mapper from a mapping lookup, checks to see if it is enabled, and then adds the relevant value to a lucene document. This is one of only two places that pulls a metadata mapper from the MappingLookup, and it would be nice to remove this method. This commit refactors field name handling by instead storing the names of fields to index in the fieldnames field in a set on the ParseContext, and then building the field itself in FieldNamesFieldMapper.postParse(). This means that all of the responsibility for enabling indexing, etc, is handled within the metadata mapper itself.
The FieldNamesFieldMapper is a metadata mapper defining a field that
can be used for exists queries if a mapper does not use doc values or
norms. Currently, data is added to it via a special method on FieldMapper
that pulls the metadata mapper from a mapping lookup, checks to see
if it is enabled, and then adds the relevant value to a lucene document.
This is one of only two places that pulls a metadata mapper from the
MappingLookup, and it would be nice to remove this method. This commit
refactors field name handling by instead storing the names of fields to
index in the fieldnames field in a set on the ParseContext, and then
building the field itself in FieldNamesFieldMapper.postParse(). This means
that all of the responsibility for enabling indexing, etc, is handled within
the metadata mapper itself.
This commit also removes indexing of all parent paths. Previously, given
a field
parent.child.grandchildwe would indexparent,parent.childand
parent.child.grandchildin the field names field, and this was originallyused to allow efficient exists queries on object fields. However, since we
moved to using docvalue iterators for exists fields where possible, exists
queries against object fields have been built differently and so these
extra paths are unused.