Fixed multi term queries support in postings highlighter for non top-level queries

[javanna]

In #4052 we added support for highlighting multi term queries using the postings highlighter. That worked only for top-level queries though, and not for multi term queries that are nested for instance within a bool query, or filtered query, or a constant score query.

The way we can make this work is by walking the query structure and temporarily overriding the query rewrite method with a method that allows for multi terms extraction.

Closes #5127

[jpountz]

The change looks good to me.

On a related node: should we have an option to not rewrite queries in order to ensure highlighting remains fast? cc @s1monw

[javanna]

Makes sense to me, I also plan to look at the upcoming changes in Lucene 4.7 aka faster ways to highlight multi term queries.

[roytmana]

Does it also fixes wildcard highlighting issue?

[jpountz]

@roytmana yes, this change will make wildcard queries highlighted

[roytmana]

thank you @jpountz
Looks like it is still in @javanna repository. I will test it once it has been committed to ES repo

[nik9000]

If this change would make multi term queries highlight at the speed they do with the plain highlighter then please. I'd prefer the default be the faster option with an option to do the highlighting for multi term queries. If not that then at least a prominent warning. Or something.

Sent from my iPhone

On Feb 17, 2014, at 8:55 AM, Adrien Grand notifications@github.com wrote:

The change looks good to me.

On a related node: should we have an option to not rewrite queries in order to ensure highlighting remains fast? cc @s1monw

—
Reply to this email directly or view it on GitHub.

[javanna]

Hi @nik9000 I see your point... on the other hand if one wants to highlight multi term queries that's what it takes at this time. It feels pretty bad not to return any snippet by default to keep highlighting fast. My take is that the best way to keep highlighting fast is not to highlight multi term queries, that's it. I'd add a warning to the docs that explains what we do with multi term queries and the fact that it can slow things down.

Also we might be able to improve this once lucene 4.7 gets released, as mentioned above.

[nik9000]

Sounds good to me.

Sent from my iPhone

On Feb 18, 2014, at 4:34 AM, Luca Cavanna notifications@github.com wrote:

Hi @nik9000 I see your point... on the other hand if one wants to highlight multi term queries that's what it takes at this time. It feels pretty bad not to return any snippet by default to keep highlighting fast. My take is that the best way to keep highlighting fast is not to highlight multi term queries, that's it. I'd add a warning to the docs that explains what we do with multi term queries and the fact that it can slow things down.

Also we might be able to improve this once lucene 4.7 gets released, as mentioned above.

—
Reply to this email directly or view it on GitHub.

[roytmana]

Hi @javanna, will it be merged into 1.0 branch so I can test it?

[s1monw]

this needs to be FilteredQuery rather than XFilteredQuery same goes for the XConstantScore

[javanna]

good point I think we should have both then... XFilteredQuery and XConstantScore needs to be there otherwise this doesn't work for our filtered query and our constant score query?

[s1monw]

true for XFilteredQuery used to subclass FilteredQuery but not for Constant:

public class XConstantScoreQuery extends ConstantScoreQuery {

[javanna]

Agreed

[s1monw]

ok LGTM in general while I really don't like how this works in general :(

[javanna]

Neither do I. One other option is to remove the setRewriteMethod call in any case and let users set the rewrite_method themselves as it's exposed in the query DSL. Even better, as soon as lucene 4.7 is out we can look into LUCENE-5415 and hopefully replace our current logic when it comes to multi term queries.