skip GeoPointMultiTermQuery when highlighting #20412
Conversation
We skip GeoPointInBBoxQuery already but not when it was rewritten it appears as GeoPointInBBoxQuery and needs to be skipped as well. see elastic#17537
|
@jimferenczi I am not sure if #20400 fixes this, it would still try to rewrite that inner geo query though? |
|
|
||
| static { | ||
| try { | ||
| unsupportedGeoQuery = Class.forName("org.apache.lucene.spatial.geopoint.search.GeoPointMultiTermQuery"); |
There was a problem hiding this comment.
can you add a comment that this class in pkg private and that's why we are making this stunt?
|
LGTM |
s1monw
added
v2.4.1
>bug
review
:Search Relevance/Highlighting
@s1monw true, but we could call |
I did that before I commented here :) |
|
@jimferenczi It seems like for this query this fixes it. But I still think it is better to check for the particular geo query when we highlight. Functionscore is just one of many and I think we should rather catch this problem for all cases even though the class check is rather ugly. |
he he, thanks !
Not so many but I get your point. Maybe we should avoid rewrite completely ? I don't see where it could be beneficial for the highlighting. I may be completely wrong but is there examples where the rewrite solves an issue with highlighting. It seems to cause more harm than good. |
Hm, that will be tricky as queries are rewritten in org.apache.lucene.search.highlight.WeightedSpanTermExtractor.extract() so that is rather a Lucene thing? I do not know the code well enough though to know if that can be easily avoided. |
|
@s1monw added a comment. Thanks for the review! |
This has been fixed in Lucene https://issues.apache.org/jira/browse/LUCENE-7293 This commit also adds the tests from elastic#20412
This has been fixed in Lucene https://issues.apache.org/jira/browse/LUCENE-7293 This commit also adds the tests from #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
This has been fixed in Lucene https://issues.apache.org/jira/browse/LUCENE-7293 This commit also adds the tests from #20412
This has been fixed in Lucene https://issues.apache.org/jira/browse/LUCENE-7293 This commit also adds the tests from #20412
We skip GeoPointInBBoxQuery already but not when it was rewritten
it appears as GeoPointMultiTermQuery and needs to be skipped as well.
see #17537
This pull request is against the 2.4 branch and a follow up to #17537 (comment) . Unfortunately my assumption that the highlighter never gets to see a rewritten query (see #18495 (comment)) was wrong and so in some cases such as the example provided by @ajayar it does. We need to check for that as well and skip the highlighting in this case.
The tests in this pull request do not reproduce the issue #17537 on the 2.4 branch for me and I believe this is due to an unrelated change in Lucene. I still think we should merge this to 2.4 anyway to be on the safe side. On 2.3 the tests reproduce the issue. In addition the discussion on #17537 is not finished so this pull request doe not close the issue yet.
On master the problem was fixed in Lucene (see https://issues.apache.org/jira/browse/LUCENE-7293) so we can remove the check in CustomQueryScorer completely. I will open a separate pull request for that.