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_r266972873
##########
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##########
@@ -532,6 +578,12 @@ protected int groupedDistributedProcess(ResponseBuilder
rb) {
nextStage = ResponseBuilder.STAGE_DONE;
}
+ if (rb.stage == ResponseBuilder.STAGE_EXECUTE_QUERY &&
rb.getGroupingSpec().isSkipSecondGroupingStep()) {
+ shardRequestFactory = new StoredFieldsShardRequestFactory();
+ nextStage = ResponseBuilder.STAGE_DONE;
+ rb.stage = ResponseBuilder.STAGE_GET_FIELDS;
Review comment:
**Background re: existing code:**
[SearchHandler.handleRequestBody](https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.0.0/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java#L345-L349)
calls `distributedProcess` on all configured components `c` and via the
```
nextStage = Math.min(nextStage, c.distributedProcess(rb));
```
formula each component thus gets a say in what `nextStage` is. The overall
decision is then effected via the [rb.stage =
nextStage;](https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.0.0/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java#L342)
assignment.
**Observation re: this proposed `QueryComponent.groupedDistributedProcess`
change:**
If `QueryComponent` also assigns to `rb.stage` then that could 'confuse' the
`SearchHandler` logic e.g. specifically components running after the
`QueryComponent` would not have an opportunity to do anything in the
`ResponseBuilder.STAGE_EXECUTE_QUERY` stage.
**Question/Suggestion:**
Might an alternative be to turn the `ResponseBuilder.STAGE_EXECUTE_QUERY`
stage into a 'no op' as far as the `QueryComponent` (in
`isSkipSecondGroupingStep==true` circumstances) is concerned?
https://github.com/bloomberg/lucene-solr/pull/229 aims to illustrate an
alternative `QueryComponent.groupedDistributedProcess` modification, changes
elsewhere might be needed too (haven't looked into that yet).
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
