From b177f104c63c3d6f7ae44b608e6373e45ce18758 Mon Sep 17 00:00:00 2001 From: Britta Weber Date: Mon, 12 Sep 2016 16:06:17 +0200 Subject: [PATCH] remove workaround for highlighter bug with geo queries This has been fixed in Lucene https://issues.apache.org/jira/browse/LUCENE-7293 This commit also adds the tests from #20412 --- .../subphase/highlight/CustomQueryScorer.java | 6 +-- .../highlight/HighlighterSearchIT.java | 40 +++++++++++++++++++ .../highlight/PlainHighlighterTests.java | 7 +++- 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/CustomQueryScorer.java b/core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/CustomQueryScorer.java index b62d28f8ab4ef..e223b8b2fffeb 100644 --- a/core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/CustomQueryScorer.java +++ b/core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/CustomQueryScorer.java @@ -90,11 +90,7 @@ protected void extractUnknownQuery(Query query, } protected void extract(Query query, float boost, Map terms) throws IOException { - if (query instanceof GeoPointInBBoxQuery) { - // skip all geo queries, see https://issues.apache.org/jira/browse/LUCENE-7293 and - // https://github.com/elastic/elasticsearch/issues/17537 - return; - } else if (query instanceof HasChildQueryBuilder.LateParsingQuery) { + if (query instanceof HasChildQueryBuilder.LateParsingQuery) { // skip has_child or has_parent queries, see: https://github.com/elastic/elasticsearch/issues/14999 return; } diff --git a/core/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/HighlighterSearchIT.java b/core/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/HighlighterSearchIT.java index 843ab09b2fe22..1e56db2301040 100644 --- a/core/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/HighlighterSearchIT.java +++ b/core/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/HighlighterSearchIT.java @@ -27,6 +27,7 @@ import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.support.WriteRequest; import org.elasticsearch.cluster.metadata.IndexMetaData; +import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.Settings.Builder; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -2756,6 +2757,45 @@ public void testGeoFieldHighlightingWithDifferentHighlighters() throws IOExcepti 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") + .setCorners(new GeoPoint(48.934059, 41.610741), new GeoPoint(-23.065941, 113.610741)))); + SearchResponse search = client().prepareSearch().setSource( + new SearchSourceBuilder().query(query).highlighter(new HighlightBuilder().highlighterType("plain").field("jd"))).get(); + assertNoFailures(search); + assertThat(search.getHits().totalHits(), equalTo(1L)); + } + + public void testKeywordFieldHighlighting() throws IOException { // check that keyword highlighting works XContentBuilder mappings = jsonBuilder(); diff --git a/core/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/PlainHighlighterTests.java b/core/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/PlainHighlighterTests.java index 428751e8859a8..b923c2464d831 100644 --- a/core/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/PlainHighlighterTests.java +++ b/core/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/PlainHighlighterTests.java @@ -68,8 +68,11 @@ 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 failure")); - // 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 failure")); } public void testGeoPointInBBoxQueryHighlighting() throws IOException, InvalidTokenOffsetsException {