Solr
  1. Solr
  2. SOLR-553

Highlighter does not match phrase queries correctly

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2
    • Fix Version/s: 1.3
    • Component/s: highlighter
    • Labels:
      None
    • Environment:

      all

      Description

      http://www.nabble.com/highlighting-pt2%3A-returning-tokens-out-of-order-from-PhraseQuery-to16156718.html

      Say we search for the band "I Love You But I've Chosen Darkness"
      .../selectrows=100&q=%22I%20Love%20You%20But%20I\'ve%20Chosen%20Darkness%22&fq=type:html&hl=true&hl.fl=content&hl.fragsize=500&hl.snippets=5&hl.simple.pre=%3Cspan%3E&hl.simple.post=%3C/span%3E

      The highlight returns a snippet that does have the name altogether:

      Lights (Live) : <span>I</span> <span>Love</span> <span>You</span> But <span>I've</span> <span>Chosen</span> <span>Darkness</span> :

      But also returns unrelated snips from the same page:

      Black Francis Shop "<span>I</span> Think <span>I</span> <span>Love</span> <span>You</span>"

      A correct highlighter should not return snippets that do not match the phrase exactly.

      LUCENE-794 (not yet committed, but seems to be ready) fixes up the problem from the Lucene end. Solr should get it too.

      Related: SOLR-575

      1. highlighttest.xml
        2 kB
        Brian Whitman
      2. Solr-553.patch
        11 kB
        Otis Gospodnetic
      3. Solr-553.patch
        10 kB
        Bojan Smid
      4. Solr-553.patch
        7 kB
        Bojan Smid
      5. SOLR-553-SC.patch
        4 kB
        Grant Ingersoll

        Issue Links

          Activity

          Hide
          Mike Klaas added a comment -

          Changed to feature request, since the current behaviour is expected. I'd be happy to review a patch to use SpanScorer in Solr, though.

          Show
          Mike Klaas added a comment - Changed to feature request, since the current behaviour is expected. I'd be happy to review a patch to use SpanScorer in Solr, though.
          Hide
          Brian Whitman added a comment -

          Attaching a base test case document xml to post to the trunk solr example to see the problem.

          Steps to reproduce:
          1) checkout solr-trunk
          2) ant example
          3) java -jar start.jar
          4) post.sh highlighttest.xml
          5) query: http://localhost:8983/solr/select?q=features:%22ax%20bx%20cx%22&hl=on&hl.fl=features&hl.fragsize=20&hl.snippets=10

          Expected results: the only highlight snip results returned should be <em>ax bx cx</em> and nothing else.

          Show
          Brian Whitman added a comment - Attaching a base test case document xml to post to the trunk solr example to see the problem. Steps to reproduce: 1) checkout solr-trunk 2) ant example 3) java -jar start.jar 4) post.sh highlighttest.xml 5) query: http://localhost:8983/solr/select?q=features:%22ax%20bx%20cx%22&hl=on&hl.fl=features&hl.fragsize=20&hl.snippets=10 Expected results: the only highlight snip results returned should be <em>ax bx cx</em> and nothing else.
          Hide
          Bojan Smid added a comment - - edited

          I am playing around with LUCENE-794 integration into Solr. I have two options:

          1) add LUCENE-794 code to current implementation in DefaultSolrHighlighter where client would provide request parameter (say useSpanScorer) if he wants to use new functionality. In case he didn't provide the parameter, he would get old functionality.

          or

          2) to provide LUCENE-794 highlighting in new SolrHighlighter, for instance in class PhraseQuerySolrHighlighter

          I would appreciate any comments on this.

          Also, since I already test some of this code, I noticed that we still wouldn't get exact behavior from description. For instance, in text ax bx cx dx ax bx

          for phrase query "ax bx cx"

          the result is : <span>ax</span><span>bx</span><span>cx</span> dx ax bx

          Which means that we got a fix for part of the problem (words from unrelated snippets are no longer highlighted), but we still wouldn't get whole phrase highlighted inside single tag.

          Show
          Bojan Smid added a comment - - edited I am playing around with LUCENE-794 integration into Solr. I have two options: 1) add LUCENE-794 code to current implementation in DefaultSolrHighlighter where client would provide request parameter (say useSpanScorer) if he wants to use new functionality. In case he didn't provide the parameter, he would get old functionality. or 2) to provide LUCENE-794 highlighting in new SolrHighlighter, for instance in class PhraseQuerySolrHighlighter I would appreciate any comments on this. Also, since I already test some of this code, I noticed that we still wouldn't get exact behavior from description. For instance, in text ax bx cx dx ax bx for phrase query "ax bx cx" the result is : <span>ax</span><span>bx</span><span>cx</span> dx ax bx Which means that we got a fix for part of the problem (words from unrelated snippets are no longer highlighted), but we still wouldn't get whole phrase highlighted inside single tag.
          Hide
          Brian Whitman added a comment -

          Probably best to create a new ticket (if necessary) about the <span>ax</span> <span>bx</span> instead of <span>ax bx</span> problem. That highlights have incorrect matches is far worse. I'll adjust the problem description.

          Show
          Brian Whitman added a comment - Probably best to create a new ticket (if necessary) about the <span>ax</span> <span>bx</span> instead of <span>ax bx</span> problem. That highlights have incorrect matches is far worse. I'll adjust the problem description.
          Hide
          Bojan Smid added a comment -

          I made a fix, patch is uploaded. LUCENE-794 is now incorporated into default Solr highlighter.

          Old way of highlighting is still retained and will be used in case requests to Solr Highlighter remain the same as they were (same request parameters). New functionality is invoked by adding another request parameter to URL, hl.usePhraseHighlighter=true.

          So, for URL:
          http://localhost:8983/solr/select?q=features:%22ax%20bx%20cx%22&hl=on&hl.fl=features&hl.fragsize=20&hl.snippets=10

          results will be the same as they were, but in case you want to use this fix (and have correct phrase highlighting), the URL would look like this:

          http://localhost:8983/solr/select?q=features:%22ax%20bx%20cx%22&hl=on&hl.fl=features&hl.fragsize=20&hl.snippets=10&hl.usePhraseHighlighter=true

          This patch needs latest lucene-highlighter-.jar and lucene-memory-.jar from trunk (since LUCENE-794 fix is committed there).

          Show
          Bojan Smid added a comment - I made a fix, patch is uploaded. LUCENE-794 is now incorporated into default Solr highlighter. Old way of highlighting is still retained and will be used in case requests to Solr Highlighter remain the same as they were (same request parameters). New functionality is invoked by adding another request parameter to URL, hl.usePhraseHighlighter=true. So, for URL: http://localhost:8983/solr/select?q=features:%22ax%20bx%20cx%22&hl=on&hl.fl=features&hl.fragsize=20&hl.snippets=10 results will be the same as they were, but in case you want to use this fix (and have correct phrase highlighting), the URL would look like this: http://localhost:8983/solr/select?q=features:%22ax%20bx%20cx%22&hl=on&hl.fl=features&hl.fragsize=20&hl.snippets=10&hl.usePhraseHighlighter=true This patch needs latest lucene-highlighter- .jar and lucene-memory- .jar from trunk (since LUCENE-794 fix is committed there).
          Hide
          Bojan Smid added a comment -

          Patch for Solr-553 (uses Lucene-794 highlighting fix)

          Show
          Bojan Smid added a comment - Patch for Solr-553 (uses Lucene-794 highlighting fix)
          Hide
          Brian Whitman added a comment -

          Patch works for me on the highlighttest.xml. thanks Bojan!!

          Show
          Brian Whitman added a comment - Patch works for me on the highlighttest.xml. thanks Bojan!!
          Hide
          Mike Klaas added a comment -

          What do people think of making span highlighting the default behaviour if the query contains phrases? It might be better to have the default behaviour that which people expect, even if it is technically different output from 1.2.

          Show
          Mike Klaas added a comment - What do people think of making span highlighting the default behaviour if the query contains phrases? It might be better to have the default behaviour that which people expect, even if it is technically different output from 1.2.
          Hide
          Brian Whitman added a comment -

          +1 on making it default if there was a phrasequery. The "old" way comes across as a bad bug if you're displaying the highlights for your search results.

          Show
          Brian Whitman added a comment - +1 on making it default if there was a phrasequery. The "old" way comes across as a bad bug if you're displaying the highlights for your search results.
          Hide
          Otis Gospodnetic added a comment -

          +1 for making it the default - it makes more sense than the old HL that highlighted other matching tokens
          that were not a part of the given phrase.

          Show
          Otis Gospodnetic added a comment - +1 for making it the default - it makes more sense than the old HL that highlighted other matching tokens that were not a part of the given phrase.
          Hide
          Bojan Smid added a comment -

          Added unit test for this fix to the patch.

          Show
          Bojan Smid added a comment - Added unit test for this fix to the patch.
          Hide
          Otis Gospodnetic added a comment -

          Added explicit check for usePhraseHighlighter=true to avoid things like usePhraseHighlighter=false to turn it on.

          I'll commit shortly, along with a fresh lucene-highlighter-2.4-dev.jar built from from Lucene trunk.

          Show
          Otis Gospodnetic added a comment - Added explicit check for usePhraseHighlighter=true to avoid things like usePhraseHighlighter=false to turn it on. I'll commit shortly, along with a fresh lucene-highlighter-2.4-dev.jar built from from Lucene trunk.
          Hide
          Brian Whitman added a comment -

          just FYI, I've tested this on a much larger/realworld index and it works great.

          Show
          Brian Whitman added a comment - just FYI, I've tested this on a much larger/realworld index and it works great.
          Hide
          Mike Klaas added a comment -

          [quote]Added explicit check for usePhraseHighlighter=true to avoid things like usePhraseHighlighter=false to turn it on.[/quote]

          I'm not sure I follow you here. Just to verify:

          • the default is to use SpanScorer when the query is a "pure" phrase query
          • you can force SS with usePhraseHighlighting
          • queries that are mixed queries with keywords and phrases are still problematic.

          If this is correct, is there any point in the usePhraseHighlighter parameter? I don't see where it would entail different behaviour. Also, what are the consequences for dismax queries (pure or mixed)?

          Show
          Mike Klaas added a comment - [quote] Added explicit check for usePhraseHighlighter=true to avoid things like usePhraseHighlighter=false to turn it on. [/quote] I'm not sure I follow you here. Just to verify: the default is to use SpanScorer when the query is a "pure" phrase query you can force SS with usePhraseHighlighting queries that are mixed queries with keywords and phrases are still problematic. If this is correct, is there any point in the usePhraseHighlighter parameter? I don't see where it would entail different behaviour. Also, what are the consequences for dismax queries (pure or mixed)?
          Hide
          Otis Gospodnetic added a comment - - edited

          I think there are no pure vs. mixed situation any more. If usePH=true we use SpanScorer otherwise we use QueryScorer, or at least that's how I read the patch.

          DefaultSolrHighlighter.java:295-304
                    if (Boolean.valueOf(req.getParams().get(HighlightParams.USE_PHRASE_HIGHLIGHTER))) {
                      // wrap CachingTokenFilter around TokenStream for reuse
                      tstream = new CachingTokenFilter(tstream);
                      
                      // get highlighter
                      highlighter = getPhraseHighlighter(query, fieldName, req, (CachingTokenFilter) tstream);
                      
                      // after highlighter initialization, reset tstream since construction of highlighter already used it
                      tstream.reset();
                    }
                    else {
                      // use "the old way"
                      highlighter = getHighlighter(query, fieldName, req);
                    }
          
          Show
          Otis Gospodnetic added a comment - - edited I think there are no pure vs. mixed situation any more. If usePH=true we use SpanScorer otherwise we use QueryScorer, or at least that's how I read the patch. DefaultSolrHighlighter.java:295-304 if ( Boolean .valueOf(req.getParams().get(HighlightParams.USE_PHRASE_HIGHLIGHTER))) { // wrap CachingTokenFilter around TokenStream for reuse tstream = new CachingTokenFilter(tstream); // get highlighter highlighter = getPhraseHighlighter(query, fieldName, req, (CachingTokenFilter) tstream); // after highlighter initialization, reset tstream since construction of highlighter already used it tstream.reset(); } else { // use "the old way" highlighter = getHighlighter(query, fieldName, req); }
          Hide
          Otis Gospodnetic added a comment -

          I'll wait for LUCENE-1285 to get committed first.

          Show
          Otis Gospodnetic added a comment - I'll wait for LUCENE-1285 to get committed first.
          Hide
          Mark Miller added a comment -

          >Probably best to create a new ticket (if necessary) about the <span>ax</span> <span>bx</span> instead of <span>ax bx</span> problem. That >highlights have incorrect matches is far worse. I'll adjust the problem description.

          If I remember correctly, this was an ease of implementation issue. Part of it was fitting into the current Highlighter framework (individual tokens are scored and highlighted) and part of it was ease in general I think. I am not sure that it would be too easy to alter.

          It's very easy to do with the new Highlighter I have been working on, the LargeDocHighlighter. It breaks from the current API, and makes this type of highlight markup quite easy. It may never see the light of day though...to do what I want, all parts of the query need to be located with the MemoryIndex, and the time this takes on non position sensitive queries clauses is almost equal to the savings I get from not iterating through and scoring each token in a TokenStream. I do still have hopes I can pull something off though, and it may end up being useful for something else.

          For now though, Highlighting each each token seems a small inconvenience to retain all the old Highlighters tests, corner cases, and speed in non position sensitive scoring. Thats not to say there will not be a way if you take a look at the code though.

          Show
          Mark Miller added a comment - >Probably best to create a new ticket (if necessary) about the <span>ax</span> <span>bx</span> instead of <span>ax bx</span> problem. That >highlights have incorrect matches is far worse. I'll adjust the problem description. If I remember correctly, this was an ease of implementation issue. Part of it was fitting into the current Highlighter framework (individual tokens are scored and highlighted) and part of it was ease in general I think. I am not sure that it would be too easy to alter. It's very easy to do with the new Highlighter I have been working on, the LargeDocHighlighter. It breaks from the current API, and makes this type of highlight markup quite easy. It may never see the light of day though...to do what I want, all parts of the query need to be located with the MemoryIndex, and the time this takes on non position sensitive queries clauses is almost equal to the savings I get from not iterating through and scoring each token in a TokenStream. I do still have hopes I can pull something off though, and it may end up being useful for something else. For now though, Highlighting each each token seems a small inconvenience to retain all the old Highlighters tests, corner cases, and speed in non position sensitive scoring. Thats not to say there will not be a way if you take a look at the code though.
          Hide
          Otis Gospodnetic added a comment -

          Thanks Bojan.

          Committed revision 659664.

          Show
          Otis Gospodnetic added a comment - Thanks Bojan. Committed revision 659664.
          Hide
          Grant Ingersoll added a comment -

          Needs new Lucene jars, per the earlier comments. Build is currently broken.

          Show
          Grant Ingersoll added a comment - Needs new Lucene jars, per the earlier comments. Build is currently broken.
          Hide
          Grant Ingersoll added a comment -

          Otis,

          Here's a patch that fixes the Spell Checker test that gets broken when you upgrade the Lucene jars.

          Show
          Grant Ingersoll added a comment - Otis, Here's a patch that fixes the Spell Checker test that gets broken when you upgrade the Lucene jars.
          Hide
          Mark Miller added a comment -

          Just to point out, as I am not sure its clear, the SpanScorer is just as fast as the old Scorer when no Phrase's, or Span's are in the query. Mark H actually tested it as slightly faster, though thats a bit odd.

          When there is a Span or Phrase, none Span/Phrase clauses of the Query are still highlighted the same and at the same speed as the original Scorer...it is just the Span/Phrase clauses that fire up a MemoryIndex and have getSpans called against it.

          So you really only pay for the extra position sensitive part where actually needed.

          Show
          Mark Miller added a comment - Just to point out, as I am not sure its clear, the SpanScorer is just as fast as the old Scorer when no Phrase's, or Span's are in the query. Mark H actually tested it as slightly faster, though thats a bit odd. When there is a Span or Phrase, none Span/Phrase clauses of the Query are still highlighted the same and at the same speed as the original Scorer...it is just the Span/Phrase clauses that fire up a MemoryIndex and have getSpans called against it. So you really only pay for the extra position sensitive part where actually needed.
          Hide
          Grant Ingersoll added a comment -

          I committed the new JARs and fixed the SpellChecker test

          Show
          Grant Ingersoll added a comment - I committed the new JARs and fixed the SpellChecker test
          Hide
          Otis Gospodnetic added a comment -

          If I understood Mark correctly, he is saying we can just have usePhraseHighlighter=true
          be the default and it won't hurt performance. Should we do that, and allow one to get the
          old behaviour with usePhraseHighlighter=false if they really prefer the old highlighting?

          Show
          Otis Gospodnetic added a comment - If I understood Mark correctly, he is saying we can just have usePhraseHighlighter=true be the default and it won't hurt performance. Should we do that, and allow one to get the old behaviour with usePhraseHighlighter=false if they really prefer the old highlighting?

            People

            • Assignee:
              Grant Ingersoll
              Reporter:
              Brian Whitman
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development