Merge pull request #18495 from brwe/geo-query-highlight-II

skip all geo point queries in plain highlighter

core/src/main/java/org/elasticsearch/search/highlight/CustomQueryScorer.java

@@ -24,6 +24,7 @@
 import org.apache.lucene.search.highlight.QueryScorer;
 import org.apache.lucene.search.highlight.WeightedSpanTerm;
 import org.apache.lucene.search.highlight.WeightedSpanTermExtractor;
+import org.apache.lucene.spatial.geopoint.search.GeoPointInBBoxQuery;
 import org.elasticsearch.common.lucene.search.function.FiltersFunctionScoreQuery;
 import org.elasticsearch.common.lucene.search.function.FunctionScoreQuery;
 
@@ -87,6 +88,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) {
+                super.extract(query, boost, terms);
+            }
+        }
     }
-
 }

core/src/test/java/org/elasticsearch/search/highlight/HighlighterSearchIT.java

@@ -2553,16 +2553,22 @@ private void phraseBoostTestCase(String highlighterType) {
         assertHighlight(response, 0, "field1", 0, 1, highlightedMatcher);
     }
 
-    public void testGeoFieldHighlighting() throws IOException {
+    public void testGeoFieldHighlightingWithDifferentHighlighters() throws IOException {
         // check that we do not get an exception for geo_point fields in case someone tries to highlight
-        // it accidential with a wildcard
+        // it accidentially with a wildcard
         // see https://github.com/elastic/elasticsearch/issues/17537
         XContentBuilder mappings = jsonBuilder();
         mappings.startObject();
         mappings.startObject("type")
             .startObject("properties")
             .startObject("geo_point")
             .field("type", "geo_point")
+            .field("geohash", true)
+            .endObject()
+            .startObject("text")
+            .field("type", "text")
+            .field("term_vector", "with_positions_offsets_payloads")
+            .field("index_options", "offsets")
             .endObject()
             .endObject()
             .endObject();
@@ -2572,14 +2578,19 @@ public void testGeoFieldHighlighting() throws IOException {
         ensureYellow();
 
         client().prepareIndex("test", "type", "1")
-            .setSource(jsonBuilder().startObject().field("geo_point", "60.12,100.34").endObject())
+            .setSource(jsonBuilder().startObject().field("text", "Arbitrary text field which will should not cause a failure").endObject())
             .get();
         refresh();
+        String highlighterType = randomFrom("plain", "fvh", "postings");
+        QueryBuilder query = QueryBuilders.boolQuery().should(QueryBuilders.geoBoundingBoxQuery("geo_point")
+            .setCorners(61.10078883158897, -170.15625, -64.92354174306496, 118.47656249999999))
+            .should(QueryBuilders.termQuery("text", "failure"));
         SearchResponse search = client().prepareSearch().setSource(
-            new SearchSourceBuilder().query(QueryBuilders.geoBoundingBoxQuery("geo_point").setCorners(61.10078883158897, -170.15625,
-                -64.92354174306496, 118.47656249999999)).highlighter(new HighlightBuilder().field("*"))).get();
+            new SearchSourceBuilder().query(query)
+                .highlighter(new HighlightBuilder().field("*").highlighterType(highlighterType))).get();
         assertNoFailures(search);
         assertThat(search.getHits().totalHits(), equalTo(1L));
+        assertThat(search.getHits().getAt(0).highlightFields().get("text").fragments().length, equalTo(1));
     }
 
     public void testKeywordFieldHighlighting() throws IOException {

core/src/test/java/org/elasticsearch/search/highlight/PlainHighlighterTests.java

@@ -19,12 +19,28 @@
 
 package org.elasticsearch.search.highlight;
 
+import org.apache.lucene.analysis.Analyzer;
 import org.apache.lucene.analysis.MockAnalyzer;
+import org.apache.lucene.analysis.standard.StandardAnalyzer;
 import org.apache.lucene.index.Term;
+import org.apache.lucene.search.BooleanClause;
+import org.apache.lucene.search.BooleanQuery;
 import org.apache.lucene.search.PhraseQuery;
 import org.apache.lucene.search.Query;
+import org.apache.lucene.search.TermQuery;
+import org.apache.lucene.search.highlight.InvalidTokenOffsetsException;
 import org.apache.lucene.search.highlight.QueryScorer;
+import org.apache.lucene.spatial.geopoint.search.GeoPointDistanceQuery;
+import org.apache.lucene.spatial.geopoint.search.GeoPointInBBoxQuery;
+import org.apache.lucene.spatial.geopoint.search.GeoPointInPolygonQuery;
 import org.apache.lucene.util.LuceneTestCase;
+import org.elasticsearch.index.analysis.FieldNameAnalyzer;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.hamcrest.Matchers.equalTo;
 
 public class PlainHighlighterTests extends LuceneTestCase {
 
@@ -39,4 +55,36 @@ public void testHighlightPhrase() throws Exception {
         assertArrayEquals(new String[] {"bar <B>foo</B> <B>bar</B> foo"}, frags);
     }
 
+    public void checkGeoQueryHighlighting(Query geoQuery) throws IOException, InvalidTokenOffsetsException {
+        Map analysers = new HashMap<String, Analyzer>();
+        analysers.put("text", new StandardAnalyzer());
+        FieldNameAnalyzer fieldNameAnalyzer = new FieldNameAnalyzer(analysers);
+        Query termQuery = new TermQuery(new Term("text", "failure"));
+        Query boolQuery = new BooleanQuery.Builder().add(new BooleanClause(geoQuery, BooleanClause.Occur.SHOULD))
+            .add(new BooleanClause(termQuery, BooleanClause.Occur.SHOULD)).build();
+        org.apache.lucene.search.highlight.Highlighter highlighter =
+            new org.apache.lucene.search.highlight.Highlighter(new CustomQueryScorer(boolQuery));
+        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?
+    }
+
+    public void testGeoPointInBBoxQueryHighlighting() throws IOException, InvalidTokenOffsetsException {
+        Query geoQuery = new GeoPointDistanceQuery("geo_point", -64.92354174306496, -170.15625, 5576757);
+        checkGeoQueryHighlighting(geoQuery);
+    }
+
+    public void testGeoPointDistanceQueryHighlighting() throws IOException, InvalidTokenOffsetsException {
+        Query geoQuery = new GeoPointInBBoxQuery("geo_point", -64.92354174306496, 61.10078883158897, -170.15625, 118.47656249999999);
+        checkGeoQueryHighlighting(geoQuery);
+    }
+
+    public void testGeoPointInPolygonQueryHighlighting() throws IOException, InvalidTokenOffsetsException {
+        double[] polyLats = new double[]{0, 60, 0, 0};
+        double[] polyLons = new double[]{0, 60, 90, 0};
+        Query geoQuery = new GeoPointInPolygonQuery("geo_point", polyLats, polyLons);
+        checkGeoQueryHighlighting(geoQuery);
+    }
 }