From 68deda6d0300eebbf5a3b3ee0db5593efbee6bfe Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Thu, 15 Jun 2017 00:33:01 +0200 Subject: [PATCH] FastVectorHighlighter should not cache the field query globally (#25197) This commit removes the global caching of the field query and replaces it with a caching per field. Each field can use a different `highlight_query` and the rewriting of some queries (prefix, automaton, ...) depends on the targeted field so the query used for highlighting must be unique per field. There might be a small performance penalty when highlighting multiple fields since the query needs to be rewritten once per highlighted field with this change. Fixes #25171 --- .../highlight/FastVectorHighlighter.java | 48 +++++++++--------- .../test/search.highlight/20_fvh.yml | 49 +++++++++++++++++++ 2 files changed, 72 insertions(+), 25 deletions(-) create mode 100644 rest-api-spec/src/main/resources/rest-api-spec/test/search.highlight/20_fvh.yml diff --git a/core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/FastVectorHighlighter.java b/core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/FastVectorHighlighter.java index c08eea2e588..22895807af6 100644 --- a/core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/FastVectorHighlighter.java +++ b/core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/FastVectorHighlighter.java @@ -87,29 +87,6 @@ public class FastVectorHighlighter implements Highlighter { HighlighterEntry cache = (HighlighterEntry) hitContext.cache().get(CACHE_KEY); try { - FieldQuery fieldQuery; - if (field.fieldOptions().requireFieldMatch()) { - if (cache.fieldMatchFieldQuery == null) { - /* - * we use top level reader to rewrite the query against all readers, - * with use caching it across hits (and across readers...) - */ - cache.fieldMatchFieldQuery = new CustomFieldQuery(highlighterContext.query, - hitContext.topLevelReader(), true, field.fieldOptions().requireFieldMatch()); - } - fieldQuery = cache.fieldMatchFieldQuery; - } else { - if (cache.noFieldMatchFieldQuery == null) { - /* - * we use top level reader to rewrite the query against all readers, - * with use caching it across hits (and across readers...) - */ - cache.noFieldMatchFieldQuery = new CustomFieldQuery(highlighterContext.query, - hitContext.topLevelReader(), true, field.fieldOptions().requireFieldMatch()); - } - fieldQuery = cache.noFieldMatchFieldQuery; - } - MapperHighlightEntry entry = cache.mappers.get(mapper); if (entry == null) { FragListBuilder fragListBuilder; @@ -151,6 +128,21 @@ public class FastVectorHighlighter implements Highlighter { } fragmentsBuilder.setDiscreteMultiValueHighlighting(termVectorMultiValue); entry = new MapperHighlightEntry(); + if (field.fieldOptions().requireFieldMatch()) { + /** + * we use top level reader to rewrite the query against all readers, + * with use caching it across hits (and across readers...) + */ + entry.fieldMatchFieldQuery = new CustomFieldQuery(highlighterContext.query, + hitContext.topLevelReader(), true, field.fieldOptions().requireFieldMatch()); + } else { + /** + * we use top level reader to rewrite the query against all readers, + * with use caching it across hits (and across readers...) + */ + entry.noFieldMatchFieldQuery = new CustomFieldQuery(highlighterContext.query, + hitContext.topLevelReader(), true, field.fieldOptions().requireFieldMatch()); + } entry.fragListBuilder = fragListBuilder; entry.fragmentsBuilder = fragmentsBuilder; if (cache.fvh == null) { @@ -162,6 +154,12 @@ public class FastVectorHighlighter implements Highlighter { CustomFieldQuery.highlightFilters.set(field.fieldOptions().highlightFilter()); cache.mappers.put(mapper, entry); } + final FieldQuery fieldQuery; + if (field.fieldOptions().requireFieldMatch()) { + fieldQuery = entry.fieldMatchFieldQuery; + } else { + fieldQuery = entry.noFieldMatchFieldQuery; + } cache.fvh.setPhraseLimit(field.fieldOptions().phraseLimit()); String[] fragments; @@ -249,12 +247,12 @@ public class FastVectorHighlighter implements Highlighter { private class MapperHighlightEntry { public FragListBuilder fragListBuilder; public FragmentsBuilder fragmentsBuilder; + public FieldQuery noFieldMatchFieldQuery; + public FieldQuery fieldMatchFieldQuery; } private class HighlighterEntry { public org.apache.lucene.search.vectorhighlight.FastVectorHighlighter fvh; - public FieldQuery noFieldMatchFieldQuery; - public FieldQuery fieldMatchFieldQuery; public Map mappers = new HashMap<>(); } } diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.highlight/20_fvh.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.highlight/20_fvh.yml new file mode 100644 index 00000000000..d4cb980a05c --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.highlight/20_fvh.yml @@ -0,0 +1,49 @@ +setup: + - do: + indices.create: + index: test + body: + mappings: + doc: + "properties": + "title": + "type": "text" + "term_vector": "with_positions_offsets" + "description": + "type": "text" + "term_vector": "with_positions_offsets" + - do: + index: + index: test + type: doc + id: 1 + body: + "title" : "The quick brown fox is brown" + "description" : "The quick pink panther is pink" + - do: + indices.refresh: {} + +--- +"Highlight query": + - skip: + version: " - 5.5.99" + reason: bug fixed in 5.6 + - do: + search: + body: + highlight: + type: fvh + fields: + description: + type: fvh + highlight_query: + prefix: + description: br + title: + type: fvh + highlight_query: + prefix: + title: br + + - match: {hits.hits.0.highlight.title.0: "The quick brown fox is brown"} + - is_false: hits.hits.0.highlight.description