Lucene - Core
  1. Lucene - Core
  2. LUCENE-5307

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

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.6, 5.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
        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
        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
        Adrien Grand added a comment -

        Thanks Uwe!

        Show
        Adrien Grand added a comment - Thanks Uwe!
        Hide
        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
        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
        Uwe Schindler added a comment -

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

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

        +1!

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

        Simplified patch. API of ConstantScorer does not change!

        Thanks Adrien for review, will commit tomorrow!

        Show
        Uwe Schindler added a comment - Simplified patch. API of ConstantScorer does not change! Thanks Adrien for review, will commit tomorrow!
        Hide
        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
        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
        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
        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
        Uwe Schindler added a comment -

        Thanks Adrien!

        Show
        Uwe Schindler added a comment - Thanks Adrien!

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development