SOLR-15595: Partial results from shard queries needlessly discarded for queries without sort fields #267

makosten · 2021-08-18T22:36:57Z

https://issues.apache.org/jira/browse/SOLR-15595

Description

Partial shard results due to timeouts are not merged into the final query results. This is unnecessary for queries sorted only by score and/or docid.

Solution

No longer exit the shard merge early when partial results are returned for queries sorted just by score or docid.

Tests

Additional tests added for SortSpecParser for detecting that the sort only includes the score and/or docid.
Added unit test class for QueryComponent.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

solr/core/src/java/org/apache/solr/search/SortSpec.java

madrob · 2021-08-20T15:56:46Z

solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java

+          sortFieldValues = new NamedList<>();
+        }
+        // skip merging results for this shard if the sortSpec includes a non-scoredoc field but the sortFieldValues is empty.
+        if (thisResponseIsPartial && sortFieldValues.size() == 0 && ss.includesNonScoreOrDocField()) {


it's interesting to me that we do a similar check later on line 1122, not sure if we can combine or remove some of this logic.

The unmarshalSortValues method just returns an empty list if the sort if by score and/or docid and no other field. I could revise the mergeIds method to not call it for this case. The only thing that gives me pause is that unmarshalSortValues is protected, so presumably its being overridden someplace? It doesn't appear to be anywhere in the code base.

I'm adding skipping calling unmarshalSortValues if it is a no-op.

…. Refactor QueryComponent changes.

epugh · 2021-08-24T14:29:17Z

The test that @makosten added looks good to me, thoughts @madrob ?

madrob · 2021-08-24T14:31:51Z

solr/core/src/test/org/apache/solr/handler/component/QueryComponentPartialResultsTest.java

+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.*;


nit: no wildcard imports please

Removed import wildcard.

madrob · 2021-08-24T15:28:47Z

solr/core/src/test/org/apache/solr/handler/component/QueryComponentPartialResultsTest.java

+            solrDocument.addField("score", id * 1.1F);
+            solrDocument.addField(SORT_FIELD_NAME, id * 10);


Do these values actually matter? What if we made them constant. Or equal to id?

Used just the id since the values don't matter,

madrob · 2021-08-24T15:31:44Z

solr/core/src/test/org/apache/solr/handler/component/QueryComponentPartialResultsTest.java

+        assertEquals((shouldIncludePartialShardResult ? shard1Size : 0) + shard2Size, responseBuilder.getResponseDocs().size());
+    }
+
+    private static class MockResponseBuilder extends ResponseBuilder {


I almost feel like all the Mock* classes shouldn't be in here, but could have their own classes. It looks like the convention we use is to do inner Mock classes when they are nearly trivial and give them their own files otherwise.

Extracted the Mock classes.

The mock implementations are very bespoke to the new tests, but this way others can extend them.

madrob · 2021-08-24T20:48:04Z

solr/core/src/test/org/apache/solr/handler/component/QueryComponentPartialResultsTest.java

+        // shard 1 is marked partial
+        NamedList<Object> responseHeader1 = new NamedList<>();
+        responseHeader1.add(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY, Boolean.TRUE);
+
+        // shard 2 is not partial
+        NamedList<Object> responseHeader2 = new NamedList<>();
+
+        MockShardRequest shardRequest = MockShardRequest.create()
+                .withShardResponse(responseHeader1, createSolrDocumentList(shard1Size))
+                .withShardResponse(responseHeader2, createSolrDocumentList(shard2Size));


Can this be pulled out into a field that only gets created once?

madrob · 2021-08-24T20:49:42Z

solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java

+        boolean needsUnmarshalling = ss.includesNonScoreOrDocField();
+        // skip merging results for this shard if the sortSpec sort values need to be marshalled but the sortFieldValues is empty.
+        if (thisResponseIsPartial && sortFieldValues.size() == 0 && needsUnmarshalling) {
+          continue;
+        }
+        NamedList<List<Object>> unmarshalledSortFieldValues = needsUnmarshalling ? unmarshalSortValues(ss, sortFieldValues, schema) : new NamedList<>();


I feel like this needs more comments, or I'm being dense here. I have to mentally trace the code every time I come back to it and convince myself that it's right (which I'm sure it is because I trust you've done the testing, not because I actually understand it).

I added some comments. Hope they help.

They do! I am super happy with these, and they help me understand it much better than I thought I got it before.

…st and use a singleton for test ShardRequest.

madrob · 2021-08-25T21:54:29Z

solr/core/src/test/org/apache/solr/handler/component/MockSortSpecBuilder.java

@@ -0,0 +1,36 @@
+package org.apache.solr.handler.component;


license header

madrob · 2021-08-25T21:58:20Z

solr/core/src/test/org/apache/solr/handler/component/MockResponseBuilder.java

@@ -0,0 +1,54 @@
+package org.apache.solr.handler.component;


license header

madrob · 2021-08-25T21:59:10Z

solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java

+        boolean needsUnmarshalling = ss.includesNonScoreOrDocField();
+        // skip merging results for this shard if the sortSpec sort values need to be marshalled but the sortFieldValues is empty.
+        if (thisResponseIsPartial && sortFieldValues.size() == 0 && needsUnmarshalling) {
+          continue;
+        }
+        NamedList<List<Object>> unmarshalledSortFieldValues = needsUnmarshalling ? unmarshalSortValues(ss, sortFieldValues, schema) : new NamedList<>();


They do! I am super happy with these, and they help me understand it much better than I thought I got it before.

madrob

LGTM pending the two missing headers, and precommit/tests coming back happy

epugh · 2021-08-26T11:41:10Z

I just triggered the precommit check ;-)

