Skip to content

Fix highlighting on a stored keyword field #21645

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

Merged
merged 2 commits into from
Nov 21, 2016
Merged

Fix highlighting on a stored keyword field #21645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89ce6c2
Fix highlighting on a stored keyword field
jimczi Nov 18, 2016
b58c131
Replace BytesRef#utf8ToString with MappedFieldType#valueForDisplay
jimczi Nov 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/PlainHighlighter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.apache.lucene.search.highlight.SimpleHTMLFormatter;
import org.apache.lucene.search.highlight.SimpleSpanFragmenter;
import org.apache.lucene.search.highlight.TextFragment;
import org.apache.lucene.util.BytesRef;
import org.apache.lucene.util.BytesRefHash;
import org.apache.lucene.util.CollectionUtil;
import org.elasticsearch.ExceptionsHelper;
Expand Down Expand Up @@ -106,7 +107,12 @@ public HighlightField highlight(HighlighterContext highlighterContext) {
textsToHighlight = HighlightUtils.loadFieldValues(field, mapper, context, hitContext);

for (Object textToHighlight : textsToHighlight) {
String text = textToHighlight.toString();
String text;
if (textToHighlight instanceof BytesRef) {
text = mapper.fieldType().valueForDisplay(textToHighlight).toString();
} else {
text = textToHighlight.toString();
}
Copy link
Contributor

@jpountz jpountz Nov 18, 2016

Choose a reason for hiding this comment

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

would it work if we called MappedFieldType.valueForDisplay instead? I'm concerned some fields are stored as bytesrefs too but do not represent utf8 strings, like ip addresses

Copy link
Contributor Author

@jimczi jimczi Nov 18, 2016

Choose a reason for hiding this comment

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

I agree that it's better to call valueForDisplay (I'll change the PR) though we are protected here since the highlighting can be done only on text or keyword fields.

Copy link
Contributor

@jpountz jpountz Nov 18, 2016

Choose a reason for hiding this comment

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

Should we just do String text = mapper.fieldType().valueForDisplay(textToHighlight).toString(); all the time? This looks correct to me and is more future-proof? If that does not work, feel free to merge the PR as-is.

Copy link
Contributor Author

@jimczi jimczi Nov 21, 2016

Choose a reason for hiding this comment

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

The type of textToHighlight can be different whether it comes from the _source or from the stored fields. Calling valueForDisplay on it when it comes from _source would not work.


try (TokenStream tokenStream = analyzer.tokenStream(mapper.fieldType().name(), text)) {
if (!tokenStream.hasAttribute(CharTermAttribute.class) || !tokenStream.hasAttribute(OffsetAttribute.class)) {
Expand Down
25 changes: 23 additions & 2 deletions .../src/test/java/org/elasticsearch/search/fetch/subphase/highlight/HighlighterSearchIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import org.elasticsearch.action.index.IndexRequestBuilder;
import org.elasticsearch.action.search.SearchRequestBuilder;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.action.search.SearchType;
import org.elasticsearch.action.support.WriteRequest;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.geo.GeoPoint;
Expand All @@ -41,7 +40,6 @@
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.index.query.functionscore.FunctionScoreQueryBuilder;
import org.elasticsearch.index.search.MatchQuery;
import org.elasticsearch.indices.IndicesRequestCache;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.search.SearchHit;
Expand Down Expand Up @@ -106,6 +104,29 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
return Collections.singletonList(InternalSettingsPlugin.class);
}

public void testHighlightingWithStoredKeyword() throws IOException {
XContentBuilder mappings = jsonBuilder();
mappings.startObject();
mappings.startObject("type")
.startObject("properties")
.startObject("text")
.field("type", "keyword")
.field("store", true)
.endObject()
.endObject()
.endObject();
mappings.endObject();
assertAcked(prepareCreate("test")
.addMapping("type", mappings));
client().prepareIndex("test", "type", "1")
.setSource(jsonBuilder().startObject().field("text", "foo").endObject())
.get();
refresh();
SearchResponse search = client().prepareSearch().setQuery(matchQuery("text", "foo"))
.highlighter(new HighlightBuilder().field(new Field("text"))).get();
assertHighlight(search, 0, "text", 0, equalTo("<em>foo</em>"));
}

public void testHighlightingWithWildcardName() throws IOException {
// test the kibana case with * as fieldname that will try highlight all fields including meta fields
XContentBuilder mappings = jsonBuilder();
Expand Down