skip GeoPointMultiTermQuery when highlighting (#20412)

* skip GeoPointMultiTermQuery when highlighting We skip GeoPointInBBoxQuery already but not when it was rewritten it appears as GeoPointInBBoxQuery and needs to be skipped as well. see #17537
elastic · Sep 13, 2016 · 723cc22 · 723cc22
1 parent e55da51
commit 723cc22
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 5 deletions.
diff --git a/core/src/main/java/org/elasticsearch/search/highlight/CustomQueryScorer.java b/core/src/main/java/org/elasticsearch/search/highlight/CustomQueryScorer.java
@@ -33,6 +33,19 @@
 
 public final class CustomQueryScorer extends QueryScorer {
 
+    private static final Class<?> unsupportedGeoQuery;
+
+    static {
+        try {
+            // in extract() we need to check for GeoPointMultiTermQuery and skip extraction for queries that inherit from it.
+            // But GeoPointMultiTermQuerythat is package private in Lucene hence we cannot use an instanceof check. This is why
+            // we use this rather ugly workaround to get a Class and later be able to compare with isAssignableFrom().
+            unsupportedGeoQuery = Class.forName("org.apache.lucene.spatial.geopoint.search.GeoPointMultiTermQuery");
+        } catch (ClassNotFoundException e) {
+            throw new AssertionError(e);
+        }
+    }
+
     public CustomQueryScorer(Query query, IndexReader reader, String field,
                              String defaultField) {
         super(query, reader, field, defaultField);
@@ -89,9 +102,12 @@ protected void extractUnknownQuery(Query query,
         }
 
         protected void extract(Query query, float boost, Map<String, WeightedSpanTerm> terms) throws IOException {
-            // skip all geo queries, see https://issues.apache.org/jira/browse/LUCENE-7293 and
-            // https://github.com/elastic/elasticsearch/issues/17537
-            if (query instanceof GeoPointInBBoxQuery == false) {
+
+            if (query instanceof GeoPointInBBoxQuery || unsupportedGeoQuery.isAssignableFrom(query.getClass())) {
+                // skip all geo queries, see https://issues.apache.org/jira/browse/LUCENE-7293 and
+                // https://github.com/elastic/elasticsearch/issues/17537
+                return;
+            } else {
                 super.extract(query, boost, terms);
             }
         }

diff --git a/core/src/test/java/org/elasticsearch/search/highlight/HighlighterSearchIT.java b/core/src/test/java/org/elasticsearch/search/highlight/HighlighterSearchIT.java
@@ -2736,4 +2736,43 @@ public void testGeoFieldHighlightingWithDifferentHighlighters() throws IOExcepti
         assertThat(search.getHits().totalHits(), equalTo(1L));
         assertThat(search.getHits().getAt(0).highlightFields().get("text").fragments().length, equalTo(1));
     }
+
+    public void testGeoFieldHighlightingWhenQueryGetsRewritten() throws IOException {
+        // same as above but in this example the query gets rewritten during highlighting
+        // see https://github.com/elastic/elasticsearch/issues/17537#issuecomment-244939633
+        XContentBuilder mappings = jsonBuilder();
+        mappings.startObject();
+        mappings.startObject("jobs")
+            .startObject("_all")
+            .field("enabled", false)
+            .endObject()
+            .startObject("properties")
+            .startObject("loc")
+            .field("type", "geo_point")
+            .endObject()
+            .startObject("jd")
+            .field("type", "string")
+            .endObject()
+            .endObject()
+            .endObject();
+        mappings.endObject();
+        assertAcked(prepareCreate("test")
+            .addMapping("jobs", mappings));
+        ensureYellow();
+
+        client().prepareIndex("test", "jobs", "1")
+            .setSource(jsonBuilder().startObject().field("jd", "some आवश्यकता है- आर्य समाज अनाथालय, 68 सिविल लाइन्स, बरेली को एक पुरूष रस text")
+                .field("loc", "12.934059,77.610741").endObject())
+            .get();
+        refresh();
+
+        QueryBuilder query = QueryBuilders.functionScoreQuery(QueryBuilders.boolQuery().filter(QueryBuilders.geoBoundingBoxQuery("loc")
+            .bottomRight(-23.065941, 113.610741)
+            .topLeft(48.934059, 41.610741)));
+        SearchResponse search = client().prepareSearch().setSource(
+            new SearchSourceBuilder().query(query)
+                .highlight(new HighlightBuilder().highlighterType("plain").field("jd")).buildAsBytes()).get();
+        assertNoFailures(search);
+        assertThat(search.getHits().totalHits(), equalTo(1L));
+    }
 }
diff --git a/core/src/test/java/org/elasticsearch/search/highlight/PlainHighlighterTests.java b/core/src/test/java/org/elasticsearch/search/highlight/PlainHighlighterTests.java
@@ -67,8 +67,12 @@ public void checkGeoQueryHighlighting(Query geoQuery) throws IOException, Invali
         String fragment = highlighter.getBestFragment(fieldNameAnalyzer.tokenStream("text", "Arbitrary text field which should not cause " +
             "a failure"), "Arbitrary text field which should not cause a failure");
         assertThat(fragment, equalTo("Arbitrary text field which should not cause a <B>failure</B>"));
-        // TODO: This test will fail if we pass in an instance of GeoPointInBBoxQueryImpl too. Should we also find a way to work around that
-        // or can the query not be rewritten before it is passed into the highlighter?
+        Query rewritten = boolQuery.rewrite(null);
+        highlighter =
+            new org.apache.lucene.search.highlight.Highlighter(new CustomQueryScorer(rewritten));
+        fragment = highlighter.getBestFragment(fieldNameAnalyzer.tokenStream("text", "Arbitrary text field which should not cause " +
+            "a failure"), "Arbitrary text field which should not cause a failure");
+        assertThat(fragment, equalTo("Arbitrary text field which should not cause a <B>failure</B>"));
     }
 
     public void testGeoPointInBBoxQueryHighlighting() throws IOException, InvalidTokenOffsetsException {