Lucene - Core
  1. Lucene - Core
  2. LUCENE-1941

MinPayloadFunction returns 0 when only one payload is present

    Details

    • Lucene Fields:
      New

      Description

      In some experiments with payload scoring through PayloadTermQuery, I'm seeing 0 returned when using MinPayloadFunction. I believe there is a bug there. No time at the moment to flesh out a unit test, but wanted to report it for tracking.

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

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment -

          Hi Erik,

          I want to release 2.9.2 and 3.0.1, is there any problem?
          I would change this to fix 3.1 only, else it should be fix for 3.0.1 and 2.9.2 both.

          Show
          Uwe Schindler added a comment - Hi Erik, I want to release 2.9.2 and 3.0.1, is there any problem? I would change this to fix 3.1 only, else it should be fix for 3.0.1 and 2.9.2 both.
          Hide
          Michael McCandless added a comment -

          This is a real bug – it happens because PayloadTermQuery (and PayloadNearQuery) processPayload calls the payload scoring function, passing in the old score and the new one.

          The problem is the old score always defaults to 0.0. Ie it doesn't handle the first payload properly – first payload should set the value for min/max. So you won't hit this if your min is < 0.

          MaxPayloadFunction has the bug as well, but you won't hit it if your max is > 0.

          Show
          Michael McCandless added a comment - This is a real bug – it happens because PayloadTermQuery (and PayloadNearQuery) processPayload calls the payload scoring function, passing in the old score and the new one. The problem is the old score always defaults to 0.0. Ie it doesn't handle the first payload properly – first payload should set the value for min/max. So you won't hit this if your min is < 0. MaxPayloadFunction has the bug as well, but you won't hit it if your max is > 0.
          Hide
          Erik Hatcher added a comment -

          Feel free to adjust this issue to whichever Lucene version makes sense. I don't have bandwidth at the moment to address this myself.

          Show
          Erik Hatcher added a comment - Feel free to adjust this issue to whichever Lucene version makes sense. I don't have bandwidth at the moment to address this myself.
          Hide
          Michael McCandless added a comment -

          Attached patch.

          The fix is simple – the payload function already receives the count of how many payloads were seen so far, so when this count is 0, min/max function should just return the current value.

          We still need a test case though... I'm probably not going to have time (off on "vacation" (scare quotes for Marvin) soon).

          Show
          Michael McCandless added a comment - Attached patch. The fix is simple – the payload function already receives the count of how many payloads were seen so far, so when this count is 0, min/max function should just return the current value. We still need a test case though... I'm probably not going to have time (off on "vacation" (scare quotes for Marvin) soon).
          Hide
          Michael McCandless added a comment -

          New patch attached – last patch had unrelated changes to the build xml files.

          Show
          Michael McCandless added a comment - New patch attached – last patch had unrelated changes to the build xml files.
          Hide
          Marvin Humphrey added a comment -

          > off on "vacation" (scare quotes for Marvin)

          Have "fun"!

          Show
          Marvin Humphrey added a comment - > off on "vacation" (scare quotes for Marvin) Have "fun"!
          Hide
          Uwe Schindler added a comment -

          As there is no real test available (for the whole class exspect ctor, Mark Miller figured out yesterday) - I think the attached fix is ok at the moment and i would like to apply it to 2.9, 3.0 and trunk to release the pending 2.9.2 and 3.0.1.

          If nobody is against it (Erik?) i would like to apply this patch and release the artifacts for PMC vote today afternoon. Also I open a new issue requesting tests at all

          Show
          Uwe Schindler added a comment - As there is no real test available (for the whole class exspect ctor, Mark Miller figured out yesterday) - I think the attached fix is ok at the moment and i would like to apply it to 2.9, 3.0 and trunk to release the pending 2.9.2 and 3.0.1. If nobody is against it (Erik?) i would like to apply this patch and release the artifacts for PMC vote today afternoon. Also I open a new issue requesting tests at all
          Hide
          Erik Hatcher added a comment -

          Uwe - patch looks good. Go for it!

          Show
          Erik Hatcher added a comment - Uwe - patch looks good. Go for it!
          Hide
          Uwe Schindler added a comment - - edited

          I created LUCENE-2264 for the tests.

          I will now proceed with applying the patches and merging to 2.9/3.0.

          Show
          Uwe Schindler added a comment - - edited I created LUCENE-2264 for the tests. I will now proceed with applying the patches and merging to 2.9/3.0.
          Hide
          Uwe Schindler added a comment -

          Patch with CHANGES.txt in the new 3.0.1/2.9.2 section of restructured trunk changes.

          Show
          Uwe Schindler added a comment - Patch with CHANGES.txt in the new 3.0.1/2.9.2 section of restructured trunk changes.
          Hide
          Uwe Schindler added a comment -

          Committed trunk revision: 910034
          Committed 3.0 branch revision: 910037
          Committed 2.9 branch revision: 910038

          Show
          Uwe Schindler added a comment - Committed trunk revision: 910034 Committed 3.0 branch revision: 910037 Committed 2.9 branch revision: 910038

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development