Lucene - Core
  1. Lucene - Core
  2. LUCENE-2011

Remove deprecated Scorer.explain(int) method

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      This is the only remaining deprecation in core, but is not so easy to handle, because lot's of code in core still uses the explain() method in Scorer. So e.g. in PhraseQuery, the explain method has to be moved from Scorer to the Weight.

      1. LUCENE-2011.patch
        37 kB
        Uwe Schindler
      2. LUCENE-2011.patch
        34 kB
        Mark Miller
      3. LUCENE-2011.patch
        40 kB
        Uwe Schindler
      4. LUCENE-2011.patch
        40 kB
        Uwe Schindler
      5. LUCENE-2011.patch
        18 kB
        Mark Miller
      6. LUCENE-2011-bw.patch
        3 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Mark Miller added a comment -

          I think I've everything but the PhraseQuery issue - thats a tough one though, and extends to MultiPhraseQuery and SpanQuery.

          Show
          Mark Miller added a comment - I think I've everything but the PhraseQuery issue - thats a tough one though, and extends to MultiPhraseQuery and SpanQuery.
          Hide
          Uwe Schindler added a comment -

          I have a patch here which moves the explain part from the scorer to the weight. Almost all test pass, only a few. I can post this here, and maybe you fix the rmaining issues.

          Show
          Uwe Schindler added a comment - I have a patch here which moves the explain part from the scorer to the weight. Almost all test pass, only a few. I can post this here, and maybe you fix the rmaining issues.
          Hide
          Uwe Schindler added a comment -

          Only TestPayloadTermQuery fails with wrong explanations.

          This patch also adds lots of @Override, because only by this I can test that I do not override a no longer available method.

          Show
          Uwe Schindler added a comment - Only TestPayloadTermQuery fails with wrong explanations. This patch also adds lots of @Override, because only by this I can test that I do not override a no longer available method.
          Hide
          Mark Miller added a comment -

          cough hack cough.

          If we went this way, we should prob also hack termweight#explain too so that i can still use the cache.

          Show
          Mark Miller added a comment - cough hack cough . If we went this way, we should prob also hack termweight#explain too so that i can still use the cache.
          Hide
          Mark Miller added a comment -

          Okay, Uwe - perhaps we can merge and get something nice - I didn't do anything with contrib with my patch as well.

          Show
          Mark Miller added a comment - Okay, Uwe - perhaps we can merge and get something nice - I didn't do anything with contrib with my patch as well.
          Hide
          Mark Miller added a comment - - edited

          Okay, nice - looks like we are pretty similar - you just pulled some more cruft that I didn't and pulled the phrasequery/spanquery stuff up a level it looks. My hack was to keep them were they are, make them package private, and just cast the scorer

          I'd say lets go with your patch and just fix that test.

          Show
          Mark Miller added a comment - - edited Okay, nice - looks like we are pretty similar - you just pulled some more cruft that I didn't and pulled the phrasequery/spanquery stuff up a level it looks. My hack was to keep them were they are, make them package private, and just cast the scorer I'd say lets go with your patch and just fix that test.
          Hide
          Uwe Schindler added a comment -

          An updated patch, I added the termDocs==null case in the TermQuery explanation (you did it correctly).

          If you can somehow fix the PayloadTermQuery, I would be happy. By the way, I have to cast the scorers, too, to get the current freq value (I added a method to these scorers to get them).

          Show
          Uwe Schindler added a comment - An updated patch, I added the termDocs==null case in the TermQuery explanation (you did it correctly). If you can somehow fix the PayloadTermQuery, I would be happy. By the way, I have to cast the scorers, too, to get the current freq value (I added a method to these scorers to get them).
          Hide
          Uwe Schindler added a comment -

          cough hack cough.
          If we went this way, we should prob also hack termweight#explain too so that i can still use the cache.

          The cache was of no use for TermWeight.explain, because the explain method created a new Scorer.

          Show
          Uwe Schindler added a comment - cough hack cough . If we went this way, we should prob also hack termweight#explain too so that i can still use the cache. The cache was of no use for TermWeight.explain, because the explain method created a new Scorer.
          Hide
          Uwe Schindler added a comment -

          I'd say lets go with your patch and just fix that test.

          Is the test wrong or my code?

          Show
          Uwe Schindler added a comment - I'd say lets go with your patch and just fix that test. Is the test wrong or my code?
          Hide
          Uwe Schindler added a comment -

          I reanimated the TestNearSpan test. TestTermScorer.testExplain cannot be reanimated, because it tests the lower level tfExplanation only and it is not easy to get from the Weight.

          Contrib builds and tests without problems.

          So only one failing test

          Show
          Uwe Schindler added a comment - I reanimated the TestNearSpan test. TestTermScorer.testExplain cannot be reanimated, because it tests the lower level tfExplanation only and it is not easy to get from the Weight. Contrib builds and tests without problems. So only one failing test
          Hide
          Mark Miller added a comment - - edited

          Its because of the loss of whats going on in PayloadTermQuery - we also lose some custom info from PayloadNearQuery - on first blush it seems best just to leave the exp in the scorer for Span queries - its still gone from Scorer so I don't see it as much of a problem myself - havn't thought on it much yet though.

          I guess we could pull all that custom info up to the weight through the span/payload stuff - but then each has to duplicate a bunch of the info from the SpanWeight - it seems nicer just to leave things as they are, let the Scorer do it, and cast. What do you think?

          Show
          Mark Miller added a comment - - edited Its because of the loss of whats going on in PayloadTermQuery - we also lose some custom info from PayloadNearQuery - on first blush it seems best just to leave the exp in the scorer for Span queries - its still gone from Scorer so I don't see it as much of a problem myself - havn't thought on it much yet though. I guess we could pull all that custom info up to the weight through the span/payload stuff - but then each has to duplicate a bunch of the info from the SpanWeight - it seems nicer just to leave things as they are, let the Scorer do it, and cast. What do you think?
          Hide
          Uwe Schindler added a comment -

          Looks good. I also thought about it and I also think, whe should just leave an explain() in SpanScorer, maybe we should make it package-protected or protected, because it is no longer public.

          In my last patch I only reanimated the TestNearSpan test.

          Show
          Uwe Schindler added a comment - Looks good. I also thought about it and I also think, whe should just leave an explain() in SpanScorer, maybe we should make it package-protected or protected, because it is no longer public. In my last patch I only reanimated the TestNearSpan test.
          Hide
          Uwe Schindler added a comment -

          Added the test to your patch.

          I also made explain() in SpanScorer protected.

          Show
          Uwe Schindler added a comment - Added the test to your patch. I also made explain() in SpanScorer protected.
          Hide
          Mark Miller added a comment -

          Nice on protected - i wanted to package private it, but couldnt because of the payloads package - good call.

          Show
          Mark Miller added a comment - Nice on protected - i wanted to package private it, but couldnt because of the payloads package - good call.
          Hide
          Uwe Schindler added a comment -

          ...had the same idea - and problem.

          Show
          Uwe Schindler added a comment - ...had the same idea - and problem.
          Hide
          Uwe Schindler added a comment -

          Attached the backwards branch patch.

          Show
          Uwe Schindler added a comment - Attached the backwards branch patch.
          Hide
          Uwe Schindler added a comment -

          All tests pass, I think it is now ready to commit. Will commit in a day.

          Show
          Uwe Schindler added a comment - All tests pass, I think it is now ready to commit. Will commit in a day.
          Hide
          Uwe Schindler added a comment -

          Committed revision: 830377

          Thanks Mark!

          Show
          Uwe Schindler added a comment - Committed revision: 830377 Thanks Mark!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development