ercsonusharma commented on code in PR #5117: URL: https://github.com/apache/solr/pull/5117#discussion_r4365869209
########## solr/core/src/test/org/apache/solr/handler/component/CombinedQueryComponentMergeTotalsTest.java: ########## @@ -0,0 +1,496 @@ +/* + * 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.params.ShardParams; +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 no shard's hits were left out. Fabricated shard responses are driven + * through {@code mergeIds} directly. + */ +public class CombinedQueryComponentMergeTotalsTest extends SolrTestCase { Review Comment: This test package already has `MockShardRequest`, `MockSortSpecBuilder` and `MockResponseBuilder` for this kind of test (see QueryComponentPartialResultsTest). Extending them would let this class drop its private helpers and the duplication that comes with them. Also, since `mergeIds` is called directly with fabricated shard responses, the core setup looks unnecessary IMO. The request, schema etc. can be mocked which would keep the class leaner. ########## changelog/unreleased/SOLR-18541-combined-query-merge-totals.yml: ########## @@ -0,0 +1,12 @@ +title: > + CombinedQueryComponent.mergeIds: a skipped partial shard no longer contributes Review Comment: Please use a user-facing title rather than the method name, e.g. "Combined queries: skipped or failed shard responses no longer add to numFound/approximateTotalHits and mark numFoundExact false; approximateTotalHits no longer double-counts across shards." ########## solr/core/src/test/org/apache/solr/handler/component/CombinedQueryComponentMergeTotalsTest.java: ########## @@ -0,0 +1,496 @@ +/* + * 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.params.ShardParams; +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 no shard's hits were left out. 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<Object> header, + SolrDocumentList combinedResponse, + List<SolrDocumentList> perQuery, + NamedList<List<Object>> sortValues0) { + NamedList<Object> 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<Object> header(Object... nameValues) { + NamedList<Object> 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<String> 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<List<Object>> sortValues = new NamedList<>(); + sortValues.add("intDefault", List.<Object>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() { Review Comment: Please drop this test as it duplicates previous `skippedPartialShardContributesNothing`. -- 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]
