[
https://issues.apache.org/jira/browse/SOLR-13970?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16993531#comment-16993531
]
Joel Bernstein edited comment on SOLR-13970 at 12/11/19 1:29 PM:
-----------------------------------------------------------------
All good points. Let's go through them:
{code:java}
The latest patch covers only collapse filter. I think we should also cover the
case where expand is specified w/o collapse(as it can cause NPE with grouping).
Since, issue is related to grouping behavior with collapse/expand, I think we
should handle both cases.{code}
Does SOLR-7798 fix the NPE with expand? Does grouping and expand have a
specific conflict? If it does then let's add the exception as you suggested.
{code:java}
The test added in SOLR-5230 was to check delegatingCollector's finish call
with grouping. Since, we are throwing error for collapse, We should use some
other component which uses delegatingCollector for testing. Maybe
significantTerms or some mock component. This is to ensure the finish call case
is covered in tests.{code}
I think the big problem now is not if finish() is being called, but whether we
can call the same PostFilter twice without resetting its internal state. This
is a bigger review problem which involves understanding the inner working of
each postfilter. So my suggestion is rather then putting in another token check
that we address the whole problem of postfilters with grouping in a different
ticket (SOLR-14045) I planned on addressing this ticket immediately after the
8.4 release is cut if not sooner.
{code:java}
I think we should also update the documentation for the same. {code}
+1, sorry if I missed this in the previous patch.
{code:java}
Some of the above things are covered in previous patch. If needed, we can
incorporate them into latest patch.
Joel Bernstein If you are okay with above things, I will attach new patch
incorporating those changes {code}
Sounds good! I'll check back later today and see how things are going.
.
was (Author: joel.bernstein):
All good points. Let's go through them:
{code:java}
The latest patch covers only collapse filter. I think we should also cover the
case where expand is specified w/o collapse(as it can cause NPE with grouping).
Since, issue is related to grouping behavior with collapse/expand, I think we
should handle both cases.{code}
Does SOLR-7798 fix the NPE with expand? Does grouping and expand have a
specific conflict? If it does then let's add the exception as you suggested.
{code:java}
The test added in SOLR-5230 was to check delegatingCollector's finish call
with grouping. Since, we are throwing error for collapse, We should use some
other component which uses delegatingCollector for testing. Maybe
significantTerms or some mock component. This is to ensure the finish call case
is covered in tests.{code}
I think the big problem now is not if finish() is being called, but whether we
can call the same PostFilter twice without resetting its internal state. This
is a bigger review problem which involves understanding the inner working of
each postfilter. So my suggestion is rather then putting in another token check
that, we address the whole problem of postfilters with grouping in a different
ticket (SOLR-14045) I planned addressing this ticket immediately after the 8.4
release is cut if not sooner.
{code:java}
I think we should also update the documentation for the same. {code}
+1, sorry if I missed this in the previous patch.
{code:java}
Some of the above things are covered in previous patch. If needed, we can
incorporate them into latest patch.
Joel Bernstein If you are okay with above things, I will attach new patch
incorporating those changes {code}
Sounds good! I'll check back later today and see how things are going.
.
> Collapse/Expand and Grouping are not designed to work together, we should
> fail requests that specify both gracefully
> --------------------------------------------------------------------------------------------------------------------
>
> Key: SOLR-13970
> URL: https://issues.apache.org/jira/browse/SOLR-13970
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Reporter: Erick Erickson
> Assignee: Munendra S N
> Priority: Major
> Attachments: SOLR-13970.patch, SOLR-13970.patch, SOLR-13970.patch
>
>
> Expand/Collapse was conceived as orthogonal to grouping, and there are odd
> interactions when both are specified. If these two options are specified, we
> should reject the query with an informative message.
> Shorter term, making this explicit in the documents would be helpful.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
