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

LUCENE-9427: Ensure unified highlighter considers all terms in fuzzy query. #1671

Closed
wants to merge 2 commits into from

Conversation

jtibshirani
Copy link
Member

This fixes a regression where if a fuzzy query corresponds to an exact match
(for example it has maxEdits: 0), then the unified highlighter doesn't produce
highlights for the matching terms.

The proposed fix is to always pass back an automaton when visiting a fuzzy
query. This seems to match the previous behavior before the regression was
introduced.

… query.

This fixes a regression where if a fuzzy query corresponds to an exact match
(for example it has maxEdits: 0), then the unified highlighter doesn't produce
highlights for the matching terms.

The proposed fix is to always pass back an automaton when visiting a fuzzy
query. This seems to match the previous behavior before the regression was
introduced.
@jtibshirani
Copy link
Member Author

It looks like this behavior changed during the QueryVisitor refactor: https://github.com/apache/lucene-solr/pull/581/files#diff-449ea2758bc22783ca3f819096e1b638R97. Previously we always extracted an automaton from fuzzy queries.

@jtibshirani jtibshirani changed the title [LUCENE-9427] Ensure unified highlighter considers all terms in fuzzy query. LUCENE-9427: Ensure unified highlighter considers all terms in fuzzy query. Jul 15, 2020
@jpountz jpountz requested a review from romseygeek July 16, 2020 17:13
@romseygeek
Copy link
Contributor

Related: https://issues.apache.org/jira/browse/LUCENE-9274

I think ideally the fix would be https://issues.apache.org/jira/browse/LUCENE-9277, but this works as a stopgap.

@jtibshirani
Copy link
Member Author

Thanks @romseygeek for taking a look. What do you see as the next steps? Would it make sense to merge this, and I could look into LUCENE-9277 in a follow-up?

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @jtibshirani . Can you add a CHANGES.txt entry, and then I'll merge it in.

@jtibshirani
Copy link
Member Author

I just pushed a changelog update.

@romseygeek
Copy link
Contributor

Merged to master in 688583f

@romseygeek romseygeek closed this Aug 6, 2020
@jtibshirani jtibshirani deleted the highlight-fuzzy-query branch August 6, 2020 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants