Lucene - Core
  1. Lucene - Core
  2. LUCENE-944

Remove deprecated methods in BooleanQuery

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      Patch Available

      Description

      Remove deprecated methods setUseScorer14 and getUseScorer14 in BooleanQuery, and adapt javadocs.

      1. BooleanQuery20070626.patch
        2 kB
        Paul Elschot
      2. lucene-944.patch
        15 kB
        Michael Busch
      3. lucene-944.patch
        15 kB
        Michael Busch
      4. lucene-944.patch
        11 kB
        Michael Busch
      5. lucene-944-bw.patch
        13 kB
        Michael Busch

        Issue Links

          Activity

          Hide
          Paul Elschot added a comment -

          Patches BooleanQuery only. Tests pass, javadocs build.

          Show
          Paul Elschot added a comment - Patches BooleanQuery only. Tests pass, javadocs build.
          Hide
          Michael Busch added a comment -

          Committed. Thanks, Paul!

          Show
          Michael Busch added a comment - Committed. Thanks, Paul!
          Hide
          Grant Ingersoll added a comment -

          Hmm, I was just trying to compile Luke source against the latest Lucene and am getting compiler errors on this being missing. Doesn't this violate our back-compatibility clause or did I miss something? Don't we have to wait until 3.0-dev to remove these?

          Show
          Grant Ingersoll added a comment - Hmm, I was just trying to compile Luke source against the latest Lucene and am getting compiler errors on this being missing. Doesn't this violate our back-compatibility clause or did I miss something? Don't we have to wait until 3.0-dev to remove these?
          Hide
          Michael Busch added a comment -

          You are right, Grant.

          I will revert this for 2.3. Thanks for catching this!!

          Show
          Michael Busch added a comment - You are right, Grant. I will revert this for 2.3. Thanks for catching this!!
          Hide
          Michael Busch added a comment - - edited

          Some test fail with this patch when a MatchAllDocQuery is used, e.g. TestSimpleExplanations.testMA1. skipTo() tries to get the delete bit for a negative document, which throws an Exception of course. This happens for a searcher that contains a MultiReader created by QueryUtils.wrapUnderlyingReader().

          I assume this is related to the change I made in QueryUtils. I need to spend some more time to figure out what exactly leads to this exception. Maybe I'm doing something silly here - I'm still catching up on some changes, such as the out-of-order scoring changes.

          I can't work on this till Monday, so anyone familiar with these tests feel free to take a look!

          Show
          Michael Busch added a comment - - edited Some test fail with this patch when a MatchAllDocQuery is used, e.g. TestSimpleExplanations.testMA1. skipTo() tries to get the delete bit for a negative document, which throws an Exception of course. This happens for a searcher that contains a MultiReader created by QueryUtils.wrapUnderlyingReader(). I assume this is related to the change I made in QueryUtils. I need to spend some more time to figure out what exactly leads to this exception. Maybe I'm doing something silly here - I'm still catching up on some changes, such as the out-of-order scoring changes. I can't work on this till Monday, so anyone familiar with these tests feel free to take a look!
          Hide
          Mark Miller added a comment -

          Hoss added some multireader test stuff that counts on where the deletes are if I remember right.

          so

          -    if (BooleanQuery.getAllowDocsOutOfOrder()) return;  // in this case order of skipTo() might differ from that of next().
          +    if (q.weight(s).scoresDocsOutOfOrder()) return;  // in this case order of skipTo() might differ from that of next().
          

          must not be exactly equiv and where it wasn't scoring out of order for this test, now it is, or vice versa (I havn't looked closely).

          Show
          Mark Miller added a comment - Hoss added some multireader test stuff that counts on where the deletes are if I remember right. so - if (BooleanQuery.getAllowDocsOutOfOrder()) return ; // in this case order of skipTo() might differ from that of next(). + if (q.weight(s).scoresDocsOutOfOrder()) return ; // in this case order of skipTo() might differ from that of next(). must not be exactly equiv and where it wasn't scoring out of order for this test, now it is, or vice versa (I havn't looked closely).
          Hide
          Michael Busch added a comment -

          Yeah I figured it has to do with that, but it seems weird to me that a call to scorer.advance() (in QueryUtils, line 348) leads to a ArrayIndexOutOfBoundsException?

          Show
          Michael Busch added a comment - Yeah I figured it has to do with that, but it seems weird to me that a call to scorer.advance() (in QueryUtils, line 348) leads to a ArrayIndexOutOfBoundsException?
          Hide
          Mark Miller added a comment -

          So this was always there - but in the past, because BoolQuery.getAllowOutOfOrder was always returning true for these tests, it didn't get into the code thats now failing.

          With the replacement, because it only returns true or false based on if its a booleanquery, it gets through there for other queries, and we are seeing the results.

          Show
          Mark Miller added a comment - So this was always there - but in the past, because BoolQuery.getAllowOutOfOrder was always returning true for these tests, it didn't get into the code thats now failing. With the replacement, because it only returns true or false based on if its a booleanquery, it gets through there for other queries, and we are seeing the results.
          Hide
          Mark Miller added a comment -

          but it seems weird to me that a call to scorer.advance() (in QueryUtils, line 348) leads to a ArrayIndexOutOfBoundsException?

          We had similar issues when we were working on these tests before. Hoss knows how the MultiReader/delete stuff is better than I do though - he wrote it, I just addressed issues/bugs.

          Show
          Mark Miller added a comment - but it seems weird to me that a call to scorer.advance() (in QueryUtils, line 348) leads to a ArrayIndexOutOfBoundsException? We had similar issues when we were working on these tests before. Hoss knows how the MultiReader/delete stuff is better than I do though - he wrote it, I just addressed issues/bugs.
          Hide
          Uwe Schindler added a comment -

          When we change this, don't forget to remove the 2.9 BW tests that may fail because of this change - whenever we remove deprecated methods, the new 2.9 BW branch must be updated.

          Show
          Uwe Schindler added a comment - When we change this, don't forget to remove the 2.9 BW tests that may fail because of this change - whenever we remove deprecated methods, the new 2.9 BW branch must be updated.
          Hide
          Mark Miller added a comment -

          Part of it or all of it is that the tests that are failing are using external ids, which doesn't work with this multireader stuff (since they are tech illegal, though they worked before per segment). We fixed a bunch of these, but missed these ones, since they didn't hit these tests. They need to be fixed in the same manner.

          Show
          Mark Miller added a comment - Part of it or all of it is that the tests that are failing are using external ids, which doesn't work with this multireader stuff (since they are tech illegal, though they worked before per segment). We fixed a bunch of these, but missed these ones, since they didn't hit these tests. They need to be fixed in the same manner.
          Hide
          Mark Miller added a comment -

          A second part is that there looks to be some insane fieldcache usage that needs to be fixed in these tests - again, because the code wasn't hit before, we didn't see it before ...

          I just keep randomly taking peeks

          Show
          Mark Miller added a comment - A second part is that there looks to be some insane fieldcache usage that needs to be fixed in these tests - again, because the code wasn't hit before, we didn't see it before ... I just keep randomly taking peeks
          Hide
          Michael Busch added a comment -

          So this was always there - but in the past, because BoolQuery.getAllowOutOfOrder was always returning true for these tests, it didn't get into the code thats now failing.

          Exactly.

          Now I just hope all bugs are in the tests...
          Sorry, have to leave now. Thanks for your help, Mark!

          Show
          Michael Busch added a comment - So this was always there - but in the past, because BoolQuery.getAllowOutOfOrder was always returning true for these tests, it didn't get into the code thats now failing. Exactly. Now I just hope all bugs are in the tests... Sorry, have to leave now. Thanks for your help, Mark!
          Hide
          Mark Miller added a comment -

          Now I just hope all bugs are in the tests...

          I'm pretty sure they are (though obviously I cant gaurantee). Its all stuff Hoss and I saw before and fixed from what I can tell - we just fixed what was causing failures though, so getting past that line is exposing stuff we didn't handle.

          Show
          Mark Miller added a comment - Now I just hope all bugs are in the tests... I'm pretty sure they are (though obviously I cant gaurantee). Its all stuff Hoss and I saw before and fixed from what I can tell - we just fixed what was causing failures though, so getting past that line is exposing stuff we didn't handle.
          Hide
          Michael Busch added a comment -

          Hey Mark, any interest in taking this, since you seem to have experience with fixing those tests?

          Otherwise I'll continue here later tonight or tomorrow... you'd probably be faster though

          Show
          Michael Busch added a comment - Hey Mark, any interest in taking this, since you seem to have experience with fixing those tests? Otherwise I'll continue here later tonight or tomorrow... you'd probably be faster though
          Hide
          Michael Busch added a comment -

          All core & contrib tests pass now.

          The problem was QueryUtils#checkSkipTo(). It still performed the checks using a global scorer, which resulted in failures in the FieldCacheSanityChecker. (the same values were cached twice: once for the global reader plus for the per-segment readers).

          QueryUtils#checkFirstSkipTo() was already implemented correctly so that it performs the checks with per-segment scorers.

          I think this is ready to commit now. Mark, do you want to take a look at my checkSkipTo() changes before I commit?

          Show
          Michael Busch added a comment - All core & contrib tests pass now. The problem was QueryUtils#checkSkipTo(). It still performed the checks using a global scorer, which resulted in failures in the FieldCacheSanityChecker. (the same values were cached twice: once for the global reader plus for the per-segment readers). QueryUtils#checkFirstSkipTo() was already implemented correctly so that it performs the checks with per-segment scorers. I think this is ready to commit now. Mark, do you want to take a look at my checkSkipTo() changes before I commit?
          Hide
          Michael Busch added a comment -

          Tiny change in QueryUtils#checkSkipTo() to keep it more consistent to how it worked before.

          Also attaching the back-compat patch. Note that I have to make the change to checkSkipTo() there too, because it was not changed before to do the search per-segment. Now more tests actually run this check, exposing this problem.

          All tests pass now.

          Show
          Michael Busch added a comment - Tiny change in QueryUtils#checkSkipTo() to keep it more consistent to how it worked before. Also attaching the back-compat patch. Note that I have to make the change to checkSkipTo() there too, because it was not changed before to do the search per-segment. Now more tests actually run this check, exposing this problem. All tests pass now.
          Hide
          Michael Busch added a comment -

          Committed revision 824870.

          Show
          Michael Busch added a comment - Committed revision 824870.

            People

            • Assignee:
              Michael Busch
              Reporter:
              Paul Elschot
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development