This is an automated email from the ASF dual-hosted git repository.
epugh pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/main by this push:
new 812de21 SOLR-15595: Partial results from shard queries needlessly
discarded for queries without sort fields (#267)
812de21 is described below
commit 812de21e75de018982788b9017261a719e1a7e8f
Author: Michael Kosten <[email protected]>
AuthorDate: Fri Sep 3 15:02:01 2021 -0700
SOLR-15595: Partial results from shard queries needlessly discarded for
queries without sort fields (#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.
---
solr/CHANGES.txt | 2 +
.../solr/handler/component/QueryComponent.java | 20 +++-
.../src/java/org/apache/solr/search/SortSpec.java | 23 ++++
.../handler/component/MockResponseBuilder.java | 70 ++++++++++++
.../solr/handler/component/MockShardRequest.java | 52 +++++++++
.../handler/component/MockSortSpecBuilder.java | 52 +++++++++
.../QueryComponentPartialResultsTest.java | 119 +++++++++++++++++++++
.../apache/solr/search/SortSpecParsingTest.java | 29 +++++
8 files changed, 363 insertions(+), 4 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 37e4f0b..45b68e7 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -446,6 +446,8 @@ Bug Fixes
* SOLR-14457: SolrClient leaks connections on compressed responses if the
response is malformed (Houston Putman)
+* SOLR-15595: Partial results from shard queries needlessly discarded for
queries without sort field (Michael Kosten via Mike Drob, Eric Pugh)
+
Other Changes
---------------------
* SOLR-15566: Clarify ref guide documentation about SQL queries with `SELECT
*` requiring a `LIMIT` clause (Timothy Potter)
diff --git
a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
index d89427a..241b966 100644
--- a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
+++ b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
@@ -955,11 +955,23 @@ public class QueryComponent extends SearchComponent
@SuppressWarnings("unchecked")
NamedList<List<Object>> sortFieldValues =
(NamedList<List<Object>>)(srsp.getSolrResponse().getResponse().get("sort_values"));
- if ((null == sortFieldValues || sortFieldValues.size()==0) && // we
bypass merging this response only if it's partial itself
- thisResponseIsPartial) { // but not the previous
one!!
- continue; //fsv timeout yields empty sort_vlaues
+ if (null == sortFieldValues) {
+ sortFieldValues = new NamedList<>();
}
- NamedList<List<Object>> unmarshalledSortFieldValues =
unmarshalSortValues(ss, sortFieldValues, schema);
+
+ // if the SortSpec contains a field besides score or the Lucene docid,
then the values will need to be unmarshalled from
+ // sortFieldValues.
+ boolean needsUnmarshalling = ss.includesNonScoreOrDocField();
+
+ // if we need to unmarshal the sortFieldValues for sorting but we have
none, which can happen if partial results are
+ // being returned from the shard, then skip merging the results for
the shard. This avoids an exception below.
+ // if the shard returned partial results but we don't need to
unmarshal (a normal scoring query), then merge what we got.
+ if (thisResponseIsPartial && sortFieldValues.size() == 0 &&
needsUnmarshalling) {
+ continue;
+ }
+
+ // Checking needsUnmarshalling saves on iterating the SortFields in
the SortSpec again.
+ NamedList<List<Object>> unmarshalledSortFieldValues =
needsUnmarshalling ? unmarshalSortValues(ss, sortFieldValues, schema) : new
NamedList<>();
// go through every doc in this response, construct a ShardDoc, and
// put it in the priority queue so it can be ordered.
diff --git a/solr/core/src/java/org/apache/solr/search/SortSpec.java
b/solr/core/src/java/org/apache/solr/search/SortSpec.java
index b79ed0a..afbfb36 100644
--- a/solr/core/src/java/org/apache/solr/search/SortSpec.java
+++ b/solr/core/src/java/org/apache/solr/search/SortSpec.java
@@ -77,6 +77,29 @@ public class SortSpec
}
/**
+ * Returns whether SortFields for the Sort includes a
+ * field besides SortField.Type.SCORE or SortField.Type.DOC
+ * @return true if SortFields contains a field besides score or the Lucene
doc
+ */
+ public boolean includesNonScoreOrDocField() {
+ return includesNonScoreOrDocField(sort);
+ }
+
+ /**
+ * Returns whether SortFields for the Sort includes a
+ * field besides SortField.Type.SCORE or SortField.Type.DOC
+ * @param sort org.apache.lucene.search.Sort
+ * @return true if SortFields contains a field besides score or the Lucene
doc
+ */
+ public static boolean includesNonScoreOrDocField(Sort sort) {
+ if (sort==null) return false;
+ for (SortField sf : sort.getSort()) {
+ if (sf.getType() != SortField.Type.SCORE && sf.getType() !=
SortField.Type.DOC) return true;
+ }
+ return false;
+ }
+
+ /**
* Gets the Lucene Sort object, or null for the default sort
* by score descending.
*/
diff --git
a/solr/core/src/test/org/apache/solr/handler/component/MockResponseBuilder.java
b/solr/core/src/test/org/apache/solr/handler/component/MockResponseBuilder.java
new file mode 100644
index 0000000..9ed7d50
--- /dev/null
+++
b/solr/core/src/test/org/apache/solr/handler/component/MockResponseBuilder.java
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler.component;
+
+import org.apache.solr.common.params.ShardParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.schema.IndexSchema;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.schema.StrField;
+import org.apache.solr.search.SortSpec;
+import org.mockito.Mockito;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class MockResponseBuilder extends ResponseBuilder {
+
+ private MockResponseBuilder(SolrQueryRequest request, SolrQueryResponse
response, List<SearchComponent> components) {
+ super(request, response, components);
+ }
+
+ public static MockResponseBuilder create() {
+
+ // the mocks
+ SolrQueryRequest request = Mockito.mock(SolrQueryRequest.class);
+ SolrQueryResponse response = Mockito.mock(SolrQueryResponse.class);
+ IndexSchema indexSchema = Mockito.mock(IndexSchema.class);
+ SolrParams params = Mockito.mock(SolrParams.class);
+
+ // SchemaField must be concrete due to field access
+ SchemaField uniqueIdField = new SchemaField("id", new StrField());
+
+ // we need this because QueryComponent adds a property to it.
+ NamedList<Object> responseHeader = new NamedList<>();
+
+ // the mock implementations
+ Mockito.when(request.getSchema()).thenReturn(indexSchema);
+
Mockito.when(indexSchema.getUniqueKeyField()).thenReturn(uniqueIdField);
+
Mockito.when(params.getBool(ShardParams.SHARDS_INFO)).thenReturn(false);
+ Mockito.when(request.getParams()).thenReturn(params);
+ Mockito.when(response.getResponseHeader()).thenReturn(responseHeader);
+
+ List<SearchComponent> components = new ArrayList<>();
+ return new MockResponseBuilder(request, response, components);
+
+ }
+
+ public MockResponseBuilder withSortSpec(SortSpec sortSpec) {
+ this.setSortSpec(sortSpec);
+ return this;
+ }
+
+}
diff --git
a/solr/core/src/test/org/apache/solr/handler/component/MockShardRequest.java
b/solr/core/src/test/org/apache/solr/handler/component/MockShardRequest.java
new file mode 100644
index 0000000..d33c4e2
--- /dev/null
+++ b/solr/core/src/test/org/apache/solr/handler/component/MockShardRequest.java
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler.component;
+
+import org.apache.solr.client.solrj.SolrResponse;
+import org.apache.solr.common.SolrDocumentList;
+import org.apache.solr.common.util.NamedList;
+import org.mockito.Mockito;
+
+import java.util.ArrayList;
+
+public class MockShardRequest extends ShardRequest {
+
+ public static MockShardRequest create() {
+ MockShardRequest mockShardRequest = new MockShardRequest();
+ mockShardRequest.responses = new ArrayList<>();
+ return mockShardRequest;
+ }
+
+ public MockShardRequest withShardResponse(NamedList<Object>
responseHeader, SolrDocumentList solrDocuments) {
+ ShardResponse shardResponse = buildShardResponse(responseHeader,
solrDocuments);
+ responses.add(shardResponse);
+ return this;
+ }
+
+ private ShardResponse buildShardResponse(NamedList<Object> responseHeader,
SolrDocumentList solrDocuments) {
+ SolrResponse solrResponse = Mockito.mock(SolrResponse.class);
+ ShardResponse shardResponse = new ShardResponse();
+ NamedList<Object> response = new NamedList<>();
+ response.add("response", solrDocuments);
+ shardResponse.setSolrResponse(solrResponse);
+ response.add("responseHeader", responseHeader);
+ Mockito.when(solrResponse.getResponse()).thenReturn(response);
+
+ return shardResponse;
+ }
+
+}
diff --git
a/solr/core/src/test/org/apache/solr/handler/component/MockSortSpecBuilder.java
b/solr/core/src/test/org/apache/solr/handler/component/MockSortSpecBuilder.java
new file mode 100644
index 0000000..3c65080
--- /dev/null
+++
b/solr/core/src/test/org/apache/solr/handler/component/MockSortSpecBuilder.java
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler.component;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.SortField;
+import org.apache.solr.search.SortSpec;
+import org.mockito.Mockito;
+
+public class MockSortSpecBuilder {
+ private final SortSpec sortSpec;
+
+ public MockSortSpecBuilder() {
+ this.sortSpec = Mockito.mock(SortSpec.class);
+ Mockito.when(sortSpec.getCount()).thenReturn(10);
+ }
+
+ public static MockSortSpecBuilder create() {
+ return new MockSortSpecBuilder();
+ }
+
+ public MockSortSpecBuilder withSortFields(SortField[] sortFields) {
+ Sort sort = Mockito.mock(Sort.class);
+ Mockito.when(sort.getSort()).thenReturn(sortFields);
+ Mockito.when(sortSpec.getSort()).thenReturn(sort);
+ return this;
+ }
+
+ public MockSortSpecBuilder withIncludesNonScoreOrDocSortField(boolean
include) {
+
Mockito.when(sortSpec.includesNonScoreOrDocField()).thenReturn(include);
+ return this;
+ }
+
+ public SortSpec build() {
+ return sortSpec;
+ }
+
+}
diff --git
a/solr/core/src/test/org/apache/solr/handler/component/QueryComponentPartialResultsTest.java
b/solr/core/src/test/org/apache/solr/handler/component/QueryComponentPartialResultsTest.java
new file mode 100644
index 0000000..952e720
--- /dev/null
+++
b/solr/core/src/test/org/apache/solr/handler/component/QueryComponentPartialResultsTest.java
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.handler.component;
+
+import org.apache.lucene.search.SortField;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrDocument;
+import org.apache.solr.common.SolrDocumentList;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.search.SortSpec;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class QueryComponentPartialResultsTest extends SolrTestCaseJ4 {
+ private static final String SORT_FIELD_NAME = "category";
+ private static final int shard1Size = 2;
+ private static final int shard2Size = 3;
+
+ private static int id = 0;
+ private static ShardRequest shardRequestWithPartialResults;
+
+ @BeforeClass
+ public static void setup() {
+ assumeWorkingMockito();
+ shardRequestWithPartialResults =
createShardRequestWithPartialResults();
+ }
+
+ @Test
+ public void includesPartialShardResultWhenUsingImplicitScoreSort() {
+ SortSpec sortSpec = MockSortSpecBuilder.create()
+ .withIncludesNonScoreOrDocSortField(false)
+ .build();
+ testPartialResultsForSortSpec(sortSpec, true);
+ }
+
+ @Test
+ public void includesPartialShardResultWhenUsingExplicitScoreSort() {
+ SortSpec sortSpec = MockSortSpecBuilder.create()
+ .withSortFields(new SortField[]{SortField.FIELD_SCORE})
+ .withIncludesNonScoreOrDocSortField(false)
+ .build();
+ testPartialResultsForSortSpec(sortSpec, true);
+ }
+
+ @Test
+ public void includesPartialShardResultWhenUsingExplicitDocSort() {
+ SortSpec sortSpec = MockSortSpecBuilder.create()
+ .withSortFields(new SortField[]{SortField.FIELD_DOC})
+ .withIncludesNonScoreOrDocSortField(false)
+ .build();
+ testPartialResultsForSortSpec(sortSpec, true);
+ }
+
+ @Test
+ public void excludesPartialShardResultWhenUsingNonScoreOrDocSortField() {
+ SortField sortField = new SortField(SORT_FIELD_NAME,
SortField.Type.INT);
+ SortSpec sortSpec = MockSortSpecBuilder.create()
+ .withSortFields(new SortField[]{sortField})
+ .withIncludesNonScoreOrDocSortField(true)
+ .build();
+ testPartialResultsForSortSpec(sortSpec, false);
+ }
+
+ private void testPartialResultsForSortSpec(SortSpec sortSpec, boolean
shouldIncludePartialShardResult) {
+
+ MockResponseBuilder responseBuilder =
MockResponseBuilder.create().withSortSpec(sortSpec);
+
+ QueryComponent queryComponent = new QueryComponent();
+ queryComponent.mergeIds(responseBuilder,
shardRequestWithPartialResults);
+
+ // do we have the expected document count?
+ // if results are not merged for the partial results shard, then the
total doc count will exclude them
+ if (shouldIncludePartialShardResult) {
+ assertEquals(shard1Size + shard2Size,
responseBuilder.getResponseDocs().size());
+ } else {
+ assertEquals(shard2Size, responseBuilder.getResponseDocs().size());
+ }
+ }
+
+ private static ShardRequest createShardRequestWithPartialResults() {
+ final NamedList<Object> shard1ResponseHeader = new NamedList<>();
+ final NamedList<Object> shard2ResponseHeader = new NamedList<>();
+
+ // the results from shard1 are marked partial
+
shard1ResponseHeader.add(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY,
Boolean.TRUE);
+
+ return MockShardRequest.create()
+ .withShardResponse(shard1ResponseHeader,
createSolrDocumentList(shard1Size))
+ .withShardResponse(shard2ResponseHeader,
createSolrDocumentList(shard2Size));
+ }
+
+ private static SolrDocumentList createSolrDocumentList(int size) {
+ SolrDocumentList solrDocuments = new SolrDocumentList();
+ for(int i = 0; i < size; i++) {
+ SolrDocument solrDocument = new SolrDocument();
+ solrDocument.addField("id", id++);
+ solrDocument.addField("score", (float)id);
+ solrDocument.addField(SORT_FIELD_NAME, id);
+ solrDocuments.add(solrDocument);
+ }
+ return solrDocuments;
+ }
+
+}
diff --git a/solr/core/src/test/org/apache/solr/search/SortSpecParsingTest.java
b/solr/core/src/test/org/apache/solr/search/SortSpecParsingTest.java
index a7eb5ed..25bcb7e 100644
--- a/solr/core/src/test/org/apache/solr/search/SortSpecParsingTest.java
+++ b/solr/core/src/test/org/apache/solr/search/SortSpecParsingTest.java
@@ -192,6 +192,35 @@ public class SortSpecParsingTest extends SolrTestCaseJ4 {
assertNotNull(spec);
assertNull(spec.getSort());
+ // test includesScore and includesNonScoreDocField methods
+ spec = doParseSortSpec("", req);
+ assertTrue(spec.includesScore());
+ assertFalse(spec.includesNonScoreOrDocField());
+
+ spec = doParseSortSpec("score desc", req);
+ assertTrue(spec.includesScore());
+ assertFalse(spec.includesNonScoreOrDocField());
+
+ spec = doParseSortSpec("score desc, _docid_ asc", req);
+ assertTrue(spec.includesScore());
+ assertFalse(spec.includesNonScoreOrDocField());
+
+ spec = doParseSortSpec("_docid_ desc", req);
+ assertFalse(spec.includesScore());
+ assertFalse(spec.includesNonScoreOrDocField());
+
+ spec = doParseSortSpec("weight desc", req);
+ assertFalse(spec.includesScore());
+ assertTrue(spec.includesNonScoreOrDocField());
+
+ spec = doParseSortSpec("weight desc, score desc", req);
+ assertTrue(spec.includesScore());
+ assertTrue(spec.includesNonScoreOrDocField());
+
+ spec = doParseSortSpec("weight desc, _docid_ desc", req);
+ assertFalse(spec.includesScore());
+ assertTrue(spec.includesNonScoreOrDocField());
+
req.close();
}
