Lucene - Core
  1. Lucene - Core
  2. LUCENE-2272

PayloadNearQuery has hardwired explanation for 'AveragePayloadFunction'

    Details

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

      Description

      The 'explain' method in PayloadNearSpanScorer assumes the AveragePayloadFunction was used. This patch adds the 'explain' method to the 'PayloadFunction' interface, where the Scorer can call it. Added unit tests for 'explain' and for

      {Min,Max}

      PayloadFunction.

      1. payloadfunctin-patch.txt
        17 kB
        Peter Keegan
      2. PNQ-patch.txt
        10 kB
        Peter Keegan
      3. PNQ-patch1.txt
        44 kB
        Peter Keegan
      4. PNQ-patch2.txt
        16 kB
        Peter Keegan

        Activity

        Hide
        Peter Keegan added a comment -

        This patch adds the 'explain' method to the 'PayloadFunction' interface, where the Scorer can call it. Added unit tests for 'explain' and for

        {Min,Max}

        PayloadFunction.

        Show
        Peter Keegan added a comment - This patch adds the 'explain' method to the 'PayloadFunction' interface, where the Scorer can call it. Added unit tests for 'explain' and for {Min,Max} PayloadFunction.
        Hide
        Grant Ingersoll added a comment -

        Peter,

        Couple of comments:

        • The base explain method can't be abstract. Something like:
          public Explanation explain(int docId){
              Explanation result = new Explanation();
              result.setDescription("Unimpl Payload Function Explain");
              result.setValue(1);
              return result;
            };
          

          should do the trick

        • The changes don't seem thread safe any more since there are now member variables. It may still be all right, but have you looked at this aspect?
        Show
        Grant Ingersoll added a comment - Peter, Couple of comments: The base explain method can't be abstract. Something like: public Explanation explain( int docId){ Explanation result = new Explanation(); result.setDescription( "Unimpl Payload Function Explain" ); result.setValue(1); return result; }; should do the trick The changes don't seem thread safe any more since there are now member variables. It may still be all right, but have you looked at this aspect?
        Hide
        Peter Keegan added a comment -

        There is a bug in PayloadNearQuery. If there are multiple top level spans that match the query, only the payloads of the first one are retrieved. This patch fixes this bug by iterating over all the top level spans to get the payloads (see 'setFreqCurrentDoc')

        > The base explain method can't be abstract. Something like
        Ah, right. This is included in the patch

        >The changes don't seem thread safe any more since there are now member variables. It may still be all right, but have you looked at this aspect?

        I guess that could be said about PayloadTermSpanScorer and PayloadNearSpanScorer, too (payloadScore, payloadsSeen). As for the PayloadFunction classes, they seem lightweight enough to be created with each query. Is there a better pattern?

        Peter

        Show
        Peter Keegan added a comment - There is a bug in PayloadNearQuery. If there are multiple top level spans that match the query, only the payloads of the first one are retrieved. This patch fixes this bug by iterating over all the top level spans to get the payloads (see 'setFreqCurrentDoc') > The base explain method can't be abstract. Something like Ah, right. This is included in the patch >The changes don't seem thread safe any more since there are now member variables. It may still be all right, but have you looked at this aspect? I guess that could be said about PayloadTermSpanScorer and PayloadNearSpanScorer, too (payloadScore, payloadsSeen). As for the PayloadFunction classes, they seem lightweight enough to be created with each query. Is there a better pattern? Peter
        Hide
        Peter Keegan added a comment -

        Revisiting this because the PayloadFunction 'explain' methods return the wrong value and aren't thread safe, as Grant points out. A new patch is attached which eliminates the member variables.

        Show
        Peter Keegan added a comment - Revisiting this because the PayloadFunction 'explain' methods return the wrong value and aren't thread safe, as Grant points out. A new patch is attached which eliminates the member variables.
        Hide
        Michael McCandless added a comment -

        Thanks Peter – this looks important to fix.

        The patch, confusingly, seems to recursively include itself! Ie I see PNQ-patch1.txt in the patch with its own diff lines. Strange.

        Also, how come your patch removes generics / @Override / @lucene.experimental, etc.?

        Show
        Michael McCandless added a comment - Thanks Peter – this looks important to fix. The patch, confusingly, seems to recursively include itself! Ie I see PNQ-patch1.txt in the patch with its own diff lines. Strange. Also, how come your patch removes generics / @Override / @lucene.experimental, etc.?
        Hide
        Peter Keegan added a comment -

        Well, this is embarrassing.

        I used Eclipse to generate the patch, and didn't exclude an existing text file in the project that already contained the patch. I have regenerated the patch against the trunk, which also restored the generics and missing annotations. Sorry for the confusion.

        I also changed my JIRA e-mail so I don't miss updates on issues sent to me vs. the java-dev list.

        Show
        Peter Keegan added a comment - Well, this is embarrassing. I used Eclipse to generate the patch, and didn't exclude an existing text file in the project that already contained the patch. I have regenerated the patch against the trunk, which also restored the generics and missing annotations. Sorry for the confusion. I also changed my JIRA e-mail so I don't miss updates on issues sent to me vs. the java-dev list.
        Hide
        Grant Ingersoll added a comment -

        Wow, weird timing, Peter. I was just looking at this today, hoping to finish it and up you put a patch.

        Show
        Grant Ingersoll added a comment - Wow, weird timing, Peter. I was just looking at this today, hoping to finish it and up you put a patch.
        Hide
        Peter Keegan added a comment -

        That is wierd! I hope you didn't spend too much time on it.

        Thanks,
        Peter

        Show
        Peter Keegan added a comment - That is wierd! I hope you didn't spend too much time on it. Thanks, Peter
        Hide
        Grant Ingersoll added a comment -

        Committed to trunk and 3.x

        Show
        Grant Ingersoll added a comment - Committed to trunk and 3.x
        Hide
        Robert Muir added a comment -

        reopening for possible 2.9.4/3.0.3 backport.

        Show
        Robert Muir added a comment - reopening for possible 2.9.4/3.0.3 backport.
        Hide
        Uwe Schindler added a comment -

        Resolving again as this issue will not be backported to 2.9/3.0 branches.

        Show
        Uwe Schindler added a comment - Resolving again as this issue will not be backported to 2.9/3.0 branches.
        Hide
        Grant Ingersoll added a comment -

        Bulk close for 3.1

        Show
        Grant Ingersoll added a comment - Bulk close for 3.1

          People

          • Assignee:
            Grant Ingersoll
            Reporter:
            Peter Keegan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development