SOLR-12393:fix score not returned if expanded docs sorted by non-score

solr/CHANGES.txt

@@ -130,6 +130,9 @@ Bug Fixes
 * SOLR-13866: Fix bug introduced by SOLR-13677 that caused DirectUpdateHandler2 stats to not be available via /admin/plugins
   handler (Tomás Fernández Löbbe)
 
+* SOLR-12393: Compute score if requested even when expanded docs not sorted by score in ExpandComponent.
+  (David Smiley, Munendra S N)
+
 Other Changes
 ---------------------
 
@@ -138,6 +141,7 @@ Other Changes
 * SOLR-13841: Add jackson databind annotations to SolrJ classpath (noble)
 
 * SOLR-13824: Strictly reject anything after JSON in most APIs (Mikhail Khludnev, Munendra S N)
+
 ==================  8.3.0 ==================
 
 Consult the LUCENE_CHANGES.txt file for additional, low level, changes in this release.
@@ -207,7 +211,7 @@ Improvements
 ----------------------
 
 * SOLR-12368: Support InPlace DV updates for a field that does not yet exist in any documents
-(hossman, Simon Willnauer, Adrien Grand, Munendra S N)
+  (hossman, Simon Willnauer, Adrien Grand, Munendra S N)
 
 * SOLR-13558, SOLR-13693: Allow dynamic resizing of SolrCache-s. (ab)
 

solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java

@@ -78,6 +78,7 @@ import org.apache.solr.search.DocIterator;
 import org.apache.solr.search.DocList;
 import org.apache.solr.search.DocSlice;
 import org.apache.solr.search.QParser;
+import org.apache.solr.search.ReturnFields;
 import org.apache.solr.search.SolrIndexSearcher;
 import org.apache.solr.search.SortSpecParsing;
 import org.apache.solr.search.SyntaxError;
@@ -406,15 +407,15 @@ public class ExpandComponent extends SearchComponent implements PluginInfoInitia
       collector = groupExpandCollector;
     }
 
