Lucene - Core
  1. Lucene - Core
  2. LUCENE-2669

NumericRangeQuery.NumericRangeTermsEnum sometimes seeks backwards

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Subclasses of FilteredTermsEnum are "supposed to" seek forwards only (this gives better performance, typically).

      However, we don't check for this, so I added an assert to do that (while digging into testing the SimpleText codec) and NumericRangeQuery trips the assert!

      Other MTQs seem not to trip it.

      I think I know what's happening – say NRQ has term ranges a-c, e-f to seek to, but then while it's .next()'ing through the first range, the first term after c is f. At this point NRQ sees the range a-c is done, and then tries to seek to term e which is before f. Maybe NRQ's accept method should detect this case (where you've accidentally .next()'d into or possibly beyond the next one or more seek ranges)?

      1. LUCENE-2669.patch
        3 kB
        Uwe Schindler
      2. LUCENE-2669.patch
        3 kB
        Uwe Schindler
      3. LUCENE-2669.patch
        1 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        Patch w/ 2 asserts. NRQ only trips up on the first (FilteredTermsEnum) assert. That it doesn't trip the 2nd shows that indeed its seek ranges are properly sorted...

        Show
        Michael McCandless added a comment - Patch w/ 2 asserts. NRQ only trips up on the first (FilteredTermsEnum) assert. That it doesn't trip the 2nd shows that indeed its seek ranges are properly sorted...
        Hide
        Robert Muir added a comment -

        This is a good catch: NRQ should play ping-pong to avoid these unnecessary seeks

        Show
        Robert Muir added a comment - This is a good catch: NRQ should play ping-pong to avoid these unnecessary seeks
        Hide
        Uwe Schindler added a comment -

        Here a patch that fixed NRTE to only seek forward. This should also improve NRQ's perf in trunk.

        It works like the following:

        1. nextSeekTerm() checks that the next range already fits the current term. If not it forwards to the next sub-range and returns a seek term that is at least greater or equal the current term
        2. accept() checks for the non-hit case (seldom as for a NRQ most terms are hits until the upper sub-range-bound is reached), if the next sub-range lower bound term on the stack is greater that the current one, and only then returns NO_AND_SEEK. If this is not the case, it does not seek but instead only move forward to the next sub-range and repeats the bounds checks [for(;;) loop].
        Show
        Uwe Schindler added a comment - Here a patch that fixed NRTE to only seek forward. This should also improve NRQ's perf in trunk. It works like the following: nextSeekTerm() checks that the next range already fits the current term. If not it forwards to the next sub-range and returns a seek term that is at least greater or equal the current term accept() checks for the non-hit case (seldom as for a NRQ most terms are hits until the upper sub-range-bound is reached), if the next sub-range lower bound term on the stack is greater that the current one, and only then returns NO_AND_SEEK. If this is not the case, it does not seek but instead only move forward to the next sub-range and repeats the bounds checks [for(;;) loop] .
        Hide
        Uwe Schindler added a comment -

        Slightly more readable patch: for(;-loop removed and so first if check in accept() negated and used as while-clause instead

        Mike you set this as fix 3.1 and 4.0, but 3.1 does not have FilteredTermsEnum. We cannot fix it there easily, as it uses the old style logic from 3.0.

        Show
        Uwe Schindler added a comment - Slightly more readable patch: for(; -loop removed and so first if check in accept() negated and used as while-clause instead Mike you set this as fix 3.1 and 4.0, but 3.1 does not have FilteredTermsEnum. We cannot fix it there easily, as it uses the old style logic from 3.0.
        Hide
        Michael McCandless added a comment -

        Mike you set this as fix 3.1 and 4.0, but 3.1 does not have FilteredTermsEnum. We cannot fix it there easily, as it uses the old style logic from 3.0.

        Woops, right – I'll fix to 4.0 only.

        Show
        Michael McCandless added a comment - Mike you set this as fix 3.1 and 4.0, but 3.1 does not have FilteredTermsEnum. We cannot fix it there easily, as it uses the old style logic from 3.0. Woops, right – I'll fix to 4.0 only.
        Hide
        Uwe Schindler added a comment -

        Mike you set this as fix 3.1 and 4.0, but 3.1 does not have FilteredTermsEnum. We cannot fix it there easily, as it uses the old style logic from 3.0.

        We can maybe fix this also in 3.0 and not fetch a new enum, when the same conditions apply. But its totally different code, will do that in a separate patch, if its easy (the 3.0/3.1 enum is complicated...)

        Show
        Uwe Schindler added a comment - Mike you set this as fix 3.1 and 4.0, but 3.1 does not have FilteredTermsEnum. We cannot fix it there easily, as it uses the old style logic from 3.0. We can maybe fix this also in 3.0 and not fetch a new enum, when the same conditions apply. But its totally different code, will do that in a separate patch, if its easy (the 3.0/3.1 enum is complicated...)
        Hide
        Uwe Schindler added a comment -

        Committed revision: 1001582

        Thanks Mike for catching this!

        Show
        Uwe Schindler added a comment - Committed revision: 1001582 Thanks Mike for catching this!
        Hide
        Michael McCandless added a comment -

        Sweet, that was fast – thanks Uwe!

        Show
        Michael McCandless added a comment - Sweet, that was fast – thanks Uwe!

          People

          • Assignee:
            Uwe Schindler
            Reporter:
            Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development