Lucene - Core
  1. Lucene - Core
  2. LUCENE-1583

SpanOrQuery 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, 2.4.1
    • Fix Version/s: 2.9
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      In SpanOrQuery the skipTo() method is improperly implemented if the target doc is less than or equal to the current doc, since skipTo() may not be called for any of the clauses' spans:

      public boolean skipTo(int target) throws IOException {
      if (queue == null)

      { return initSpanQueue(target); }

      while (queue.size() != 0 && top().doc() < target) {
      if (top().skipTo(target)) { queue.adjustTop(); } else { queue.pop(); }
      }

      return queue.size() != 0;
      }

      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:

      public boolean skipTo(int target) throws IOException {
      if (queue == null) { return initSpanQueue(target); }

      boolean skipCalled = false;
      while (queue.size() != 0 && top().doc() < target) {
      if (top().skipTo(target))

      { queue.adjustTop(); }

      else

      { queue.pop(); }

      skipCalled = true;
      }

      if (skipCalled)

      { return queue.size() != 0; }

      return next();
      }

      1. LUCENE-1583.patch
        2 kB
        Mark Miller
      2. LUCENE-1583.patch
        0.9 kB
        Mark Miller

        Activity

        Hide
        Mark Miller added a comment -

        Thanks Moti!

        Show
        Mark Miller added a comment - Thanks Moti!
        Hide
        Mark Miller added a comment -

        Adds a Unit test and Changes entry

        Show
        Mark Miller added a comment - Adds a Unit test and Changes entry
        Hide
        Mark Miller added a comment -

        I'm going to commit this soon.

        Show
        Mark Miller added a comment - I'm going to commit this soon.
        Hide
        Mark Miller added a comment -

        That change would always call next once before skipping though right? And the way the patch is, you would only call next if no skipping occurred? I know its semantically the same, and I guess the clarity is probably worth any (probably extremely minor) slowdown?

        Show
        Mark Miller added a comment - That change would always call next once before skipping though right? And the way the patch is, you would only call next if no skipping occurred? I know its semantically the same, and I guess the clarity is probably worth any (probably extremely minor) slowdown?
        Hide
        Paul Elschot added a comment -

        Midnight here, still reading

        The patch looks good. However, it might be easier to understand by starting like this after the queue initialisation:

        if (!next()) return false;
        

        and leave the rest of the code as it is.

        I did not run any tests on this, so a thinking cap might well save time.

        Show
        Paul Elschot added a comment - Midnight here, still reading The patch looks good. However, it might be easier to understand by starting like this after the queue initialisation: if (!next()) return false ; and leave the rest of the code as it is. I did not run any tests on this, so a thinking cap might well save time.
        Hide
        Mark Miller added a comment -

        I guess I'll do this one.

        You out there reading Paul Elschot? This look right to you? Any issues it might cause?

        Else I guess I'll have to put on my thinking cap and figure it myself.

        Show
        Mark Miller added a comment - I guess I'll do this one. You out there reading Paul Elschot? This look right to you? Any issues it might cause? Else I guess I'll have to put on my thinking cap and figure it myself.
        Hide
        Mark Miller added a comment -

        I havn't looked closely at it yet, but tests appear to pass (unknown if tests actually hit skipto on SpanOrQuery though)

        Show
        Mark Miller added a comment - I havn't looked closely at it yet, but tests appear to pass (unknown if tests actually hit skipto on SpanOrQuery though)
        Hide
        Michael McCandless added a comment -

        LUCENE-1327 was a similar issue.

        Show
        Michael McCandless added a comment - LUCENE-1327 was a similar issue.

          People

          • Assignee:
            Mark Miller
            Reporter:
            Moti Nisenson
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development