Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-5307

Inconsistency between Weight.scorer documentation and ConstantScoreQuery on top of a Filter

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.6, 6.0
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Weight.scorer states that if topScorer == true, Scorer.collect will be called and that otherwise Scorer.nextDoc/advance will be called.

      This is a problem when ConstantScoreQuery is used on top of a QueryWrapperFilter:
      1. ConstantScoreWeight calls getDocIdSet on the filter to know which documents to collect.
      2. QueryWrapperFilter.getDocIdSet returns a Scorer created with topScorer == false so that nextDoc/advance are supported.
      3. But then ConstantScorer.score(Collector) has the following optimization:

          // this optimization allows out of order scoring as top scorer!
          @Override
          public void score(Collector collector) throws IOException {
            if (docIdSetIterator instanceof Scorer) {
              ((Scorer) docIdSetIterator).score(wrapCollector(collector));
            } else {
              super.score(collector);
            }
          }
      

      So the filter iterator is a scorer which was created with topScorer = false but ParentScorer ends up using its score(Collector) method, which is illegal. (I found this out because AssertingSearcher has some checks to make sure Scorers are used accordingly to the value of topScorer.)

      I can imagine several fixes, including:

      • removing this optimization when working on top of a filter
      • relaxing Weight documentation to allow for using score(Collector) when topScorer == false

      but I'm not sure which one is the best one. What do you think?

      1. LUCENE-5307.patch
        7 kB
        Uwe Schindler
      2. LUCENE-5307.patch
        8 kB
        Uwe Schindler
      3. LUCENE-5307-test.patch
        4 kB
        Adrien Grand

        Activity

        Hide
        thetaphi Uwe Schindler added a comment -

        Hi Adrien,
        This is actually my fault. The following fix would be most correct and makes the optimization still work for the not really useful combination: ConstsntScoreQuery(QueryWrapperFilter(Query))

        LotS of old code is using this, instead it should directly wrap the Query instead of creating a filter wrap.

        I would fix:

        • change the instanceof check to a query != null and assert that it is a Scorer
        • add another special case in rewrite to prevent the old style stupidity: rewrite the above combination to a simple ConstantScoreQuery with the Query that was wrapped by the filter, ignoring inner boost.

        Il'll upoad patch later.

        Show
        thetaphi Uwe Schindler added a comment - Hi Adrien, This is actually my fault. The following fix would be most correct and makes the optimization still work for the not really useful combination: ConstsntScoreQuery(QueryWrapperFilter(Query)) LotS of old code is using this, instead it should directly wrap the Query instead of creating a filter wrap. I would fix: change the instanceof check to a query != null and assert that it is a Scorer add another special case in rewrite to prevent the old style stupidity: rewrite the above combination to a simple ConstantScoreQuery with the Query that was wrapped by the filter, ignoring inner boost. Il'll upoad patch later.
        Hide
        jpountz Adrien Grand added a comment -

        Thanks Uwe!

        Show
        jpountz Adrien Grand added a comment - Thanks Uwe!
        Hide
        jpountz Adrien Grand added a comment -

        Here is a test that fails (feel free to not reuse it, this is just to demonstrate the problem).

        Show
        jpountz Adrien Grand added a comment - Here is a test that fails (feel free to not reuse it, this is just to demonstrate the problem).
        Hide
        thetaphi Uwe Schindler added a comment -

        Here is the patch. Thanks for the test, I was about to write a similar one!

        Show
        thetaphi Uwe Schindler added a comment - Here is the patch. Thanks for the test, I was about to write a similar one!
        Hide
        jpountz Adrien Grand added a comment -

        +1!

        Show
        jpountz Adrien Grand added a comment - +1!
        Hide
        thetaphi Uwe Schindler added a comment -

        Simplified patch. API of ConstantScorer does not change!

        Thanks Adrien for review, will commit tomorrow!

        Show
        thetaphi Uwe Schindler added a comment - Simplified patch. API of ConstantScorer does not change! Thanks Adrien for review, will commit tomorrow!
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1535950 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1535950 ]

        LUCENE-5307: Fix topScorer inconsistency in handling QueryWrapperFilter inside ConstantScoreQuery, which now rewrites to a query removing the obsolete QueryWrapperFilter

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1535950 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1535950 ] LUCENE-5307 : Fix topScorer inconsistency in handling QueryWrapperFilter inside ConstantScoreQuery, which now rewrites to a query removing the obsolete QueryWrapperFilter
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1535955 from Uwe Schindler in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1535955 ]

        Merged revision(s) 1535950 from lucene/dev/trunk:
        LUCENE-5307: Fix topScorer inconsistency in handling QueryWrapperFilter inside ConstantScoreQuery, which now rewrites to a query removing the obsolete QueryWrapperFilter

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1535955 from Uwe Schindler in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1535955 ] Merged revision(s) 1535950 from lucene/dev/trunk: LUCENE-5307 : Fix topScorer inconsistency in handling QueryWrapperFilter inside ConstantScoreQuery, which now rewrites to a query removing the obsolete QueryWrapperFilter
        Hide
        thetaphi Uwe Schindler added a comment -

        Thanks Adrien!

        Show
        thetaphi Uwe Schindler added a comment - Thanks Adrien!

          People

          • Assignee:
            thetaphi Uwe Schindler
            Reporter:
            jpountz Adrien Grand
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development