Lucene - Core
  1. Lucene - Core
  2. LUCENE-1327

TermSpans skipTo() doesn't always move forwards

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9, 2.0.0, 2.1, 2.2, 2.3, 2.3.1, 2.3.2, 2.4
    • Fix Version/s: 2.9
    • Labels:
      None

      Description

      In TermSpans (or the anonymous Spans class returned by SpansTermQuery, depending on the version), the skipTo() method is improperly implemented if the target doc is less than or equal to the current doc:

      public boolean skipTo(int target) throws IOException {
      // are we already at the correct position?
      if (doc >= target)

      { return true; }

      ...

      This violates the correct behavior (as described in the Spans interface documentation), that skipTo() should always move forwards, in other words the correct implementation would be:

      if (doc >= target)

      { return next(); }

      This bug causes particular problems if one wants to use the payloads feature - this is because if one loads a payload, then performs a skipTo() to the same document, then tries to load the "next" payload, the spans hasn't changed position and it attempts to load the same payload again (which is an error).

      1. lucene-1327.patch
        3 kB
        Michael Busch

        Issue Links

          Activity

          Hide
          Mark Miller added a comment -

          I swear I saw this discussion in another issue...this a dupe?

          In any case, if you use the isPayloadAvailable method before trying to get the payload, that should help.

          Show
          Mark Miller added a comment - I swear I saw this discussion in another issue...this a dupe? In any case, if you use the isPayloadAvailable method before trying to get the payload, that should help.
          Hide
          Michael McCandless added a comment -

          This is indeed a bug in that the implementation does not match the javadocs. Mark or Grant, what should we do here?

          Show
          Michael McCandless added a comment - This is indeed a bug in that the implementation does not match the javadocs. Mark or Grant, what should we do here?
          Hide
          Mark Miller added a comment -

          I found the issue this one dupes. It looks to me like relaxing the spec is the most likely fix. I don't know if that solves this guys problem though - is calling isPayloadAvailable good enough, or is his logic screwed up as a result...

          Show
          Mark Miller added a comment - I found the issue this one dupes. It looks to me like relaxing the spec is the most likely fix. I don't know if that solves this guys problem though - is calling isPayloadAvailable good enough, or is his logic screwed up as a result...
          Hide
          Mark Miller added a comment -

          Not an exact dupe by the way (spans vs scorer), but same issue, and from Dougs comment, should probably be handled in a similar manner (whatever is decided).

          Show
          Mark Miller added a comment - Not an exact dupe by the way (spans vs scorer), but same issue, and from Dougs comment, should probably be handled in a similar manner (whatever is decided).
          Hide
          Michael McCandless added a comment -

          We're still iterating on the approach to resolve this, and LUCENE-914, so I'm taking this out of 2.4.

          Show
          Michael McCandless added a comment - We're still iterating on the approach to resolve this, and LUCENE-914 , so I'm taking this out of 2.4.
          Hide
          Michael Busch added a comment -

          I think the right fix here is to simply remove these lines:

          // are we already at the correct position?
          if (doc >= target) { return true; }
          

          Then TermSpans#skipTo() basically returns what the TermPositions#skipTo() call returns.
          TermPositions#skipTo() has the same semantics as described in the javadocs of Spans#skipTo().

          The patch I'm attaching contains a junit that fails before applying the fix, and passes after applying. All other tests pass too.

          Any objections here? Otherwise I'll commit in a day or two.

          Show
          Michael Busch added a comment - I think the right fix here is to simply remove these lines: // are we already at the correct position? if (doc >= target) { return true ; } Then TermSpans#skipTo() basically returns what the TermPositions#skipTo() call returns. TermPositions#skipTo() has the same semantics as described in the javadocs of Spans#skipTo(). The patch I'm attaching contains a junit that fails before applying the fix, and passes after applying. All other tests pass too. Any objections here? Otherwise I'll commit in a day or two.
          Hide
          Michael Busch added a comment -

          Committed revision 756669.

          Show
          Michael Busch added a comment - Committed revision 756669.

            People

            • Assignee:
              Michael Busch
              Reporter:
              Moti Nisenson
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development