HoustonPutman commented on a change in pull request #248:
URL: https://github.com/apache/solr/pull/248#discussion_r696856469
##########
File path:
solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java
##########
@@ -216,6 +225,9 @@ private void searchWithTimeLimiter(Query query,
ProcessedFilter filter,
Collector collector) throws IOException {
if (queryCommand.getTimeAllowed() > 0 ) {
+ if (collector == null) {
Review comment:
Thanks for the checks, looking at the DelegatingCollector, there's a lot
of method calls that don't screen for nulls.
After thinking about this, we shouldn't need to `setLastDelegate` if it's
null, as there is no delegate to set.
Likewise, we shouldn't need to wrap a null collector with the
`hitCountCollector`. we can just set collector = hitCountCollector.
I feel like the null check makes sense independently in both of these other
if statements. So I retract my original suggestion, but we should definitely be
doing null checks below.
I might be going to far into the weeds here, but we probably want one last
null check before doing the search, because search will not play nicely with a
null collector. (I imagine the user would have to enable grouping and ask for
literally no grouping information to be returned, so who knows if this is
actually worth putting into the code.)
--
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.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
