Lucene - Core
  1. Lucene - Core
  2. LUCENE-3087

highlighting exact phrase with overlapping tokens fails.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.9.4, 3.1
    • Fix Version/s: 3.2, 4.0-ALPHA
    • Component/s: modules/highlighter
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Fields with overlapping token are not highlighted in search results when searching exact phrases, when using TermVector.WITH_OFFSET.

      The document builded in MemoryIndex for highlight does not preserve positions of tokens in this case. Overlapping tokens get "flattened" (position increment always set to 1), the spanquery used for searching relevant fragment will fail to identify the correct token sequence because the position shift.

      I corrected this by adding a position increment calculation in sub class StoredTokenStream. I added junit test covering this case.

      I used the eclipse codestyle from trunk, but style add quite a few format differences between repository and working copy files. I tried to reduce them, but some linewrapping rules still doesn't match.

      Correction patch joined

      1. LUCENE-3087.patch
        10 kB
        Pierre Gossé

        Activity

        Hide
        Pierre Gossé added a comment -

        correction patch with junit tests

        Show
        Pierre Gossé added a comment - correction patch with junit tests
        Hide
        Michael McCandless added a comment -

        Thank you for the patch with test case Pierre!

        So this bug only applies if you store offset but not positions in your term vectors (TermVector.WITH_OFFSET). Previously we would always set posIncr=1, now (with your patch) we set it to 0 if the offset didn't change vs the prior token. I think this seems reasonable – we have to fallback on heuristics since positions were not stored.

        Show
        Michael McCandless added a comment - Thank you for the patch with test case Pierre! So this bug only applies if you store offset but not positions in your term vectors (TermVector.WITH_OFFSET). Previously we would always set posIncr=1, now (with your patch) we set it to 0 if the offset didn't change vs the prior token. I think this seems reasonable – we have to fallback on heuristics since positions were not stored.
        Hide
        Pierre Gossé added a comment - - edited

        Thanks for taking a look at this Michael.

        In fact, I should be in the case of TermVector.WITH_POSITIONS_OFFSETS, using this parameters in my solr Shema.xml
        <field name="..." type="..." indexed="true" stored="true" compressed="true" omitNorms="true" termVectors="true" termPositions="true" termOffsets="true"/>

        Somehow, I end up in TokenSources with argument tokenPositionsGuaranteedContiguous to false, which falls back to using offsets instead of positions.

        Maybe this is because of my overlapping tokens, maybe not, I'll have to take a couple of hours sometime to figure this out. At first sight, however it seams this parameter is always set to false when calling TokenSource.getTokenStream with an IndexReader because some code to use field infos is missing.

        Some work to do here, maybe, sometime.

        Show
        Pierre Gossé added a comment - - edited Thanks for taking a look at this Michael. In fact, I should be in the case of TermVector.WITH_POSITIONS_OFFSETS, using this parameters in my solr Shema.xml <field name="..." type="..." indexed="true" stored="true" compressed="true" omitNorms="true" termVectors="true" termPositions="true" termOffsets="true"/> Somehow, I end up in TokenSources with argument tokenPositionsGuaranteedContiguous to false, which falls back to using offsets instead of positions. Maybe this is because of my overlapping tokens, maybe not, I'll have to take a couple of hours sometime to figure this out. At first sight, however it seams this parameter is always set to false when calling TokenSource.getTokenStream with an IndexReader because some code to use field infos is missing. Some work to do here, maybe, sometime.
        Hide
        Michael McCandless added a comment -

        Ahh, interesting.

        So... maybe we should fix the code so that if in fact positions were included in the TVs, we use them? Else, we fallback to the offset check to guess at the posIncr? Could that work?

        Show
        Michael McCandless added a comment - Ahh, interesting. So... maybe we should fix the code so that if in fact positions were included in the TVs, we use them? Else, we fallback to the offset check to guess at the posIncr? Could that work?
        Hide
        Pierre Gossé added a comment - - edited

        Yes, that would be the best.

        But I'm not sure how to do that :

        • Check for positions in token stream ? Not sure it "guaranties" anything.
        • add some kind of additionnal properties to the TermFreqVector returned by the IndexReader.getTermFreqVector() since it already access fields info ? Not sure it has'nt too much impact.
        • Ask the index for field infos from TokenSources.getTokenStream ? Not sure it is the place but looks like the less dangerous option.

        I haven't much time 'till the end of month to take a serious look at this, but I'll try to take some time next month.

        Show
        Pierre Gossé added a comment - - edited Yes, that would be the best. But I'm not sure how to do that : Check for positions in token stream ? Not sure it "guaranties" anything. add some kind of additionnal properties to the TermFreqVector returned by the IndexReader.getTermFreqVector() since it already access fields info ? Not sure it has'nt too much impact. Ask the index for field infos from TokenSources.getTokenStream ? Not sure it is the place but looks like the less dangerous option. I haven't much time 'till the end of month to take a serious look at this, but I'll try to take some time next month.
        Hide
        Robert Muir added a comment -

        So... maybe we should fix the code so that if in fact positions were included in the TVs, we use them? Else, we fallback to the offset check to guess at the posIncr? Could that work?

        But this patch is still good right? We introduce this heuristic when positions are not available (or when highlighter pretends they are not).

        From my very vague understanding of highlighting, when overlapping positions or gaps in the position increment exist (tokenPositionsGuaranteedContiguous=false), the highlighter uses this algorithm intentionally (there are comments in the code indicating this position-based algorithm would fail otherwise).

        We could try to improve that on a followup issue?

        Show
        Robert Muir added a comment - So... maybe we should fix the code so that if in fact positions were included in the TVs, we use them? Else, we fallback to the offset check to guess at the posIncr? Could that work? But this patch is still good right? We introduce this heuristic when positions are not available (or when highlighter pretends they are not). From my very vague understanding of highlighting, when overlapping positions or gaps in the position increment exist (tokenPositionsGuaranteedContiguous=false), the highlighter uses this algorithm intentionally (there are comments in the code indicating this position-based algorithm would fail otherwise). We could try to improve that on a followup issue?
        Hide
        Michael McCandless added a comment -

        We could try to improve that on a followup issue?

        +1, I agree: progress not perfection!

        So I'll commit this patch and then open a follow on issue...

        Thanks Pierre!

        Show
        Michael McCandless added a comment - We could try to improve that on a followup issue? +1, I agree: progress not perfection! So I'll commit this patch and then open a follow on issue... Thanks Pierre!
        Hide
        Michael McCandless added a comment -

        OK I opened LUCENE-3091.

        Show
        Michael McCandless added a comment - OK I opened LUCENE-3091 .
        Hide
        Robert Muir added a comment -

        Bulk closing for 3.2

        Show
        Robert Muir added a comment - Bulk closing for 3.2
        Hide
        vishal parekh added a comment -

        i am facing same problem in solr 3.5

        Show
        vishal parekh added a comment - i am facing same problem in solr 3.5

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Pierre Gossé
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development