Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip all geo point queries in plain highlighter #18495

Merged
merged 1 commit into from May 25, 2016

Conversation

Projects
None yet
5 participants
@brwe
Copy link
Contributor

commented May 20, 2016

Geo queries and plain highlighter do not seem to work well
together (https://issues.apache.org/jira/browse/LUCENE-7293)
so we need to skip all geo related queries when we highlight.

hopefully closes #17537

Only plain highlighter seems to be affected.
I am worried that we might need to take care of GeoPointInBBoxQueryImpl too although I do not see how a rewritten query can be used for highlighting, see TODO below.

@nik9000 since this was your idea, maybe you want to take a look?

skip all geo point queries in plain highlighter
Geo queries and plain highlighter do not seem to work well
together (https://issues.apache.org/jira/browse/LUCENE-7293)
so we need to skip all geo related queries when we highlight.

closes #17537
@nik9000

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

LGTM. Highlighter is all hacks so another one won't hurt. Much.

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) {

This comment has been minimized.

Copy link
@mikemccand

mikemccand May 20, 2016

Contributor

Do we also expose GeoPointDistanceQuery or GeoPointInPolygonQuery?

This comment has been minimized.

Copy link
@mikemccand

mikemccand May 20, 2016

Contributor

Sorry, nevermind: both of those queries also subclass GeoPoinInBBoxQuery, so this check is sufficient!

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

I left one comment, else LGTM. Thanks @brwe!

@s1monw

This comment has been minimized.

Copy link
Contributor

commented May 23, 2016

LGTM too

@brwe brwe merged commit 6862c48 into elastic:master May 25, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@clintongormley clintongormley changed the title skip all geo point queries in plain highlighter Skip all geo point queries in plain highlighter May 25, 2016

@brwe

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2016

Backported to 2.x here d2d670d and here 13c3dd8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.