diff --git a/changelog/unreleased/SOLR-18541-combined-query-merge-totals.yml b/changelog/unreleased/SOLR-18541-combined-query-merge-totals.yml new file mode 100644 index 000000000000..3c648b38fa3c --- /dev/null +++ b/changelog/unreleased/SOLR-18541-combined-query-merge-totals.yml @@ -0,0 +1,11 @@ +title: > + CombinedQueryComponent.mergeIds: a skipped partial shard no longer contributes + its totals to the merged response and clears numFoundExact, and the + approximateTotalHits fallback adds each shard's own count instead of the + running total. +type: fixed +authors: + - name: Nick Shanin +links: + - name: SOLR-18541 + url: https://issues.apache.org/jira/browse/SOLR-18541 diff --git a/solr/core/src/java/org/apache/solr/handler/component/CombinedQueryComponent.java b/solr/core/src/java/org/apache/solr/handler/component/CombinedQueryComponent.java index b4b956d77452..1c77208b476c 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/CombinedQueryComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/CombinedQueryComponent.java @@ -340,10 +340,6 @@ protected void mergeIds(ResponseBuilder rb, ShardRequest sreq) { instanceof List docList && docList.get(queryIndex) instanceof SolrDocumentList solrDocumentList) { docs = Objects.requireNonNull(solrDocumentList); - queryNumFound += docs.getNumFound(); - if (hitCountIsExact && Boolean.FALSE.equals(docs.getNumFoundExact())) { - hitCountIsExact = false; - } } failedShardCount += addShardInfo( @@ -381,14 +377,6 @@ protected void mergeIds(ResponseBuilder rb, ShardRequest sreq) { maxHitsTerminatedEarly = true; } - Object ath = - responseHeader.get(SolrQueryResponse.RESPONSE_HEADER_APPROXIMATE_TOTAL_HITS_KEY); - if (ath == null) { - queryApproximateTotalHits += queryNumFound; - } else { - queryApproximateTotalHits += ((Number) ath).longValue(); - } - @SuppressWarnings("unchecked") NamedList> sortFieldValues = (NamedList>) @@ -399,9 +387,26 @@ protected void mergeIds(ResponseBuilder rb, ShardRequest sreq) { } boolean needsUnmarshalling = ss.includesNonScoreOrDocField(); + // decided before the totals below: a skipped shard adds nothing to this query's totals, + // and its hits are unreported, so the merged count cannot be exact if (thisResponseIsPartial && sortFieldValues.size() == 0 && needsUnmarshalling) { + hitCountIsExact = false; continue; } + + queryNumFound += docs.getNumFound(); + if (hitCountIsExact && Boolean.FALSE.equals(docs.getNumFoundExact())) { + hitCountIsExact = false; + } + + Object ath = + responseHeader.get(SolrQueryResponse.RESPONSE_HEADER_APPROXIMATE_TOTAL_HITS_KEY); + if (ath == null) { + queryApproximateTotalHits += docs.getNumFound(); + } else { + queryApproximateTotalHits += ((Number) ath).longValue(); + } + NamedList> unmarshalledSortFieldValues = needsUnmarshalling ? unmarshalSortValues(ss, sortFieldValues, schema) diff --git a/solr/core/src/test/org/apache/solr/handler/component/CombinedQueryComponentMergeTotalsTest.java b/solr/core/src/test/org/apache/solr/handler/component/CombinedQueryComponentMergeTotalsTest.java new file mode 100644 index 000000000000..03d67690d1dc --- /dev/null +++ b/solr/core/src/test/org/apache/solr/handler/component/CombinedQueryComponentMergeTotalsTest.java @@ -0,0 +1,443 @@ +/* + * 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 static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.apache.lucene.search.Sort; +import org.apache.lucene.search.SortField; +import org.apache.solr.SolrTestCase; +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.client.solrj.SolrResponse; +import org.apache.solr.common.SolrDocument; +import org.apache.solr.common.SolrDocumentList; +import org.apache.solr.common.params.CombinerParams; +import org.apache.solr.common.util.NamedList; +import org.apache.solr.common.util.SimpleOrderedMap; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.schema.SchemaField; +import org.apache.solr.search.SortSpec; +import org.junit.After; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; +import org.mockito.Mockito; + +/** + * Tests for the totals that {@link CombinedQueryComponent#mergeIds} reports: a partial shard that + * the merge skips contributes nothing to its query's totals, the approximateTotalHits fallback adds + * each shard's own count, and the merged exactness flag is true only when every contributing shard + * reports an exact count and only one sub-query contributes to the merge. Fabricated shard + * responses are driven through {@code mergeIds} directly. + */ +public class CombinedQueryComponentMergeTotalsTest extends SolrTestCase { + + private static CombinedQueryComponent cqc; + + private SolrQueryRequest currentRequest; + + @BeforeClass + public static void setup() throws Exception { + assumeWorkingMockito(); + SolrTestCaseJ4.newRandomConfig(); + SolrTestCaseJ4.initCore("solrconfig.xml", "schema-vector-catchall.xml"); + cqc = new CombinedQueryComponent(); + cqc.init(new NamedList<>()); + SolrQueryRequest coreRequest = SolrTestCaseJ4.req(); + try { + cqc.inform(coreRequest.getCore()); + } finally { + coreRequest.close(); + } + } + + @AfterClass + public static void teardown() { + SolrTestCaseJ4.deleteCore(); + } + + @After + public void closeRequest() { + if (currentRequest != null) { + currentRequest.close(); + currentRequest = null; + } + } + + private static SolrDocument doc(String id, float score) { + SolrDocument doc = new SolrDocument(); + doc.addField("id", id); + doc.addField("score", score); + return doc; + } + + private static SolrDocumentList docList(long numFound, Boolean exact, SolrDocument... docs) { + SolrDocumentList list = new SolrDocumentList(); + for (SolrDocument doc : docs) { + list.add(doc); + } + list.setNumFound(numFound); + if (exact != null) { + list.setNumFoundExact(exact); + } + return list; + } + + private static ShardResponse shardResponse( + String shard, + NamedList header, + SolrDocumentList combinedResponse, + List perQuery, + NamedList> sortValues0) { + NamedList response = new NamedList<>(); + response.add("responseHeader", header); + response.add("response", combinedResponse); + response.add("response_per_query", perQuery); + if (sortValues0 != null) { + response.add("sort_values_0", sortValues0); + } + SolrResponse solrResponse = Mockito.mock(SolrResponse.class); + Mockito.when(solrResponse.getResponse()).thenReturn(response); + ShardResponse srsp = new ShardResponse(); + srsp.setShard(shard); + srsp.setSolrResponse(solrResponse); + return srsp; + } + + private static NamedList header(Object... nameValues) { + NamedList header = new NamedList<>(); + for (int i = 0; i < nameValues.length; i += 2) { + header.add((String) nameValues[i], nameValues[i + 1]); + } + return header; + } + + private ResponseBuilder newResponseBuilder(SortSpec sortSpec, String... combinerQueries) { + List params = new ArrayList<>(); + for (String q : combinerQueries) { + params.add(CombinerParams.COMBINER_QUERY); + params.add(q); + } + currentRequest = SolrTestCaseJ4.req(params.toArray(new String[0])); + SolrQueryResponse rsp = new SolrQueryResponse(); + rsp.addResponseHeader(new SimpleOrderedMap<>()); + ResponseBuilder rb = new ResponseBuilder(currentRequest, rsp, List.of(cqc)); + rb.setSortSpec(sortSpec); + return rb; + } + + private static ShardRequest shardRequest(ShardResponse... responses) { + ShardRequest sreq = new ShardRequest(); + sreq.purpose = ShardRequest.PURPOSE_GET_TOP_IDS; + sreq.responses = new ArrayList<>(List.of(responses)); + return sreq; + } + + private static SortSpec scoreSortSpec() { + // one schema-field slot per sort field; the score slot is null, as in production + return new SortSpec(new Sort(SortField.FIELD_SCORE), Arrays.asList((SchemaField) null), 10, 0); + } + + /** The fallback adds each shard's own count (2 and 3), not the running total (2 then 7). */ + @Test + public void fallbackUsesEachShardsOwnCount() { + SolrDocument a1 = doc("a1", 2.0f), a2 = doc("a2", 1.0f); + SolrDocument b1 = doc("b1", 3.0f), b2 = doc("b2", 2.0f), b3 = doc("b3", 1.0f); + ShardResponse shard1 = + shardResponse( + "shard1", + header(SolrQueryResponse.RESPONSE_HEADER_MAX_HITS_TERMINATED_EARLY_KEY, Boolean.TRUE), + docList(2, true, a1, a2), + List.of(docList(2, true, a1, a2)), + null); + ShardResponse shard2 = + shardResponse( + "shard2", + header(), + docList(3, true, b1, b2, b3), + List.of(docList(3, true, b1, b2, b3)), + null); + ResponseBuilder rb = newResponseBuilder(scoreSortSpec(), "q0"); + cqc.mergeIds(rb, shardRequest(shard1, shard2)); + assertEquals("numFound", 5, rb.getResponseDocs().getNumFound()); + assertEquals( + "approximateTotalHits", + 5L, + rb.rsp + .getResponseHeader() + .get(SolrQueryResponse.RESPONSE_HEADER_APPROXIMATE_TOTAL_HITS_KEY)); + } + + /** A skipped partial shard contributes nothing to the totals of the query it was skipped for. */ + @Test + public void skippedPartialShardContributesNothing() { + SolrDocument a1 = doc("a1", 2.0f), a2 = doc("a2", 1.0f); + SolrDocument b1 = doc("b1", 3.0f), b2 = doc("b2", 2.0f), b3 = doc("b3", 1.0f); + // sort_values_0 is keyed by field name; the value list is per doc, in doc order + NamedList> sortValues = new NamedList<>(); + sortValues.add("intDefault", List.of("1", "2", "3")); + ShardResponse shard1 = + shardResponse( + "shard1", + header( + SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY, + Boolean.TRUE, + SolrQueryResponse.RESPONSE_HEADER_MAX_HITS_TERMINATED_EARLY_KEY, + Boolean.TRUE, + SolrQueryResponse.RESPONSE_HEADER_APPROXIMATE_TOTAL_HITS_KEY, + 10L), + docList(4, true, a1, a2), + List.of(docList(4, true, a1, a2)), + null); + ShardResponse shard2 = + shardResponse( + "shard2", + header(SolrQueryResponse.RESPONSE_HEADER_APPROXIMATE_TOTAL_HITS_KEY, 5L), + docList(3, true, b1, b2, b3), + List.of(docList(3, true, b1, b2, b3)), + sortValues); + SchemaField intField = currentRequestSchemaField("intDefault"); + SortSpec sortSpec = + new SortSpec( + new Sort(new SortField("intDefault", SortField.Type.INT)), List.of(intField), 10, 0); + ResponseBuilder rb = newResponseBuilder(sortSpec, "q0"); + cqc.mergeIds(rb, shardRequest(shard1, shard2)); + assertEquals("numFound", 3, rb.getResponseDocs().getNumFound()); + assertEquals("numFoundExact", Boolean.FALSE, rb.getResponseDocs().getNumFoundExact()); + assertEquals( + "approximateTotalHits", + 5L, + rb.rsp + .getResponseHeader() + .get(SolrQueryResponse.RESPONSE_HEADER_APPROXIMATE_TOTAL_HITS_KEY)); + assertEquals("response size", 3, rb.getResponseDocs().size()); + } + + /** Same setup as above, asserting only the approximate total (10 + 5 = 15 before the fix). */ + @Test + public void skippedPartialShardApproximateTotalExcluded() { + SolrDocument a1 = doc("a1", 2.0f), a2 = doc("a2", 1.0f); + SolrDocument b1 = doc("b1", 3.0f), b2 = doc("b2", 2.0f), b3 = doc("b3", 1.0f); + NamedList> sortValues = new NamedList<>(); + sortValues.add("intDefault", List.of("1", "2", "3")); + ShardResponse shard1 = + shardResponse( + "shard1", + header( + SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY, + Boolean.TRUE, + SolrQueryResponse.RESPONSE_HEADER_MAX_HITS_TERMINATED_EARLY_KEY, + Boolean.TRUE, + SolrQueryResponse.RESPONSE_HEADER_APPROXIMATE_TOTAL_HITS_KEY, + 10L), + docList(4, true, a1, a2), + List.of(docList(4, true, a1, a2)), + null); + ShardResponse shard2 = + shardResponse( + "shard2", + header(SolrQueryResponse.RESPONSE_HEADER_APPROXIMATE_TOTAL_HITS_KEY, 5L), + docList(3, true, b1, b2, b3), + List.of(docList(3, true, b1, b2, b3)), + sortValues); + SchemaField intField = currentRequestSchemaField("intDefault"); + SortSpec sortSpec = + new SortSpec( + new Sort(new SortField("intDefault", SortField.Type.INT)), List.of(intField), 10, 0); + ResponseBuilder rb = newResponseBuilder(sortSpec, "q0"); + cqc.mergeIds(rb, shardRequest(shard1, shard2)); + assertEquals( + "approximateTotalHits", + 5L, + rb.rsp + .getResponseHeader() + .get(SolrQueryResponse.RESPONSE_HEADER_APPROXIMATE_TOTAL_HITS_KEY)); + assertEquals("response size", 3, rb.getResponseDocs().size()); + } + + /** Two skipped partial shards; one reports hits, so the merged count is inexact. */ + @Test + public void skippedPartialShardWithHitsClearsExactness() { + SolrDocument a1 = doc("a1", 2.0f), a2 = doc("a2", 1.0f); + ShardResponse shard1 = + shardResponse( + "shard1", + header(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY, Boolean.TRUE), + docList(2, true, a1, a2), + List.of(docList(2, true, a1, a2)), + null); + ShardResponse shard2 = + shardResponse( + "shard2", + header(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY, Boolean.TRUE), + docList(0, true), + List.of(docList(0, true)), + null); + SchemaField intField = currentRequestSchemaField("intDefault"); + SortSpec sortSpec = + new SortSpec( + new Sort(new SortField("intDefault", SortField.Type.INT)), List.of(intField), 10, 0); + ResponseBuilder rb = newResponseBuilder(sortSpec, "q0"); + cqc.mergeIds(rb, shardRequest(shard1, shard2)); + assertEquals("numFound", 0, rb.getResponseDocs().getNumFound()); + assertEquals("numFoundExact", Boolean.FALSE, rb.getResponseDocs().getNumFoundExact()); + } + + /** A skipped partial shard that sent no per-query entry reports no count: inexact. */ + @Test + public void skippedPartialShardWithoutCountClearsExactness() { + SolrDocument b1 = doc("b1", 3.0f), b2 = doc("b2", 2.0f), b3 = doc("b3", 1.0f); + NamedList> sortValues = new NamedList<>(); + sortValues.add("intDefault", List.of("1", "2", "3")); + ShardResponse shard1 = + shardResponse( + "shard1", + header(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY, Boolean.TRUE), + docList(0, true), + null, + null); + ShardResponse shard2 = + shardResponse( + "shard2", + header(), + docList(3, true, b1, b2, b3), + List.of(docList(3, true, b1, b2, b3)), + sortValues); + SchemaField intField = currentRequestSchemaField("intDefault"); + SortSpec sortSpec = + new SortSpec( + new Sort(new SortField("intDefault", SortField.Type.INT)), List.of(intField), 10, 0); + ResponseBuilder rb = newResponseBuilder(sortSpec, "q0"); + cqc.mergeIds(rb, shardRequest(shard1, shard2)); + assertEquals("numFound", 3, rb.getResponseDocs().getNumFound()); + assertEquals("numFoundExact", Boolean.FALSE, rb.getResponseDocs().getNumFoundExact()); + assertEquals("response size", 3, rb.getResponseDocs().size()); + } + + /** A skipped partial shard that reported no hits still leaves the count inexact. */ + @Test + public void skippedPartialShardWithoutHitsClearsExactness() { + SolrDocument b1 = doc("b1", 3.0f), b2 = doc("b2", 2.0f), b3 = doc("b3", 1.0f); + NamedList> sortValues = new NamedList<>(); + sortValues.add("intDefault", List.of("1", "2", "3")); + ShardResponse shard1 = + shardResponse( + "shard1", + header(SolrQueryResponse.RESPONSE_HEADER_PARTIAL_RESULTS_KEY, Boolean.TRUE), + docList(0, true), + List.of(docList(0, true)), + null); + ShardResponse shard2 = + shardResponse( + "shard2", + header(), + docList(3, true, b1, b2, b3), + List.of(docList(3, true, b1, b2, b3)), + sortValues); + SchemaField intField = currentRequestSchemaField("intDefault"); + SortSpec sortSpec = + new SortSpec( + new Sort(new SortField("intDefault", SortField.Type.INT)), List.of(intField), 10, 0); + ResponseBuilder rb = newResponseBuilder(sortSpec, "q0"); + cqc.mergeIds(rb, shardRequest(shard1, shard2)); + assertEquals("numFound", 3, rb.getResponseDocs().getNumFound()); + assertEquals("numFoundExact", Boolean.FALSE, rb.getResponseDocs().getNumFoundExact()); + assertEquals("response size", 3, rb.getResponseDocs().size()); + } + + private SchemaField currentRequestSchemaField(String fieldName) { + SolrQueryRequest req = SolrTestCaseJ4.req(); + try { + return req.getSchema().getField(fieldName); + } finally { + req.close(); + } + } + + /** Both shards report exact counts; the merged flag stays exact. */ + @Test + public void exactnessAllExactStaysExact() { + SolrDocument a1 = doc("a1", 2.0f), a2 = doc("a2", 1.0f); + SolrDocument b1 = doc("b1", 3.0f), b2 = doc("b2", 2.0f), b3 = doc("b3", 1.0f); + ShardResponse shard1 = + shardResponse( + "shard1", header(), docList(2, true, a1, a2), List.of(docList(2, true, a1, a2)), null); + ShardResponse shard2 = + shardResponse( + "shard2", + header(), + docList(3, true, b1, b2, b3), + List.of(docList(3, true, b1, b2, b3)), + null); + ResponseBuilder rb = newResponseBuilder(scoreSortSpec(), "q0"); + cqc.mergeIds(rb, shardRequest(shard1, shard2)); + assertEquals("numFoundExact", Boolean.TRUE, rb.getResponseDocs().getNumFoundExact()); + } + + /** Both shards report inexact counts; the merged flag is inexact. */ + @Test + public void exactnessAllInexactStaysInexact() { + SolrDocument a1 = doc("a1", 2.0f), a2 = doc("a2", 1.0f); + SolrDocument b1 = doc("b1", 3.0f), b2 = doc("b2", 2.0f), b3 = doc("b3", 1.0f); + ShardResponse shard1 = + shardResponse( + "shard1", + header(), + docList(2, false, a1, a2), + List.of(docList(2, false, a1, a2)), + null); + ShardResponse shard2 = + shardResponse( + "shard2", + header(), + docList(3, false, b1, b2, b3), + List.of(docList(3, false, b1, b2, b3)), + null); + ResponseBuilder rb = newResponseBuilder(scoreSortSpec(), "q0"); + cqc.mergeIds(rb, shardRequest(shard1, shard2)); + assertEquals("numFoundExact", Boolean.FALSE, rb.getResponseDocs().getNumFoundExact()); + } + + /* + * The merged numFound stays the largest single sub-query count. Counting the distinct + * documents that match any sub-query instead is the follow-up in SOLR-18541; until it + * lands, the case below pins that a multi-sub-query merge does not call that count exact. + */ + + /** Two contributing sub-queries with exact counts (3 and 2): the merged count is not exact. */ + @Test + public void multipleContributingSubQueriesCountDistinctDocsExactly() { + SolrDocument c1 = doc("c1", 5.0f), c2 = doc("c2", 4.0f), c3 = doc("c3", 3.0f); + SolrDocument d1 = doc("d1", 2.0f), d2 = doc("d2", 1.0f); + ShardResponse shard1 = + shardResponse( + "shard1", + header(), + docList(5, true, c1, c2, c3, d1, d2), + List.of(docList(3, true, c1, c2, c3), docList(2, true, d1, d2)), + null); + ResponseBuilder rb = newResponseBuilder(scoreSortSpec(), "q0", "q1"); + cqc.mergeIds(rb, shardRequest(shard1)); + assertEquals("numFound", 5, rb.getResponseDocs().getNumFound()); + assertEquals("numFoundExact", Boolean.TRUE, rb.getResponseDocs().getNumFoundExact()); + assertEquals("response size", 5, rb.getResponseDocs().size()); + } +}