cpoerschke 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_r272682603
########## File path: solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java ########## @@ -288,6 +328,13 @@ protected void prepareGrouping(ResponseBuilder rb) throws IOException { groupingSpec.setMain(params.getBool(GroupParams.GROUP_MAIN, false)); groupingSpec.setNeedScore((rb.getFieldFlags() & SolrIndexSearcher.GET_SCORES) != 0); groupingSpec.setTruncateGroups(params.getBool(GroupParams.GROUP_TRUNCATE, false)); + + groupingSpec.setSkipSecondGroupingStep(params.getBool(GroupParams.GROUP_SKIP_DISTRIBUTED_SECOND, false)); + boolean isReranking = (rb.getRankQuery() != null); + if (groupingSpec.isSkipSecondGroupingStep() & !allowSkipSecondGroupingStep(groupingSpec.getWithinGroupSortSpec(), groupingSpec.getGroupSortSpec(), isReranking)){ Review comment: observation: `allowSkipSecondGroupingStep` currently does 'error' logging in response to illegal grouping specifications in a request. this probably should not be 'error' or 'warn' logging (since there is nothing 'wrong' with Solr itself) but 'info' logging at most or no logging at all with the details returned in the `Illegal grouping specification` that is thrown here. An alternative could also be to 'info' log and _not_ throw but to just ignore the `group.skip.second.step=true` parameter if it's invalid. ---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org