Lucene - Core
  1. Lucene - Core
  2. LUCENE-6220

Move needsScores from Weight.scorer to Query.createWeight

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.1, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Whether scores are needed is currently a Scorer-level property while it should actually be a Weight thing I think?

      1. LUCENE-6220.patch
        146 kB
        Adrien Grand
      2. LUCENE-6220.patch
        137 kB
        Adrien Grand
      3. LUCENE-6220.patch
        110 kB
        Adrien Grand

        Activity

        Hide
        Michael McCandless added a comment -

        +1

        Show
        Michael McCandless added a comment - +1
        Hide
        Adrien Grand added a comment -

        Here is a work-in-progress patch: IndexSearcher currently needs to guess in advance whether the collector will need scores. I'll have a second look at it tomorrow to see if I can somehow refactor it so that we always have a collector ready before creating the weight.

        Show
        Adrien Grand added a comment - Here is a work-in-progress patch: IndexSearcher currently needs to guess in advance whether the collector will need scores. I'll have a second look at it tomorrow to see if I can somehow refactor it so that we always have a collector ready before creating the weight.
        Hide
        Robert Muir added a comment -

        +1 if we can do this somehow. When i looked into it, it seemed it might require major refactoring of IndexSearcher. But I think it would end up better if we can do it!

        Show
        Robert Muir added a comment - +1 if we can do this somehow. When i looked into it, it seemed it might require major refactoring of IndexSearcher. But I think it would end up better if we can do it!
        Hide
        Adrien Grand added a comment -

        New patch.

        IndexSearcher no more tries to guess whether scores are needed or not by always creating the collector before the weight. I kept the signatures of the public methods compatible but had to remove some protected methods that did not make sense anymore (because the Weight was an argument while the collector was not constructed yet).

        I also tried to add more type safety to TopDoc.merge to avoid unchecked casts on the IndexSearcher side.

        Show
        Adrien Grand added a comment - New patch. IndexSearcher no more tries to guess whether scores are needed or not by always creating the collector before the weight. I kept the signatures of the public methods compatible but had to remove some protected methods that did not make sense anymore (because the Weight was an argument while the collector was not constructed yet). I also tried to add more type safety to TopDoc.merge to avoid unchecked casts on the IndexSearcher side.
        Hide
        Adrien Grand added a comment -

        New iteration:

        • removed some useless needsScores constructor arguments
        • removed the doScores parameter of ToChildBlockJoinQuery in favor of needsScores in createWeight

        I think it's ready.

        Show
        Adrien Grand added a comment - New iteration: removed some useless needsScores constructor arguments removed the doScores parameter of ToChildBlockJoinQuery in favor of needsScores in createWeight I think it's ready.
        Hide
        Robert Muir added a comment -

        +1, this is a great cleanup.

        Show
        Robert Muir added a comment - +1, this is a great cleanup.
        Hide
        Alan Woodward added a comment -

        +1, nice

        Show
        Alan Woodward added a comment - +1, nice
        Hide
        ASF subversion and git services added a comment -

        Commit 1657874 from Adrien Grand in branch 'dev/trunk'
        [ https://svn.apache.org/r1657874 ]

        LUCENE-6220: Move needsScores to Query.createWeight.

        Show
        ASF subversion and git services added a comment - Commit 1657874 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1657874 ] LUCENE-6220 : Move needsScores to Query.createWeight.
        Hide
        ASF subversion and git services added a comment -

        Commit 1657876 from Adrien Grand in branch 'dev/trunk'
        [ https://svn.apache.org/r1657876 ]

        LUCENE-6220: Remove needsScores from FilterStrategy.

        Show
        ASF subversion and git services added a comment - Commit 1657876 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1657876 ] LUCENE-6220 : Remove needsScores from FilterStrategy.
        Hide
        ASF subversion and git services added a comment -

        Commit 1657878 from Adrien Grand in branch 'dev/trunk'
        [ https://svn.apache.org/r1657878 ]

        LUCENE-6220: Remove unnecessary cast.

        Show
        ASF subversion and git services added a comment - Commit 1657878 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1657878 ] LUCENE-6220 : Remove unnecessary cast.
        Hide
        ASF subversion and git services added a comment -

        Commit 1657883 from Adrien Grand in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1657883 ]

        LUCENE-6220: Move needsScores to Query.createWeight.

        Show
        ASF subversion and git services added a comment - Commit 1657883 from Adrien Grand in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1657883 ] LUCENE-6220 : Move needsScores to Query.createWeight.
        Hide
        Mark Miller added a comment -

        I see problems locally with the change to TopGroupsShardResponseProcessor. It complains with an arraystoreexception at runtime it looks. I'll try to take a look at it.

        Show
        Mark Miller added a comment - I see problems locally with the change to TopGroupsShardResponseProcessor. It complains with an arraystoreexception at runtime it looks. I'll try to take a look at it.
        Hide
        Adrien Grand added a comment -

        Woops, sorry that I missed that. It's probably because I made sure that you pass a TopFieldDocs[] to TopDocs.merge when there is a sort. I'll look.

        Show
        Adrien Grand added a comment - Woops, sorry that I missed that. It's probably because I made sure that you pass a TopFieldDocs[] to TopDocs.merge when there is a sort. I'll look.
        Hide
        Mark Miller added a comment -

        I put in this hack, and that led to a test fail mid way through - unfortunetly I've never seen this grouping code before.

        Index: solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/TopGroupsShardResponseProcessor.java
        ===================================================================
        --- solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/TopGroupsShardResponseProcessor.java	(revision 1657874)
        +++ solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/TopGroupsShardResponseProcessor.java	(working copy)
        @@ -173,7 +173,13 @@
         
                 int topN = rb.getGroupingSpec().getOffset() + rb.getGroupingSpec().getLimit();
                 final TopDocs mergedTopDocs;
        -        if (sortWithinGroup == null) {
        +        
        +        boolean sort = false;
        +        if (topDocs.size() > 0) {
        +          sort = topDocs.get(0) instanceof TopFieldDocs;
        +        }
        +        
        +        if (sortWithinGroup == null || !sort) {
                   mergedTopDocs = TopDocs.merge(topN, topDocs.toArray(new TopDocs[topDocs.size()]));
                 } else {
                   mergedTopDocs = TopDocs.merge(sortWithinGroup, topN, topDocs.toArray(new TopFieldDocs[topDocs.size()]));
        
        Show
        Mark Miller added a comment - I put in this hack, and that led to a test fail mid way through - unfortunetly I've never seen this grouping code before. Index: solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/TopGroupsShardResponseProcessor.java =================================================================== --- solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/TopGroupsShardResponseProcessor.java (revision 1657874) +++ solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/TopGroupsShardResponseProcessor.java (working copy) @@ -173,7 +173,13 @@ int topN = rb.getGroupingSpec().getOffset() + rb.getGroupingSpec().getLimit(); final TopDocs mergedTopDocs; - if (sortWithinGroup == null) { + + boolean sort = false; + if (topDocs.size() > 0) { + sort = topDocs.get(0) instanceof TopFieldDocs; + } + + if (sortWithinGroup == null || !sort) { mergedTopDocs = TopDocs.merge(topN, topDocs.toArray(new TopDocs[topDocs.size()])); } else { mergedTopDocs = TopDocs.merge(sortWithinGroup, topN, topDocs.toArray(new TopFieldDocs[topDocs.size()]));
        Hide
        Adrien Grand added a comment -

        OK I think I found the root cause of this. As part of this change I tried to add more type safety to TopDocs.merge, and in particular this method now expects instances of TopFieldDocs when the sort is not null. But TopGroupsResultTransformer deserializes a NamedList into a TopDocs instance and while the individual ScoreDoc are instances of FieldDoc, it returns a new TopDocs instead of a new TopFieldDocs. I'm not sure how to fix it as it would require to also get the original Sort object (is it available somewhere?) but in the meantime I can try to make TopDocs.merge more permissive again to fix the problem.

        Show
        Adrien Grand added a comment - OK I think I found the root cause of this. As part of this change I tried to add more type safety to TopDocs.merge, and in particular this method now expects instances of TopFieldDocs when the sort is not null. But TopGroupsResultTransformer deserializes a NamedList into a TopDocs instance and while the individual ScoreDoc are instances of FieldDoc, it returns a new TopDocs instead of a new TopFieldDocs. I'm not sure how to fix it as it would require to also get the original Sort object (is it available somewhere?) but in the meantime I can try to make TopDocs.merge more permissive again to fix the problem.
        Hide
        ASF subversion and git services added a comment -

        Commit 1657919 from Adrien Grand in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1657919 ]

        LUCENE-6220: Fix ArrayStoreException in grouping tests.

        Show
        ASF subversion and git services added a comment - Commit 1657919 from Adrien Grand in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1657919 ] LUCENE-6220 : Fix ArrayStoreException in grouping tests.
        Hide
        ASF subversion and git services added a comment -

        Commit 1657921 from Adrien Grand in branch 'dev/trunk'
        [ https://svn.apache.org/r1657921 ]

        LUCENE-6220: Fix ArrayStoreException in grouping tests.

        Show
        ASF subversion and git services added a comment - Commit 1657921 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1657921 ] LUCENE-6220 : Fix ArrayStoreException in grouping tests.
        Hide
        Adrien Grand added a comment -

        The fix was actually easier than I initially thought, and it looks like it fixed the ArrayStoreException errors. Thanks Mark and sorry for the noise.

        Show
        Adrien Grand added a comment - The fix was actually easier than I initially thought, and it looks like it fixed the ArrayStoreException errors. Thanks Mark and sorry for the noise.
        Hide
        Timothy Potter added a comment -

        Bulk close after 5.1 release

        Show
        Timothy Potter added a comment - Bulk close after 5.1 release

          People

          • Assignee:
            Adrien Grand
            Reporter:
            Adrien Grand
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development