Lucene - Core
  1. Lucene - Core
  2. LUCENE-4297

BooleanScorer2 sometimes multiplies coord() twice into the score

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0, 3.6.2, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      this is a problem if you have a custom coord impl

      1. LUCENE-4297_test.patch
        4 kB
        Robert Muir
      2. LUCENE-4297.patch
        7 kB
        Robert Muir
      3. LUCENE-4297.patch
        7 kB
        Robert Muir

        Activity

        Hide
        Robert Muir added a comment -

        here's a test. See e.g. ant test -Dtestcase=TestBoolean2 -Dtests.method=testRandomQueries -Dtests.seed=4CC92AB04D277F13 -Dtests.locale=en_AU -Dtests.timezone=Africa/Porto-Novo -Dtests.file.encoding=UTF-8

        Show
        Robert Muir added a comment - here's a test. See e.g. ant test -Dtestcase=TestBoolean2 -Dtests.method=testRandomQueries -Dtests.seed=4CC92AB04D277F13 -Dtests.locale=en_AU -Dtests.timezone=Africa/Porto-Novo -Dtests.file.encoding=UTF-8
        Hide
        Robert Muir added a comment -

        Simple patch: remove the coord multiplication in ConjunctionScorer completely.
        the counting one is then correct, and the non-counting "dual" one is fine too, because its used in more complex situations (e.g. minShouldMatch) where its subscorers are counting scorers.

        tests pass.

        Show
        Robert Muir added a comment - Simple patch: remove the coord multiplication in ConjunctionScorer completely. the counting one is then correct, and the non-counting "dual" one is fine too, because its used in more complex situations (e.g. minShouldMatch) where its subscorers are counting scorers. tests pass.
        Hide
        Robert Muir added a comment -

        patch with improved testing: sometimes use a coord() like this in lucenetestcase.

        for 3.6.x backport we can use the original simpler test since it doesnt randomize sims

        Show
        Robert Muir added a comment - patch with improved testing: sometimes use a coord() like this in lucenetestcase. for 3.6.x backport we can use the original simpler test since it doesnt randomize sims
        Hide
        Uwe Schindler added a comment -

        Is BQ1 correct? Just to be sure!
        +1 to backport.

        Show
        Uwe Schindler added a comment - Is BQ1 correct? Just to be sure! +1 to backport.
        Hide
        Robert Muir added a comment -

        It is: basically the reason TestBoolean2.testRandomQueries fails is not the comparison with BS1, but instead the fact the score differs from the explanation.

        However, see the latest patch, its way more thorough because it means we sometimes use a coord() like this across a host of tests.
        So with the latest patch's tests, if you revert the fix, many tests fail such as TestPhraseQuery.testPhraseQueryInConjunctionScorer, TestFilteredQuery, etc.

        But the first test is good for 3.6, since we don't have this Similarity randomization there.

        Show
        Robert Muir added a comment - It is: basically the reason TestBoolean2.testRandomQueries fails is not the comparison with BS1, but instead the fact the score differs from the explanation. However, see the latest patch, its way more thorough because it means we sometimes use a coord() like this across a host of tests. So with the latest patch's tests, if you revert the fix, many tests fail such as TestPhraseQuery.testPhraseQueryInConjunctionScorer, TestFilteredQuery, etc. But the first test is good for 3.6, since we don't have this Similarity randomization there.
        Hide
        Robert Muir added a comment -

        Thanks to Pascal for discovering this

        Show
        Robert Muir added a comment - Thanks to Pascal for discovering this
        Hide
        Pascal Chollet added a comment -

        Thanks for fixing it so quickly!

        Show
        Pascal Chollet added a comment - Thanks for fixing it so quickly!
        Hide
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.

          People

          • Assignee:
            Unassigned
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development