dsmiley commented on code in PR #3418:
URL: https://github.com/apache/solr/pull/3418#discussion_r2318929806
##########
solr/core/src/java/org/apache/solr/handler/component/FacetComponent.java:
##########
@@ -82,6 +82,14 @@ public void prepare(ResponseBuilder rb) throws IOException {
rb.setNeedDocSet(true);
rb.doFacets = true;
+ if (rb instanceof CombinedQueryResponseBuilder crb) {
+ crb.responseBuilders.forEach(
+ thisRb -> {
+ thisRb.setNeedDocSet(true);
+ thisRb.doFacets = true;
+ });
+ }
+
Review Comment:
This is unfortunate.
Could `CombinedQueryResponseBuilder.setNeedDocSet` be overridden to
propagate to children? That leaves `doFacets`; maybe a setter cold be added
similarly.
OR,
Maybe CombinedQueryComponent could at some point after the other components
to their preparation, it could propagate settings from parent to children using
new code to do so on CombinedQueryResponseBuilder.
I recognize ResponseBuilder is a sloppy mess of entanglement. I don't think
it has ever been cleaned up and re-thought out.
##########
solr/core/src/java/org/apache/solr/handler/component/FacetComponent.java:
##########
@@ -82,6 +82,14 @@ public void prepare(ResponseBuilder rb) throws IOException {
rb.setNeedDocSet(true);
rb.doFacets = true;
+ if (rb instanceof CombinedQueryResponseBuilder crb) {
+ crb.responseBuilders.forEach(
+ thisRb -> {
+ thisRb.setNeedDocSet(true);
+ thisRb.doFacets = true;
+ });
+ }
+
Review Comment:
I could imagine ResponseBuilder having a method like `copyTo(ResponseBuilder
other)`. Albeit certain fields relate to the query indirectly, and that's what
varies across the queries you are combining. The CombinedQueryResponseBuilder
could arrange for saving those, and reinstating them after copying them.
##########
solr/core/src/java/org/apache/solr/handler/component/HighlightComponent.java:
##########
@@ -97,6 +97,13 @@ public void prepare(ResponseBuilder rb) throws IOException {
rb.doHighlights = solrConfigHighlighter.isHighlightingEnabled(params);
if (rb.doHighlights) {
rb.setNeedDocList(true);
+ if (rb instanceof CombinedQueryResponseBuilder crb) {
Review Comment:
same feedback as for facets
##########
solr/core/src/java/org/apache/solr/handler/component/HighlightComponent.java:
##########
@@ -97,6 +97,13 @@ public void prepare(ResponseBuilder rb) throws IOException {
rb.doHighlights = solrConfigHighlighter.isHighlightingEnabled(params);
if (rb.doHighlights) {
rb.setNeedDocList(true);
+ if (rb instanceof CombinedQueryResponseBuilder crb) {
Review Comment:
And there are other components; for some reason these two alone have your
attention but a number of them use the ResponseBuilder.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
