[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)

[GitBox <gi...@apache.org>]

diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r271027782
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SearchGroupShardResponseProcessor.java
 ##########
 @@ -143,12 +151,61 @@ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
       if (mergedTopGroups == null) {
         continue;
       }
-
-      rb.mergedSearchGroups.put(groupField, mergedTopGroups);
-      for (SearchGroup<BytesRef> mergedTopGroup : mergedTopGroups) {
-        rb.searchGroupToShards.get(groupField).put(mergedTopGroup, tempSearchGroupToShards.get(groupField).get(mergedTopGroup));
+      if (rb.getGroupingSpec().isSkipSecondGroupingStep()){
+          /* If we are skipping the second grouping step we want to translate the response of the
+           * first step in the response of the second step and send it to the get_fields step.
+           */
+          processSkipSecondGroupingStep(rb, mergedTopGroups, tempSearchGroupToShards, docIdToShard, groupSort, fields,  groupField);
 
 Review comment:
   Thanks @cpoerschke for the analysis. Please find the answers:
   
   (1) I agree, allocation can be avoided.
   (2) I agree, I'll see if it is possible to reduce or wrap them in a private object
   (3) `rb.mergedSearchGroups`
   It is used only to generate the second group request
   https://github.com/apache/lucene-solr/blob/1d85cd783863f75cea133fb9c452302214165a4d/solr/core/src/java/org/apache/solr/search/grouping/distributed/requestfactory/TopGroupsShardRequestFactory.java#L114
   
   ... That we are going to skip if we apply the skip second step:
   https://github.com/bloomberg/lucene-solr/blob/SOLR-11831/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java#L573
   
   (4-7b) Yes
   
   (10, 11) I Merged your changes, and I'll check that the tests are still fine, and that precommit is still happy
   
   Thanks

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org