You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/08/06 19:45:22 UTC
[GitHub] [solr] cpoerschke opened a new pull request #248: SOLR-6156: Fix NullPointerException if group.field grouping is used with rows=0 and timeAllowed
cpoerschke opened a new pull request #248:
URL: https://github.com/apache/solr/pull/248
https://issues.apache.org/jira/browse/SOLR-6156
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #248: SOLR-6156: Fix NullPointerException if group.field grouping is used with rows=0 and timeAllowed
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #248:
URL: https://github.com/apache/solr/pull/248#discussion_r698537736
##########
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:
Good idea.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #248: SOLR-6156: Fix NullPointerException if group.field grouping is used with rows=0 and timeAllowed
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #248:
URL: https://github.com/apache/solr/pull/248#issuecomment-900419308
selected code path links that may be helpful when code reviewing this change:
* `CommandHandler.searchWithTimeLimiter` is passed a null `collector` if called from https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java#L164
* `null` is passed if the https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java#L149-L151 loop left the `collectors` list empty.
* `CommandHandler.execute` is called by `QueryComponent.doProcessGroupedDistributedSearchFirstPhase` code: https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java#L1330
* `SearchGroupsFieldCommand.create` can return an empty list as per https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/solr/core/src/java/org/apache/solr/search/grouping/distributed/command/SearchGroupsFieldCommand.java#L100-L122
https://issues.apache.org/jira/browse/SOLR-6156 also has more context.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #248: SOLR-6156: Fix NullPointerException if group.field grouping is used with rows=0 and timeAllowed
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #248: SOLR-6156: Fix NullPointerException if group.field grouping is used with rows=0 and timeAllowed
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #248:
URL: https://github.com/apache/solr/pull/248#discussion_r696832684
##########
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:
`MultiCollector.wrap` screens out nulls: https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/lucene/core/src/java/org/apache/lucene/search/MultiCollector.java#L56-L89
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #248: SOLR-6156: Fix NullPointerException if group.field grouping is used with rows=0 and timeAllowed
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #248:
URL: https://github.com/apache/solr/pull/248#discussion_r696828554
##########
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:
I see collector being used after this if statement. Should the null check go in the beginning of the method or are all the other usages fine with a null collector?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #248: SOLR-6156: Fix NullPointerException if group.field grouping is used with rows=0 and timeAllowed
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #248:
URL: https://github.com/apache/solr/pull/248#issuecomment-900419308
selected code path links that may be helpful when code reviewing this change:
* `CommandHandler.searchWithTimeLimiter` is passed a null `collector` if called from https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java#L164
* `null` is passed if the https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java#L149-L151 loop left the `collectors` list empty.
* `CommandHandler.execute` is called by `QueryComponent.doProcessGroupedDistributedSearchFirstPhase` code: https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java#L1330
* `SearchGroupsFieldCommand.create` can return an empty list as per https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/solr/core/src/java/org/apache/solr/search/grouping/distributed/command/SearchGroupsFieldCommand.java#L100-L122
https://issues.apache.org/jira/browse/SOLR-6156 also has more context.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #248: SOLR-6156: Fix NullPointerException if group.field grouping is used with rows=0 and timeAllowed
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #248:
URL: https://github.com/apache/solr/pull/248#discussion_r698537736
##########
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:
Good idea.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #248: SOLR-6156: Fix NullPointerException if group.field grouping is used with rows=0 and timeAllowed
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #248:
URL: https://github.com/apache/solr/pull/248#discussion_r696836386
##########
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:
`DelegatingCollector.setLastDelegate` that's from a quick look a little less obvious how that would "react" to a null: https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/solr/core/src/java/org/apache/solr/search/DelegatingCollector.java#L46
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #248: SOLR-6156: Fix NullPointerException if group.field grouping is used with rows=0 and timeAllowed
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #248:
URL: https://github.com/apache/solr/pull/248#discussion_r696879443
##########
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:
How about changing the caller of this (private) method to not pass `null` in the first place?
* \+ clearly reflect at caller level that no collecting is intended and to be done (other than for hit counting purposes)
* -/+ use of no-op and hit-count collector in the `group=true rows=0` edge case
* \+ no extra null-checks or logic complication for the `group=true rows!=0` common case
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke merged pull request #248: SOLR-6156: Fix NullPointerException if group.field grouping is used with rows=0 and timeAllowed
Posted by GitBox <gi...@apache.org>.
cpoerschke merged pull request #248:
URL: https://github.com/apache/solr/pull/248
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org