-    if (pfilter.filter == null) {
+    if (pfilter.filter != null) {
-      searcher.search(query, collector);
+      query = new BooleanQuery.Builder()
-    } else {
-      Query q = new BooleanQuery.Builder()
           .add(query, Occur.MUST)
           .add(pfilter.filter, Occur.FILTER)
           .build();
-      searcher.search(q, collector);
     }
+    searcher.search(query, collector);
+
+    ReturnFields returnFields = rb.rsp.getReturnFields();
     LongObjectMap<Collector> groups = ((GroupCollector) groupExpandCollector).getGroups();
     NamedList outMap = new SimpleOrderedMap();
     CharsRefBuilder charsRef = new CharsRefBuilder();
@@ -424,6 +425,9 @@ public class ExpandComponent extends SearchComponent implements PluginInfoInitia
       TopDocs topDocs = topDocsCollector.topDocs();
       ScoreDoc[] scoreDocs = topDocs.scoreDocs;
       if (scoreDocs.length > 0) {
+        if (returnFields.wantsScore() && sort != null) {
+          TopFieldCollector.populateScores(scoreDocs, searcher, query);
+        }
         int[] docs = new int[scoreDocs.length];
         float[] scores = new float[scoreDocs.length];
         for (int i = 0; i < docs.length; i++) {

solr/core/src/test/org/apache/solr/handler/component/DistributedExpandComponentTest.java

@@ -29,9 +29,9 @@ import org.junit.BeforeClass;
 import org.junit.Test;
 
 /**
- * Test for QueryComponent's distributed querying
+ * Test for distributed ExpandComponent
  *
- * @see org.apache.solr.handler.component.QueryComponent
+ * @see org.apache.solr.handler.component.ExpandComponent
  */
 public class DistributedExpandComponentTest extends BaseDistributedSearchTestCase {
 

solr/core/src/test/org/apache/solr/handler/component/TestExpandComponent.java

@@ -17,6 +17,7 @@
 package org.apache.solr.handler.component;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 
@@ -80,42 +81,36 @@ public class TestExpandComponent extends SolrTestCaseJ4 {
   }
 
   private void _testExpand(String group, String floatAppend, String hint) throws Exception {
-
+    String[][] docs = {
-    String[] doc = {"id","1", "term_s", "YYYY", group, "1"+floatAppend, "test_i", "5", "test_l", "10", "test_f", "2000", "type_s", "parent"};
+        {"id","1", "term_s", "YYYY", group, "1"+floatAppend, "test_i", "5", "test_l", "10", "test_f", "2000", "type_s", "parent"},
+        {"id","2", "term_s","YYYY", group, "1"+floatAppend, "test_i", "50", "test_l", "100", "test_f", "200", "type_s", "child"},
+        {"id","3", "term_s", "YYYY", "test_i", "5000", "test_l", "100", "test_f", "200"},
+        {"id","4", "term_s", "YYYY", "test_i", "500", "test_l", "1000", "test_f", "2000"},
+        {"id","5", "term_s", "YYYY", group, "2"+floatAppend, "test_i", "4", "test_l", "10", "test_f", "2000", "type_s", "parent"},
+        {"id","6", "term_s","YYYY", group, "2"+floatAppend, "test_i", "10", "test_l", "100", "test_f", "200", "type_s", "child"},
+        {"id","7", "term_s", "YYYY", group, "1"+floatAppend, "test_i", "1", "test_l", "100000", "test_f", "2000", "type_s", "child"},
+        {"id","8", "term_s","YYYY", group, "2"+floatAppend, "test_i", "2", "test_l",  "100000", "test_f", "200", "type_s", "child"}
+    };
+    // randomize addition of docs into bunch of segments
+    // TODO there ought to be a test utility to do this; even add in batches
+    Collections.shuffle(Arrays.asList(docs), random());
+    for (String[] doc : docs) {
       assertU(adoc(doc));
+      if (random().nextBoolean()) {
         assertU(commit());
-    String[] doc1 = {"id","2", "term_s","YYYY", group, "1"+floatAppend, "test_i", "50", "test_l", "100", "test_f", "200", "type_s", "child"};
+      }
-    assertU(adoc(doc1));
+    }
-
-    String[] doc2 = {"id","3", "term_s", "YYYY", "test_i", "5000", "test_l", "100", "test_f", "200"};
-    assertU(adoc(doc2));
-    assertU(commit());
-    String[] doc3 = {"id","4", "term_s", "YYYY", "test_i", "500", "test_l", "1000", "test_f", "2000"};
-    assertU(adoc(doc3));
-
-
-    String[] doc4 = {"id","5", "term_s", "YYYY", group, "2"+floatAppend, "test_i", "4", "test_l", "10", "test_f", "2000", "type_s", "parent"};
-    assertU(adoc(doc4));
-    assertU(commit());
-    String[] doc5 = {"id","6", "term_s","YYYY", group, "2"+floatAppend, "test_i", "10", "test_l", "100", "test_f", "200", "type_s", "child"};
-    assertU(adoc(doc5));
-    assertU(commit());
-
-    String[] doc6 = {"id","7", "term_s", "YYYY", group, "1"+floatAppend, "test_i", "1", "test_l", "100000", "test_f", "2000", "type_s", "child"};
-    assertU(adoc(doc6));
-    assertU(commit());
-    String[] doc7 = {"id","8", "term_s","YYYY", group, "2"+floatAppend, "test_i", "2", "test_l", "100000", "test_f", "200", "type_s", "child"};
-    assertU(adoc(doc7));
 
     assertU(commit());
 
-    //First basic test case.
     ModifiableSolrParams params = new ModifiableSolrParams();
     params.add("q", "*:*");
     params.add("fq", "{!collapse field="+group+hint+"}");
     params.add("defType", "edismax");
     params.add("bf", "field(test_i)");
     params.add("expand", "true");
+
+    //First basic test case.
     assertQ(req(params), "*[count(/response/result/doc)=2]",
         "*[count(/response/lst[@name='expanded']/result)=2]",
         "/response/result/doc[1]/str[@name='id'][.='2']",
@@ -128,15 +123,7 @@ public class TestExpandComponent extends SolrTestCaseJ4 {
 
     //Basic test case page 2
 
-    params = new ModifiableSolrParams();
+    assertQ(req(params, "rows", "1", "start", "1"), "*[count(/response/result/doc)=1]",
-    params.add("q", "*:*");
-    params.add("fq", "{!collapse field="+group+hint+"}");
-    params.add("defType", "edismax");
-    params.add("bf", "field(test_i)");
-    params.add("expand", "true");
-    params.add("rows", "1");
-    params.add("start", "1");
-    assertQ(req(params), "*[count(/response/result/doc)=1]",
         "*[count(/response/lst[@name='expanded']/result)=1]",
         "/response/result/doc[1]/str[@name='id'][.='6']",
         "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[1]/str[@name='id'][.='5']",
@@ -144,14 +131,9 @@ public class TestExpandComponent extends SolrTestCaseJ4 {
     );
 
     //Test expand.sort
-    params = new ModifiableSolrParams();
+    //the "sub()" just testing function queries
-    params.add("q", "*:*");
+    assertQ(req(params,"expand.sort", "test_l desc, sub(1,1) asc"),
-    params.add("fq", "{!collapse field="+group+hint+"}");
+        "*[count(/response/result/doc)=2]",
-    params.add("defType", "edismax");
-    params.add("bf", "field(test_i)");
-    params.add("expand", "true");
-    params.add("expand.sort", "test_l desc, sub(1,1) asc");//the "sub()" just testing function queries
-    assertQ(req(params), "*[count(/response/result/doc)=2]",
         "*[count(/response/lst[@name='expanded']/result)=2]",
         "/response/result/doc[1]/str[@name='id'][.='2']",
         "/response/result/doc[2]/str[@name='id'][.='6']",
@@ -182,7 +164,7 @@ public class TestExpandComponent extends SolrTestCaseJ4 {
     );
 
 
-    //Test overide expand.q
+    //Test override expand.q
 
     params = new ModifiableSolrParams();
     params.add("q", "type_s:parent");
@@ -203,7 +185,7 @@ public class TestExpandComponent extends SolrTestCaseJ4 {
     );
 
 
-    //Test overide expand.fq
+    //Test override expand.fq
 
     params = new ModifiableSolrParams();
     params.add("q", "*:*");
@@ -224,7 +206,7 @@ public class TestExpandComponent extends SolrTestCaseJ4 {
         "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[2]/str[@name='id'][.='6']"
     );
 
-    //Test overide expand.fq and expand.q
+    //Test override expand.fq and expand.q
 
     params = new ModifiableSolrParams();
     params.add("q", "*:*");
@@ -303,24 +285,81 @@ public class TestExpandComponent extends SolrTestCaseJ4 {
     params.add("defType", "edismax");
     params.add("bf", "field(test_i)");
     params.add("expand", "true");
-    params.add("fl", "id");
+
-    assertQ(req(params), "*[count(/response/result/doc)=2]",
+    assertQ(req(params, "fl", "id"),
+        "*[count(/response/result/doc)=2]",
         "*[count(/response/lst[@name='expanded']/result)=2]",
         "/response/result/doc[1]/str[@name='id'][.='2']",
         "/response/result/doc[2]/str[@name='id'][.='6']",
         "/response/lst[@name='expanded']/result[@name='1"+floatAppend+"']/doc[1]/str[@name='id'][.='1']",
         "/response/lst[@name='expanded']/result[@name='1"+floatAppend+"']/doc[2]/str[@name='id'][.='7']",
         "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[1]/str[@name='id'][.='5']",
-        "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[2]/str[@name='id'][.='8']"
+        "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[2]/str[@name='id'][.='8']",
+        "count(//*[@name='score'])=0" // score shouldn't be returned when not requested
+    );
+
+    //Test key-only fl with score but no sorting
+
+    assertQ(req(params, "fl", "id,score"), "*[count(/response/result/doc)=2]",
+        "*[count(/response/lst[@name='expanded']/result)=2]",
+        "/response/result/doc[1]/str[@name='id'][.='2']",
+        "/response/result/doc[2]/str[@name='id'][.='6']",
+        "/response/lst[@name='expanded']/result[@name='1"+floatAppend+"']/doc[1]/str[@name='id'][.='1']",
+        "/response/lst[@name='expanded']/result[@name='1"+floatAppend+"']/doc[2]/str[@name='id'][.='7']",
+        "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[1]/str[@name='id'][.='5']",
+        "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[2]/str[@name='id'][.='8']",
+        "count(//*[@name='score' and .='NaN'])=0"
+    );
+
+
+    // Test with fl and sort=score desc
+    assertQ(req(params, "expand.sort", "score desc", "fl", "id,score"),
+        "*[count(/response/result/doc)=2]",
+        "*[count(/response/lst[@name='expanded']/result)=2]",
+        "/response/result/doc[1]/str[@name='id'][.='2']",
+        "/response/result/doc[2]/str[@name='id'][.='6']",
+        "/response/lst[@name='expanded']/result[@name='1"+floatAppend+"']/doc[1]/str[@name='id'][.='1']",
+        "/response/lst[@name='expanded']/result[@name='1"+floatAppend+"']/doc[2]/str[@name='id'][.='7']",
+        "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[1]/str[@name='id'][.='5']",
+        "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[2]/str[@name='id'][.='8']",
+        "count(//*[@name='score' and .='NaN'])=0"
+    );
+
+    //Test fl with score, sort by non-score
+
+    assertQ(req(params, "expand.sort", "test_l desc", "fl", "id,test_i,score"),
+        "*[count(/response/result/doc)=2]",
+        "count(/response/lst[@name='expanded']/result)=2",
+        "/response/result/doc[1]/str[@name='id'][.='2']",
+        "/response/result/doc[2]/str[@name='id'][.='6']",
+        // note that the expanded docs are score descending order (score is 1 test_i)
+        "/response/lst[@name='expanded']/result[@name='1"+floatAppend+"']/doc[1]/str[@name='id'][.='7']",
+        "/response/lst[@name='expanded']/result[@name='1"+floatAppend+"']/doc[2]/str[@name='id'][.='1']",
+        "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[1]/str[@name='id'][.='8']",
+        "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[2]/str[@name='id'][.='5']",
+        "count(//*[@name='score' and .='NaN'])=0",
+        "count(/response/lst[@name='expanded']/result/doc[number(*/@name='score')!=number(*/@name='test_i')])=0"
+    );
+
+    //Test fl with score with multi-sort
+
+    assertQ(req(params, "expand.sort", "test_l desc, score asc", "fl", "id,test_i,score"),
+        "*[count(/response/result/doc)=2]",
+        "count(/response/lst[@name='expanded']/result)=2",
+        "/response/result/doc[1]/str[@name='id'][.='2']",
+        "/response/result/doc[2]/str[@name='id'][.='6']",
+        // note that the expanded docs are score descending order (score is 1 test_i)
+        "/response/lst[@name='expanded']/result[@name='1"+floatAppend+"']/doc[1]/str[@name='id'][.='7']",
+        "/response/lst[@name='expanded']/result[@name='1"+floatAppend+"']/doc[2]/str[@name='id'][.='1']",
+        "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[1]/str[@name='id'][.='8']",
+        "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[2]/str[@name='id'][.='5']",
+        "count(//*[@name='score' and .='NaN'])=0",
+        "count(/response/lst[@name='expanded']/result/doc[number(*/@name='score')!=number(*/@name='test_i')])=0"
     );
   }
 
   @Test
   public void testExpandWithEmptyIndexReturnsZeroResults() {
-    //We make sure the index is cleared
-
-    clearIndex();
-    assertU(commit());
 
     ModifiableSolrParams params = new ModifiableSolrParams();
     params.add("q", "*:*");