Fix Term Vectors with artificial docs and keyword fields #53504
Conversation
Previously, Term Vectors API was returning empty results for artificial documents with keyword fields. Checking only for `string()` on `IndexableField` is not enough, since for `KeywordFieldType` `binaryValue()` must be used instead. Fixes elastic#53494
matriv
added
>bug
:Search/Search
|
Pinging @elastic/es-search (:Search/Search) |
| if (field.name().equals(name)) { | ||
| if (field.stringValue() != null) { | ||
| result.add(field.stringValue()); | ||
| } else if (field.binaryValue() != null) { // KeywordFieldType |
There was a problem hiding this comment.
We cannot use the binary value without checking the type of the field. It could be a real binary field that doesn't store utf8 bytes. Since this function is only used in the terms vector service, can you move it there and adds a check to extract binary values only if the field is of type keyword ?
There was a problem hiding this comment.
The method is called as you said only by TermVectorService and it's guarded by isValidField:
https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java#L304 but I agree, it makes sense to move the method close to this so it's visible.
|
@jimczi I moved it to |
| for (IndexableField field : fields) { | ||
| if (field.stringValue() != null) { | ||
| result.add(field.stringValue()); | ||
| } else if (field.binaryValue() != null) { // KeywordFieldType |
There was a problem hiding this comment.
I think that this will create duplicates due to doc values field that also use a binary value (SortedSetDocValuesField). You should change this to something like:
if (field.fieldType() instanceof KeywordFieldMapper.KeywordFieldType) {
result.add(field.binaryValue().utf8ToString());
} else if (field.fieldType() instanceof StringFieldType) {
result.add(field.stringValue());
}
Previously, Term Vectors API was returning empty results for artificial documents with keyword fields. Checking only for `string()` on `IndexableField` is not enough, since for `KeywordFieldType` `binaryValue()` must be used instead. Fixes elastic#53494 (cherry picked from commit 1fc3fe3)
Previously, Term Vectors API was returning empty results for
artificial documents with keyword fields. Checking only for
string()on
IndexableFieldis not enough, since forKeywordFieldTypebinaryValue()must be used instead.Fixes #53494