Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Term Vectors with artificial docs and keyword fields #53504

Merged
merged 8 commits into from Mar 13, 2020
Merged

Fix Term Vectors with artificial docs and keyword fields #53504

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fb058c6
Fix Term Vectors with artificial docs and keyword fields
matriv Mar 12, 2020
5c6b6ec
Merge remote-tracking branch 'upstream/master' into fix-53494
matriv Mar 12, 2020
7fd234c
move getValues to TermVectorsService
matriv Mar 12, 2020
ea1a4ef
address comments
matriv Mar 13, 2020
ea87219
Merge remote-tracking branch 'upstream/master' into fix-53494
matriv Mar 13, 2020
ffd1ac9
Merge remote-tracking branch 'upstream/master' into fix-53494
matriv Mar 13, 2020
f1b69af
add import
matriv Mar 13, 2020
38d1eeb
fix test
matriv Mar 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions server/src/main/java/org/elasticsearch/index/mapper/ParseContext.java
View file
Open in desktop
Expand Up @@ -131,8 +131,12 @@ public IndexableField[] getFields(String name) {
public final String[] getValues(String name) {
List<String> result = new ArrayList<>();
for (IndexableField field : fields) {
if (field.name().equals(name) && field.stringValue() != null) {
result.add(field.stringValue());
if (field.name().equals(name)) {
if (field.stringValue() != null) {
result.add(field.stringValue());
} else if (field.binaryValue() != null) { // KeywordFieldType
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

@matriv matriv Mar 12, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

result.add(field.binaryValue().utf8ToString());
}
}
}
return result.toArray(new String[result.size()]);
Expand Down
2 changes: 2 additions & 0 deletions server/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java
View file
Open in desktop
Expand Up @@ -128,6 +128,8 @@ public void testDefaults() throws Exception {
fieldType = fields[1].fieldType();
assertThat(fieldType.indexOptions(), equalTo(IndexOptions.NONE));
assertEquals(DocValuesType.SORTED_SET, fieldType.docValuesType());

assertArrayEquals(new String[] { "1234", "1234" }, doc.rootDoc().getValues("field")); // used for TermVectors
}

public void testIgnoreAbove() throws IOException {
Expand Down