Solr
  1. Solr
  2. SOLR-5408

CollapsingQParserPlugin scores incorrectly when multiple sort criteria are used

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 4.5
    • Fix Version/s: 4.6.1
    • Component/s: None
    • Labels:
      None

      Description

      When using the collapsing query parser, only the last sort field appears to be used.

      http://172.18.0.10:8080/solr/product/select_eng?sort=score%20desc,name_sort_eng%20desc&qf=name_eng^3+brand^2+categories_term_eng+sku+upc+promoTag+model+related_terms_eng&pf2=name_eng^2&defType=edismax&rows=12&pf=name_eng~5^3&start=0&q=ipad&boost=sqrt(popularity)&qt=/select_eng&fq=productType:MERCHANDISE&fq=merchant:bestbuycanada&fq=(:AND-all_all_suppressed_b_ovly:[*+TO+*]AND-rbc_all_suppressed_b_ovly:[*+TO+*]AND-rbc_cpx_suppressed_b_ovly:[*+TO+*])OR(all_all_suppressed_b_ovly:false+AND+-rbc_all_suppressed_b_ovly:[*+TO+*]AND-rbc_cpx_suppressed_b_ovly:[*+TO+*])OR(rbc_all_suppressed_b_ovly:false+AND+-rbc_cpx_suppressed_b_ovly:[*+TO+*])OR(rbc_cpx_suppressed_b_ovly:false)&fq=translations:eng&fl=psid,name_eng,score&debug=true&debugQuery=true&fq=

      {!collapse+field%3DgroupId+nullPolicy%3Dexpand}

      <result name="response" numFound="5927" start="0" maxScore="5.6674457">
      <doc>
      <str name="psid">3002010250210</str>
      <str name="name_eng">
      ZOTAC ZBOX nano XS AD13 Plus All-In-One PC (AMD E2-1800/2GB RAM/64GB SSD)
      </str>
      <float name="score">0.41423172</float>
      </doc>

      The same query without using the collapsing query parser produces the expected result.

      1. CollapsingQParserPlugin.java
        29 kB
        Joel Bernstein
      2. CollapsingQParserPlugin.java
        29 kB
        Brandon Chapman
      3. SOLR-5027.patch
        33 kB
        Brandon Chapman
      4. SOLR-5408.2.patch
        0.9 kB
        Joel Bernstein
      5. SOLR-5408.patch
        5 kB
        Erik Hatcher
      6. SOLR-5408.patch
        1 kB
        Joel Bernstein

        Issue Links

          Activity

          Hide
          Joel Bernstein added a comment -

          I was able to reproduce and am investigating what the issue is.

          Show
          Joel Bernstein added a comment - I was able to reproduce and am investigating what the issue is.
          Hide
          Joel Bernstein added a comment -

          Brandon,

          I believe this patch should resolve the issue. It was created on branch_4x. If it doesn't apply to your build, let me know and I'll create a patch for the version you're working with.

          The problem was that the scorer needed to be set on the delegate collecter after each segment reader was set. The initial code was setting the scorer on the delegate collector only once, which worked fine for single sort critera.

          Joel

          Show
          Joel Bernstein added a comment - Brandon, I believe this patch should resolve the issue. It was created on branch_4x. If it doesn't apply to your build, let me know and I'll create a patch for the version you're working with. The problem was that the scorer needed to be set on the delegate collecter after each segment reader was set. The initial code was setting the scorer on the delegate collector only once, which worked fine for single sort critera. Joel
          Hide
          Joel Bernstein added a comment -

          I'll add a test case for this as well going forward.

          Show
          Joel Bernstein added a comment - I'll add a test case for this as well going forward.
          Hide
          ASF subversion and git services added a comment -

          Commit 1540904 from Joel Bernstein in branch 'dev/trunk'
          [ https://svn.apache.org/r1540904 ]

          SOLR-5408 Fix CollapsingQParserPlugin issue with compound sort criteria

          Show
          ASF subversion and git services added a comment - Commit 1540904 from Joel Bernstein in branch 'dev/trunk' [ https://svn.apache.org/r1540904 ] SOLR-5408 Fix CollapsingQParserPlugin issue with compound sort criteria
          Hide
          Erik Hatcher added a comment -

          Here's a test case with Joel's fix merged in too.

          Show
          Erik Hatcher added a comment - Here's a test case with Joel's fix merged in too.
          Hide
          ASF subversion and git services added a comment -

          Commit 1540922 from Joel Bernstein in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1540922 ]

          SOLR-5408 Fix CollapsingQParserPlugin issue with compound sort criteria

          Show
          ASF subversion and git services added a comment - Commit 1540922 from Joel Bernstein in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1540922 ] SOLR-5408 Fix CollapsingQParserPlugin issue with compound sort criteria
          Hide
          Joel Bernstein added a comment -

          Erick,

          I like the a test case. Only one concern and that is the assertion of 5 segments. Could be a race condition here where a background merge gets done in between commits, causing there to be fewer then 5 segments.

          I was thinking about just taking out this assertion and using the rest of the test. All we need is there to be more then 1 segment to ensure the test is valid.

          Let me know what you think,

          Thanks,
          Joel

          Show
          Joel Bernstein added a comment - Erick, I like the a test case. Only one concern and that is the assertion of 5 segments. Could be a race condition here where a background merge gets done in between commits, causing there to be fewer then 5 segments. I was thinking about just taking out this assertion and using the rest of the test. All we need is there to be more then 1 segment to ensure the test is valid. Let me know what you think, Thanks, Joel
          Hide
          Joel Bernstein added a comment -

          Further testing shows the bug is still present after this patch. Continuing to investigate...

          Show
          Joel Bernstein added a comment - Further testing shows the bug is still present after this patch. Continuing to investigate...
          Hide
          Erik Hatcher added a comment -

          Only one concern and that is the assertion of 5 segments... All we need is there to be more then 1 segment to ensure the test is valid.

          +1, by all means modify the test to whatever makes sense here. Maybe

          assertTrue("Must be more than one segment", searcherRef.get().getIndexReader().leaves().size() > 1)

          or something like that?

          Show
          Erik Hatcher added a comment - Only one concern and that is the assertion of 5 segments... All we need is there to be more then 1 segment to ensure the test is valid. +1, by all means modify the test to whatever makes sense here. Maybe assertTrue( "Must be more than one segment" , searcherRef.get().getIndexReader().leaves().size() > 1) or something like that?
          Hide
          Erik Hatcher added a comment -

          Further testing shows the bug is still present after this patch.

          Under what conditions? Can you post a failing test case?

          Show
          Erik Hatcher added a comment - Further testing shows the bug is still present after this patch. Under what conditions? Can you post a failing test case?
          Hide
          Joel Bernstein added a comment - - edited

          When I started testing with a larger dataset (millions) it become apparent pretty quickly that things were still broken with the score sorting when other criteria is used.

          I'm about to post the fix that worked on the larger data set.

          I'll have to give some thought into how get this to break with a small test case.

          Show
          Joel Bernstein added a comment - - edited When I started testing with a larger dataset (millions) it become apparent pretty quickly that things were still broken with the score sorting when other criteria is used. I'm about to post the fix that worked on the larger data set. I'll have to give some thought into how get this to break with a small test case.
          Hide
          Joel Bernstein added a comment - - edited

          I just posted a new patch.

          This patch makes a change to the "dummy" scorer that is passed down to the delegate collector.

          The issue was that certain priority queue implementations wrap the scorer in a cache that saves the score for the last docId. For this cache to work properly the dummy scorer needed to implement the docID() method properly. This patch does that.

          Not all the priority queue implementations use this technique, so depending on how the query is executed you may or may not hit the bug.

          Show
          Joel Bernstein added a comment - - edited I just posted a new patch. This patch makes a change to the "dummy" scorer that is passed down to the delegate collector. The issue was that certain priority queue implementations wrap the scorer in a cache that saves the score for the last docId. For this cache to work properly the dummy scorer needed to implement the docID() method properly. This patch does that. Not all the priority queue implementations use this technique, so depending on how the query is executed you may or may not hit the bug.
          Hide
          Joel Bernstein added a comment -

          Brandon,

          If you want to post your version of the CollapsingQParserPlugin directly to this ticket. I will make the changes to the version you have and post it back.

          Otherwise, I will shortly be committing this fix to trunk and 4x so you could replace your version with the latest version.

          Show
          Joel Bernstein added a comment - Brandon, If you want to post your version of the CollapsingQParserPlugin directly to this ticket. I will make the changes to the version you have and post it back. Otherwise, I will shortly be committing this fix to trunk and 4x so you could replace your version with the latest version.
          Hide
          ASF subversion and git services added a comment -

          Commit 1541232 from Joel Bernstein in branch 'dev/trunk'
          [ https://svn.apache.org/r1541232 ]

          SOLR-5408 Fixed issue with scorer

          Show
          ASF subversion and git services added a comment - Commit 1541232 from Joel Bernstein in branch 'dev/trunk' [ https://svn.apache.org/r1541232 ] SOLR-5408 Fixed issue with scorer
          Hide
          Brandon Chapman added a comment -

          Attached patch that we are using.

          Show
          Brandon Chapman added a comment - Attached patch that we are using.
          Hide
          Joel Bernstein added a comment -

          Brandon,

          I'll edit your version of the CollapsingQParserPlugin directly and post that back to the ticket.

          So just attach the CollapserQParserPlugin.java file.

          Joel

          Show
          Joel Bernstein added a comment - Brandon, I'll edit your version of the CollapsingQParserPlugin directly and post that back to the ticket. So just attach the CollapserQParserPlugin.java file. Joel
          Hide
          Brandon Chapman added a comment -

          attaching java file instead of patch

          Show
          Brandon Chapman added a comment - attaching java file instead of patch
          Hide
          Joel Bernstein added a comment -

          Brandon,

          I put a file up for you to test. I don't have the same build as you have anymore so I won't be able to compile and test. But the changes were very small, so I suspect they will work.

          Joel

          Show
          Joel Bernstein added a comment - Brandon, I put a file up for you to test. I don't have the same build as you have anymore so I won't be able to compile and test. But the changes were very small, so I suspect they will work. Joel
          Hide
          ASF subversion and git services added a comment -

          Commit 1541277 from Joel Bernstein in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1541277 ]

          SOLR-5408 Fixed issue with scorer

          Show
          ASF subversion and git services added a comment - Commit 1541277 from Joel Bernstein in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1541277 ] SOLR-5408 Fixed issue with scorer
          Hide
          Brandon Chapman added a comment -

          Joel,

          The file you provided worked. I did a brief test in our staging environment for the sorting and ran all our integration tests.

          Thanks,

          Brandon

          Show
          Brandon Chapman added a comment - Joel, The file you provided worked. I did a brief test in our staging environment for the sorting and ran all our integration tests. Thanks, Brandon
          Hide
          Joel Bernstein added a comment -

          Brandon,

          That's good news. This fix has been committed to trunk and 4x. Thanks for reporting.

          Joel

          Show
          Joel Bernstein added a comment - Brandon, That's good news. This fix has been committed to trunk and 4x. Thanks for reporting. Joel
          Hide
          ASF subversion and git services added a comment -

          Commit 1552875 from Joel Bernstein in branch 'dev/branches/lucene_solr_4_6'
          [ https://svn.apache.org/r1552875 ]

          SOLR-5408: CollapsingQParserPlugin scores incorrectly when multiple sort criteria are used

          Show
          ASF subversion and git services added a comment - Commit 1552875 from Joel Bernstein in branch 'dev/branches/lucene_solr_4_6' [ https://svn.apache.org/r1552875 ] SOLR-5408 : CollapsingQParserPlugin scores incorrectly when multiple sort criteria are used

            People

            • Assignee:
              Joel Bernstein
              Reporter:
              Brandon Chapman
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development