epugh · 2021-08-27T10:35:43Z

What do we think @madrob @makosten ? Ready for merging? It would be nice to get this into 8.10! Do we need to update the ref guide?

madrob · 2021-09-02T15:23:18Z

LGTM, yes!

…or queries without sort fields (apache#267) * No longer skip merging partial shard results for non-sort queries * Add unit test for QueryComponent. Add javadoc for new SortSpec method. Refactor QueryComponent changes. * Improve javadocs. Fix parameter name in unit test class method. * Add comments to QueryComponent. Extract inner test class for added test and use a singleton for test ShardRequest.

No longer skip merging partial shard results for non-sort queries

3229bc9

madrob reviewed Aug 20, 2021

View reviewed changes

mkosten added 2 commits August 23, 2021 14:53

Add unit test for QueryComponent. Add javadoc for new SortSpec method…

30c2cdf

…. Refactor QueryComponent changes.

Improve javadocs. Fix parameter name in unit test class method.

306f3ce

madrob reviewed Aug 24, 2021

View reviewed changes

Add comments to QueryComponent. Extract inner test class for added te…

eb8de8a

…st and use a singleton for test ShardRequest.

madrob reviewed Aug 25, 2021

View reviewed changes

madrob approved these changes Aug 25, 2021

View reviewed changes

Add license at top of new files

75285a0

epugh@opensourceconnections.com and others added 2 commits September 3, 2021 17:57

track changes

c6f244d

Merge branch 'main' into solr-15595

46dee9c

epugh merged commit 812de21 into apache:main Sep 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-15595: Partial results from shard queries needlessly discarded for queries without sort fields #267

SOLR-15595: Partial results from shard queries needlessly discarded for queries without sort fields #267

makosten commented Aug 18, 2021 •

edited

madrob Aug 20, 2021

makosten Aug 23, 2021

makosten Aug 23, 2021

epugh commented Aug 24, 2021

madrob Aug 24, 2021

makosten Aug 24, 2021

madrob Aug 24, 2021

makosten Aug 24, 2021

madrob Aug 24, 2021

makosten Aug 24, 2021

makosten Aug 24, 2021

madrob Aug 24, 2021

makosten Aug 24, 2021

madrob Aug 24, 2021

makosten Aug 24, 2021

madrob Aug 25, 2021

madrob Aug 25, 2021

makosten Aug 25, 2021

madrob Aug 25, 2021

madrob Aug 25, 2021

madrob left a comment

epugh commented Aug 26, 2021

epugh commented Aug 27, 2021 •

edited

madrob commented Sep 2, 2021

		solrDocument.addField("score", id * 1.1F);
		solrDocument.addField(SORT_FIELD_NAME, id * 10);

SOLR-15595: Partial results from shard queries needlessly discarded for queries without sort fields #267

SOLR-15595: Partial results from shard queries needlessly discarded for queries without sort fields #267

Conversation

makosten commented Aug 18, 2021 • edited

Description

Solution

Tests

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epugh commented Aug 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madrob left a comment

Choose a reason for hiding this comment

epugh commented Aug 26, 2021

epugh commented Aug 27, 2021 • edited

madrob commented Sep 2, 2021

makosten commented Aug 18, 2021 •

edited

epugh commented Aug 27, 2021 •

edited