SOLR-13709: Fixed distributed grouping when multiple 'fl' params are specified

solr/CHANGES.txt

@@ -192,6 +192,8 @@ Bug Fixes
 
 * SOLR-13718: SPLITSHARD (async) with failures in underlying sub-operations can result in data loss (Ishan Chattopadhyaya)
 
+* SOLR-13709: Fixed distributed grouping when multiple 'fl' params are specified (hossman, Christine Poerschke)
+
 Other Changes
 ----------------------
 

solr/core/src/java/org/apache/solr/search/grouping/distributed/requestfactory/StoredFieldsShardRequestFactory.java

@@ -64,14 +64,11 @@ public class StoredFieldsShardRequestFactory implements ShardRequestFactory {
       sreq.params.remove(GroupParams.GROUP);
       sreq.params.remove(CommonParams.SORT);
       sreq.params.remove(ResponseBuilder.FIELD_SORT_VALUES);
-      String fl = sreq.params.get(CommonParams.FL);
+      
-      if (fl != null) {
+      // we need to ensure the uniqueField is included for collating docs with their return fields
-         fl = fl.trim();
+      if (! rb.rsp.getReturnFields().wantsField(uniqueField.getName())) {
-        // currently, "score" is synonymous with "*,score" so
+        // the user didn't ask for it, so we have to...
-        // don't add "id" if the fl is empty or "score" or it would change the meaning.
+        sreq.params.add(CommonParams.FL, uniqueField.getName());
-         if (fl.length()!=0 && !"score".equals(fl) && !"*".equals(fl)) {
-           sreq.params.set(CommonParams.FL, fl+','+uniqueField.getName());
-         }
       }
 
       List<String> ids = new ArrayList<>(shardDocs.size());

solr/core/src/test/org/apache/solr/TestDistributedGrouping.java

@@ -27,6 +27,7 @@ import org.apache.solr.common.SolrDocumentList;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.SolrTestCaseJ4.SuppressPointFields;
 import org.junit.Test;
@@ -67,6 +68,7 @@ public class TestDistributedGrouping extends BaseDistributedSearchTestCase {
 
     handle.clear();
     handle.put("timestamp", SKIPVAL);
+    handle.put("_version_", SKIP);
     handle.put("grouped", UNORDERED);   // distrib grouping doesn't guarantee order of top level group commands
 
     // Test distributed grouping with empty indices
@@ -188,6 +190,7 @@ public class TestDistributedGrouping extends BaseDistributedSearchTestCase {
 
     query("q", "*:*", "fl", "id," + i1dv, "group", "true", "group.field", i1dv, "group.limit", 10, "sort", i1 + " asc, id asc");
 
+    
     // SOLR-4150: what if group.query has no matches, 
     // or only matches on one shard
     query("q", "*:*", "rows", 100, "fl", "id," + i1, "group", "true",
@@ -318,6 +321,39 @@ public class TestDistributedGrouping extends BaseDistributedSearchTestCase {
     query("q", "{!func}id_i1", "rows", 100, "fl", "score,id," + i1, "group", "true", "group.field", i1, "group.limit", -1, "sort", "score desc, _docid_ asc, id asc");
     query("q", "{!func}id_i1", "rows", 100, "fl", "score,id," + i1, "group", "true", "group.field", i1, "group.limit", -1);
 
+    // grouping shouldn't care if there are multiple fl params, or what order the fl field names are in
+    variantQuery(params("q", "*:*",
+                        "group", "true", "group.field", i1dv, "group.limit", "10",
+                        "sort", i1 + " asc, id asc")
+                 , params("fl", "id," + i1dv)
+                 , params("fl", i1dv + ",id")
+                 , params("fl", "id", "fl", i1dv)
+                 , params("fl", i1dv, "fl", "id")
+                 );
+    variantQuery(params("q", "*:*", "rows", "100",
+                        "group", "true", "group.field", s1dv, "group.limit", "-1", 
+                        "sort", b1dv + " asc, id asc",
+                        "group.sort", "id desc")
+                 , params("fl", "id," + s1dv + "," + tdate_a)
+                 , params("fl", "id", "fl", s1dv, "fl", tdate_a)
+                 , params("fl", tdate_a, "fl", s1dv, "fl", "id")
+                 );
+    variantQuery(params("q", "*:*", "rows", "100",
+                        "group", "true", "group.field", s1dv, "group.limit", "-1", 
+                        "sort", b1dv + " asc, id asc",
+                        "group.sort", "id desc")
+                 , params("fl", s1dv + "," + tdate_a)
+                 , params("fl", s1dv, "fl", tdate_a)
+                 , params("fl", tdate_a, "fl", s1dv)
+                 );
+    variantQuery(params("q", "{!func}id_i1", "rows", "100",
+                        "group", "true", "group.field", i1, "group.limit", "-1",
+                        "sort", tlong+" asc, id desc")
+                 , params("fl", t1 + ",score," + i1dv)
+                 , params("fl", t1, "fl", "score", "fl", i1dv)
+                 , params("fl", "score", "fl", t1, "fl", i1dv)
+                 );
+                             
     // some explicit checks of non default sorting, and sort/group.sort with diff clauses
     query("q", "{!func}id_i1", "rows", 100, "fl", tlong + ",id," + i1, "group", "true",
           "group.field", i1, "group.limit", -1,
@@ -330,21 +366,47 @@ public class TestDistributedGrouping extends BaseDistributedSearchTestCase {
           "group.field", i1, "group.limit", -1,
           "sort", tlong+" asc, id desc",
           "group.sort", "id asc");
-    rsp = query("q", "{!func}id_i1", "fq", oddField+":[* TO *]",
+    for (boolean withFL : new boolean[] {true, false}) {
-                "rows", 100, "fl", tlong + ",id," + i1, "group", "true",
+      if (withFL) {
-                "group.field", i1, "group.limit", -1,
+        rsp = variantQuery(params("q", "{!func}id_i1", "fq", oddField+":[* TO *]",
-                "sort", tlong+" asc",
+                                  "rows", "100",
-                "group.sort", oddField+" asc");
+                                  "group", "true", "group.field", i1, "group.limit", "-1",
-    nl = (NamedList<?>) rsp.getResponse().get("grouped");
+                                  "sort", tlong+" asc", "group.sort", oddField+" asc")
-    nl = (NamedList<?>) nl.get(i1);
+                           , params("fl", tlong + ",id," + i1)
-    assertEquals(rsp.toString(), 6, nl.get("matches"));
+                           , params("fl", tlong, "fl", "id", "fl", i1)
-    assertEquals(rsp.toString(), 2, ((List<NamedList<?>>)nl.get("groups")).size());
+                           , params("fl", "id", "fl", i1, "fl", tlong)
-    nl = ((List<NamedList<?>>)nl.get("groups")).get(0);
+                           );
-    assertEquals(rsp.toString(), 232, nl.get("groupValue"));
+      } else {
-    SolrDocumentList docs = (SolrDocumentList) nl.get("doclist");
+        // special check: same query, but empty fl...
-    assertEquals(docs.toString(), 5, docs.getNumFound());
+        rsp = query("q", "{!func}id_i1", "fq", oddField+":[* TO *]",
-    assertEquals(docs.toString(), "22", docs.get(0).getFirstValue("id"));
+                    "rows", "100",
-    assertEquals(docs.toString(), "21", docs.get(4).getFirstValue("id"));
+                    "group", "true", "group.field", i1, "group.limit", "-1",
+                    "sort", tlong+" asc", "group.sort", oddField+" asc");
+      }
+      nl = (NamedList<?>) rsp.getResponse().get("grouped");
+      nl = (NamedList<?>) nl.get(i1);
+      assertEquals(rsp.toString(), 6, nl.get("matches"));
+      assertEquals(rsp.toString(), 2, ((List<NamedList<?>>)nl.get("groups")).size());
+      nl = ((List<NamedList<?>>)nl.get("groups")).get(0);
+      assertEquals(rsp.toString(), 232, nl.get("groupValue"));
+      SolrDocumentList docs = (SolrDocumentList) nl.get("doclist");
+      assertEquals(docs.toString(), 5, docs.getNumFound());
+      //
+      assertEquals(docs.toString(), "22", docs.get(0).getFirstValue("id"));
+      assertEquals(docs.toString(), 732L, docs.get(0).getFirstValue(tlong));
+      assertEquals(docs.toString(), 232,  docs.get(0).getFirstValue(i1));
+      //
+      assertEquals(docs.toString(), "21", docs.get(4).getFirstValue("id"));
+      assertEquals(docs.toString(), 632L, docs.get(4).getFirstValue(tlong));
+      assertEquals(docs.toString(), 232,  docs.get(4).getFirstValue(i1));
+      //
+      if (withFL == false) {
+        // exact number varies based on test randomization, but there should always be at least the 8
+        // explicitly indexed in these 2 docs...
+        assertTrue(docs.toString(), 8 <= docs.get(0).getFieldNames().size());
+        assertTrue(docs.toString(), 8 <= docs.get(4).getFieldNames().size());
+      }
+    }
     
     // grouping on boolean non-stored docValued enabled field
     rsp = query("q", b1dv + ":*", "fl", "id," + b1dv, "group", "true", "group.field",
@@ -355,7 +417,7 @@ public class TestDistributedGrouping extends BaseDistributedSearchTestCase {
     assertEquals(rsp.toString(), 2, ((List<NamedList<?>>)nl.get("groups")).size());
     nl = ((List<NamedList<?>>)nl.get("groups")).get(0);
     assertEquals(rsp.toString(), false, nl.get("groupValue"));
-    docs = (SolrDocumentList) nl.get("doclist");
+    SolrDocumentList docs = (SolrDocumentList) nl.get("doclist");
     assertEquals(docs.toString(), 4, docs.getNumFound());
     
     // Can't validate the response, but can check if no errors occur.
@@ -374,4 +436,28 @@ public class TestDistributedGrouping extends BaseDistributedSearchTestCase {
     queryServer(params);
   }
 
+  /**
+   * Special helper method for verifying that multiple queries behave the same as each other, 
+   * both in distributed and single node queries
+   *
+   * @param commonParams params that are common to all queries
+   * @param variantParams params that will be appended to the common params to create a variant query
+   * @return the last response returned by the last variant
+   * @see #query
+   * @see #compareResponses
+   * @see SolrParams#wrapAppended
+   */
+  protected QueryResponse variantQuery(final SolrParams commonParams,
+                                       final SolrParams... variantParams) throws Exception {
+    QueryResponse lastResponse = null;
+    for (SolrParams extra : variantParams) {
+      final QueryResponse rsp = query(SolrParams.wrapAppended(commonParams, extra));
+      if (null != lastResponse) {
+        compareResponses(rsp, lastResponse);
+      }
+      lastResponse = rsp;
+    }
+    return lastResponse;
+  }
+  
 }