[8.x] Only aggregations require at least one shard request (#115314) (#115794)

piergm · elasticmachine · web-flow · commit fa7ac36f9c4a · 2024-12-05T10:32:26.000+01:00
* Only aggregations require at least one shard request (#115314) * unskipping shards only when aggs * Update docs/changelog/115314.yaml * fixed more tests * null check for searchRequest.source() (cherry picked from commit 7f573c6) * applying #115774 * skipped test * fixed test --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
diff --git a/docs/changelog/115314.yaml b/docs/changelog/115314.yaml
@@ -0,0 +1,5 @@
+pr: 115314
+summary: Only aggregations require at least one shard request
+area: Search
+type: enhancement
+issues: []
diff --git a/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBIndexingIT.java b/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBIndexingIT.java
@@ -412,7 +412,7 @@ public void testSkippingShards() throws Exception {
             assertResponse(client().search(searchRequest), searchResponse -> {
                 ElasticsearchAssertions.assertNoSearchHits(searchResponse);
                 assertThat(searchResponse.getTotalShards(), equalTo(2));
-                assertThat(searchResponse.getSkippedShards(), equalTo(1));
+                assertThat(searchResponse.getSkippedShards(), equalTo(2));
                 assertThat(searchResponse.getSuccessfulShards(), equalTo(2));
             });
         }
diff --git a/qa/multi-cluster-search/src/test/java/org/elasticsearch/search/CCSDuelIT.java b/qa/multi-cluster-search/src/test/java/org/elasticsearch/search/CCSDuelIT.java
@@ -43,6 +43,7 @@
 import org.elasticsearch.rest.action.search.RestSearchAction;
 import org.elasticsearch.script.Script;
 import org.elasticsearch.script.ScriptType;
+import org.elasticsearch.search.aggregations.AggregationBuilders;
 import org.elasticsearch.search.aggregations.BucketOrder;
 import org.elasticsearch.search.aggregations.bucket.filter.FilterAggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder;
@@ -580,13 +581,14 @@ public void testSortByField() throws Exception {
 
     public void testSortByFieldOneClusterHasNoResults() throws Exception {
         assumeMultiClusterSetup();
-        // set to a value greater than the number of shards to avoid differences due to the skipping of shards
+        // setting aggs to avoid differences due to the skipping of shards when matching none
         SearchSourceBuilder sourceBuilder = new SearchSourceBuilder();
         boolean onlyRemote = randomBoolean();
         sourceBuilder.query(new TermQueryBuilder("_index", onlyRemote ? REMOTE_INDEX_NAME : INDEX_NAME));
         sourceBuilder.sort("type.keyword", SortOrder.ASC);
         sourceBuilder.sort("creationDate", SortOrder.DESC);
         sourceBuilder.sort("user.keyword", SortOrder.ASC);
+        sourceBuilder.aggregation(AggregationBuilders.max("max").field("creationDate"));
         CheckedConsumer<ObjectPath, IOException> responseChecker = response -> {
             assertHits(response);
             int size = response.evaluateArraySize("hits.hits");
diff --git a/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/70_skip_shards.yml b/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/70_skip_shards.yml
@@ -166,8 +166,7 @@
   - match: { hits.total.value: 0 }
   - match: { _shards.total: 2 }
   - match: { _shards.successful: 2 }
-    # When all shards are skipped current logic returns 1 to produce a valid search result
-  - match: { _shards.skipped : 1}
+  - match: { _shards.skipped : 2}
   - match: { _shards.failed: 0 }
 
   # check that skipped when we don't match the alias with a terms query
@@ -183,8 +182,7 @@
   - match: { hits.total.value: 0 }
   - match: { _shards.total: 2 }
   - match: { _shards.successful: 2 }
-    # When all shards are skipped current logic returns 1 to produce a valid search result
-  - match: { _shards.skipped : 1}
+  - match: { _shards.skipped : 2}
   - match: { _shards.failed: 0 }
 
   # check that skipped when we don't match the alias with a prefix query
@@ -200,8 +198,7 @@
   - match: { hits.total.value: 0 }
   - match: { _shards.total: 2 }
   - match: { _shards.successful: 2 }
-    # When all shards are skipped current logic returns 1 to produce a valid search result
-  - match: { _shards.skipped : 1}
+  - match: { _shards.skipped : 2}
   - match: { _shards.failed: 0 }
 
   # check that skipped when we don't match the alias with a wildcard query
@@ -217,7 +214,6 @@
   - match: { hits.total.value: 0 }
   - match: { _shards.total: 2 }
   - match: { _shards.successful: 2 }
-    # When all shards are skipped current logic returns 1 to produce a valid search result
-  - match: { _shards.skipped : 1}
+  - match: { _shards.skipped : 2}
   - match: { _shards.failed: 0 }
 
diff --git a/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/90_index_name_query.yml b/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/90_index_name_query.yml
@@ -81,7 +81,7 @@ teardown:
   - match: { hits.total.value: 0 }
   - match: { _shards.total: 2 }
   - match: { _shards.successful: 2 }
-  - match: { _shards.skipped : 1}
+  - match: { _shards.skipped : 2}
   - match: { _shards.failed: 0 }
 
   - do:
@@ -98,5 +98,5 @@ teardown:
   - match: { hits.total.value: 0 }
   - match: { _shards.total: 2 }
   - match: { _shards.successful: 2 }
-  - match: { _shards.skipped : 1}
+  - match: { _shards.skipped : 2}
   - match: { _shards.failed: 0 }
diff --git a/rest-api-spec/build.gradle b/rest-api-spec/build.gradle
@@ -86,6 +86,7 @@ tasks.named("yamlRestTestV7CompatTransform").configure { task ->
   task.skipTest("search/240_date_nanos/doc value fields are working as expected across date and date_nanos fields", "Fetching docvalues field multiple times is no longer allowed")
   task.skipTest("search/110_field_collapsing/field collapsing and rescore", "#107779 Field collapsing is compatible with rescore in 8.15")
   task.skipTest("indices.create/11_basic_with_types/Create index with mappings", "Empty mapping creation has changed")
+  task.skipTest("search/140_pre_filter_search_shards/pre_filter_shard_size with shards that have no hit", "#115314 we skipp all shards unless the query has aggs, therefore the _shard.skipped do not match")
 
   task.replaceValueInMatch("_type", "_doc")
   task.addAllowedWarningRegex("\\[types removal\\].*")
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/ccs/CrossClusterSearchIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/ccs/CrossClusterSearchIT.java
@@ -214,7 +214,7 @@ public void testCCSClusterDetailsWhereAllShardsSkippedInCanMatch() throws Except
                 // with DFS_QUERY_THEN_FETCH, the local shards are never skipped
                 assertThat(localClusterSearchInfo.getSkippedShards(), equalTo(0));
             } else {
-                assertThat(localClusterSearchInfo.getSkippedShards(), equalTo(localNumShards - 1));
+                assertThat(localClusterSearchInfo.getSkippedShards(), equalTo(localNumShards));
             }
             assertThat(localClusterSearchInfo.getFailedShards(), equalTo(0));
             assertThat(localClusterSearchInfo.getFailures().size(), equalTo(0));
@@ -224,7 +224,7 @@ public void testCCSClusterDetailsWhereAllShardsSkippedInCanMatch() throws Except
             assertThat(remoteClusterSearchInfo.getTotalShards(), equalTo(remoteNumShards));
             assertThat(remoteClusterSearchInfo.getSuccessfulShards(), equalTo(remoteNumShards));
             if (clusters.isCcsMinimizeRoundtrips()) {
-                assertThat(remoteClusterSearchInfo.getSkippedShards(), equalTo(remoteNumShards - 1));
+                assertThat(remoteClusterSearchInfo.getSkippedShards(), equalTo(remoteNumShards));
             } else {
                 assertThat(remoteClusterSearchInfo.getSkippedShards(), equalTo(remoteNumShards));
             }
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/profile/query/QueryProfilerIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/profile/query/QueryProfilerIT.java
@@ -68,7 +68,11 @@ public void testProfileQuery() throws Exception {
                 prepareSearch().setQuery(q).setTrackTotalHits(true).setProfile(true).setSearchType(SearchType.QUERY_THEN_FETCH),
                 response -> {
                     assertNotNull("Profile response element should not be null", response.getProfileResults());
-                    assertThat("Profile response should not be an empty array", response.getProfileResults().size(), not(0));
+                    if (response.getSkippedShards() == response.getSuccessfulShards()) {
+                        assertEquals(0, response.getProfileResults().size());
+                    } else {
+                        assertThat("Profile response should not be an empty array", response.getProfileResults().size(), not(0));
+                    }
                     for (Map.Entry<String, SearchProfileShardResult> shard : response.getProfileResults().entrySet()) {
                         for (QueryProfileShardResult searchProfiles : shard.getValue().getQueryProfileResults()) {
                             for (ProfileResult result : searchProfiles.getQueryResults()) {
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/stats/FieldUsageStatsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/stats/FieldUsageStatsIT.java
@@ -158,11 +158,15 @@ public void testFieldUsageStats() throws ExecutionException, InterruptedExceptio
 
         assertTrue(stats.hasField("date_field"));
         assertEquals(Set.of(UsageContext.POINTS), stats.get("date_field").keySet());
-        // can_match does not enter search stats
-        // there is a special case though where we have no hit but we need to get at least one search response in order
-        // to produce a valid search result with all the aggs etc., so we hit one of the two shards
+
+        long expectedShards = 2L * numShards;
+        if (numShards == 1) {
+            // with 1 shard and setPreFilterShardSize(1) we don't perform can_match phase but instead directly query the shard
+            expectedShards += 1;
+        }
+
         assertEquals(
-            (2 * numShards) + 1,
+            expectedShards,
             indicesAdmin().prepareStats("test")
                 .clear()
                 .setSearch(true)
diff --git a/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java b/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
@@ -1457,6 +1457,8 @@ public SearchPhase newSearchPhase(
             SearchResponse.Clusters clusters
         ) {
             if (preFilter) {
+                // only for aggs we need to contact shards even if there are no matches
+                boolean requireAtLeastOneMatch = searchRequest.source() != null && searchRequest.source().aggregations() != null;
                 return new CanMatchPreFilterSearchPhase(
                     logger,
                     searchTransportService,
@@ -1468,7 +1470,7 @@ public SearchPhase newSearchPhase(
                     shardIterators,
                     timeProvider,
                     task,
-                    true,
+                    requireAtLeastOneMatch,
                     searchService.getCoordinatorRewriteContextProvider(timeProvider::absoluteStartMillis),
                     listener.delegateFailureAndWrap(
                         (l, iters) -> newSearchPhase(
diff --git a/x-pack/plugin/async-search/src/internalClusterTest/java/org/elasticsearch/xpack/search/CrossClusterAsyncSearchIT.java b/x-pack/plugin/async-search/src/internalClusterTest/java/org/elasticsearch/xpack/search/CrossClusterAsyncSearchIT.java
@@ -274,6 +274,8 @@ public void testCCSClusterDetailsWhereAllShardsSkippedInCanMatch() throws Except
         boolean dfs = randomBoolean();
         if (dfs) {
             request.getSearchRequest().searchType(SearchType.DFS_QUERY_THEN_FETCH);
+        } else {
+            request.getSearchRequest().searchType(SearchType.QUERY_THEN_FETCH);
         }
         RangeQueryBuilder rangeQueryBuilder = new RangeQueryBuilder("@timestamp").from(100).to(2000);
         request.getSearchRequest().source(new SearchSourceBuilder().query(rangeQueryBuilder).size(10));
@@ -285,23 +287,32 @@ public void testCCSClusterDetailsWhereAllShardsSkippedInCanMatch() throws Except
         try {
             responseId = response.getId();
             assertNotNull(response.getSearchResponse());
-            assertTrue(response.isRunning());
             SearchResponse.Clusters clusters = response.getSearchResponse().getClusters();
             assertThat(clusters.getTotal(), equalTo(2));
-            assertTrue("search cluster results should be marked as partial", clusters.hasPartialResults());
-
+            if (dfs) {
+                assertTrue("search cluster results should be marked as partial", clusters.hasPartialResults());
+            } else {
+                assertFalse(
+                    "search cluster results should not be marked as partial as all shards are skipped",
+                    clusters.hasPartialResults()
+                );
+            }
             SearchResponse.Cluster localClusterSearchInfo = clusters.getCluster(RemoteClusterAware.LOCAL_CLUSTER_GROUP_KEY);
             assertNotNull(localClusterSearchInfo);
-            assertThat(localClusterSearchInfo.getStatus(), equalTo(SearchResponse.Cluster.Status.RUNNING));
+            if (dfs) {
+                assertThat(localClusterSearchInfo.getStatus(), equalTo(SearchResponse.Cluster.Status.RUNNING));
+            } else {
+                assertThat(localClusterSearchInfo.getStatus(), equalTo(SearchResponse.Cluster.Status.SUCCESSFUL));
+            }
 
             SearchResponse.Cluster remoteClusterSearchInfo = clusters.getCluster(REMOTE_CLUSTER);
             assertNotNull(remoteClusterSearchInfo);
-            assertThat(localClusterSearchInfo.getStatus(), equalTo(SearchResponse.Cluster.Status.RUNNING));
         } finally {
             response.decRef();
         }
-
-        SearchListenerPlugin.waitSearchStarted();
+        if (dfs) {
+            SearchListenerPlugin.waitSearchStarted();
+        }
         SearchListenerPlugin.allowQueryPhase();
 
         waitForSearchTasksToFinish();
@@ -331,7 +342,7 @@ public void testCCSClusterDetailsWhereAllShardsSkippedInCanMatch() throws Except
                 // no skipped shards locally when DFS_QUERY_THEN_FETCH is used
                 assertThat(localClusterSearchInfo.getSkippedShards(), equalTo(0));
             } else {
-                assertThat(localClusterSearchInfo.getSkippedShards(), equalTo(localNumShards - 1));
+                assertThat(localClusterSearchInfo.getSkippedShards(), equalTo(localNumShards));
             }
             assertThat(localClusterSearchInfo.getFailedShards(), equalTo(0));
             assertThat(localClusterSearchInfo.getFailures().size(), equalTo(0));
@@ -341,7 +352,7 @@ public void testCCSClusterDetailsWhereAllShardsSkippedInCanMatch() throws Except
             assertThat(remoteClusterSearchInfo.getTotalShards(), equalTo(remoteNumShards));
             assertThat(remoteClusterSearchInfo.getSuccessfulShards(), equalTo(remoteNumShards));
             if (minimizeRoundtrips) {
-                assertThat(remoteClusterSearchInfo.getSkippedShards(), equalTo(remoteNumShards - 1));
+                assertThat(remoteClusterSearchInfo.getSkippedShards(), equalTo(remoteNumShards));
             } else {
                 assertThat(remoteClusterSearchInfo.getSkippedShards(), equalTo(remoteNumShards));
             }
@@ -377,7 +388,7 @@ public void testCCSClusterDetailsWhereAllShardsSkippedInCanMatch() throws Except
                 // no skipped shards locally when DFS_QUERY_THEN_FETCH is used
                 assertThat(localClusterSearchInfo.getSkippedShards(), equalTo(0));
             } else {
-                assertThat(localClusterSearchInfo.getSkippedShards(), equalTo(localNumShards - 1));
+                assertThat(localClusterSearchInfo.getSkippedShards(), equalTo(localNumShards));
             }
             assertThat(localClusterSearchInfo.getFailedShards(), equalTo(0));
             assertThat(localClusterSearchInfo.getFailures().size(), equalTo(0));
@@ -387,7 +398,7 @@ public void testCCSClusterDetailsWhereAllShardsSkippedInCanMatch() throws Except
             assertThat(remoteClusterSearchInfo.getTotalShards(), equalTo(remoteNumShards));
             assertThat(remoteClusterSearchInfo.getSuccessfulShards(), equalTo(remoteNumShards));
             if (minimizeRoundtrips) {
-                assertThat(remoteClusterSearchInfo.getSkippedShards(), equalTo(remoteNumShards - 1));
+                assertThat(remoteClusterSearchInfo.getSkippedShards(), equalTo(remoteNumShards));
             } else {
                 assertThat(remoteClusterSearchInfo.getSkippedShards(), equalTo(remoteNumShards));
             }
diff --git a/x-pack/plugin/mapper-constant-keyword/src/test/java/org/elasticsearch/xpack/constantkeyword/mapper/SearchIdleTests.java b/x-pack/plugin/mapper-constant-keyword/src/test/java/org/elasticsearch/xpack/constantkeyword/mapper/SearchIdleTests.java
@@ -42,7 +42,6 @@
 
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponse;
 import static org.hamcrest.Matchers.empty;
-import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 
 public class SearchIdleTests extends ESSingleNodeTestCase {
@@ -133,8 +132,7 @@ public void testSearchIdleConstantKeywordMatchNoIndex() throws InterruptedExcept
         // WHEN
         assertResponse(search("test*", "constant_keyword", randomAlphaOfLength(5), 5), searchResponse -> {
             assertEquals(RestStatus.OK, searchResponse.status());
-            // NOTE: we need an empty result from at least one shard
-            assertEquals(idleIndexShardsCount + activeIndexShardsCount - 1, searchResponse.getSkippedShards());
+            assertEquals(idleIndexShardsCount + activeIndexShardsCount, searchResponse.getSkippedShards());
             assertEquals(0, searchResponse.getFailedShards());
             assertEquals(0, searchResponse.getHits().getHits().length);
         });
@@ -144,12 +142,8 @@ public void testSearchIdleConstantKeywordMatchNoIndex() throws InterruptedExcept
 
         assertIdleShardsRefreshStats(beforeStatsResponse, afterStatsResponse);
 
-        // If no shards match the can match phase then at least one shard gets queries for an empty response.
-        // However, this affects the search idle stats.
         List<ShardStats> active = Arrays.stream(afterStatsResponse.getShards()).filter(s -> s.isSearchIdle() == false).toList();
-        assertThat(active, hasSize(1));
-        assertThat(active.get(0).getShardRouting().getIndexName(), equalTo("test1"));
-        assertThat(active.get(0).getShardRouting().id(), equalTo(0));
+        assertThat(active, hasSize(0));
     }
 
     public void testSearchIdleConstantKeywordMatchOneIndex() throws InterruptedException {
diff --git a/x-pack/plugin/rank-rrf/src/internalClusterTest/java/org/elasticsearch/xpack/rank/rrf/RRFRankCoordinatorCanMatchIT.java b/x-pack/plugin/rank-rrf/src/internalClusterTest/java/org/elasticsearch/xpack/rank/rrf/RRFRankCoordinatorCanMatchIT.java
@@ -10,6 +10,7 @@
 import org.apache.lucene.document.LongPoint;
 import org.apache.lucene.index.DirectoryReader;
 import org.apache.lucene.index.PointValues;
+import org.apache.lucene.search.TotalHits;
 import org.elasticsearch.action.search.SearchType;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.index.IndexSettings;
@@ -206,10 +207,10 @@ public void testCanMatchCoordinator() throws Exception {
                 )
                 .setSize(5),
             response -> {
-                assertNull(response.getHits().getTotalHits());
+                assertEquals(new TotalHits(0, TotalHits.Relation.EQUAL_TO), response.getHits().getTotalHits());
                 assertEquals(0, response.getHits().getHits().length);
                 assertEquals(5, response.getSuccessfulShards());
-                assertEquals(4, response.getSkippedShards());
+                assertEquals(5, response.getSkippedShards());
             }
         );
 
diff --git a/x-pack/plugin/rank-rrf/src/internalClusterTest/java/org/elasticsearch/xpack/rank/rrf/RRFRankShardCanMatchIT.java b/x-pack/plugin/rank-rrf/src/internalClusterTest/java/org/elasticsearch/xpack/rank/rrf/RRFRankShardCanMatchIT.java
@@ -7,6 +7,7 @@
 
 package org.elasticsearch.xpack.rank.rrf;
 
+import org.apache.lucene.search.TotalHits;
 import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.action.DocWriteResponse;
 import org.elasticsearch.action.search.SearchType;
@@ -199,10 +200,10 @@ public void testCanMatchShard() throws IOException {
                 )
                 .setSize(5),
             response -> {
-                assertNull(response.getHits().getTotalHits());
+                assertEquals(new TotalHits(0, TotalHits.Relation.EQUAL_TO), response.getHits().getTotalHits());
                 assertEquals(0, response.getHits().getHits().length);
                 assertEquals(5, response.getSuccessfulShards());
-                assertEquals(4, response.getSkippedShards());
+                assertEquals(5, response.getSkippedShards());
             }
         );
 
diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsCanMatchOnCoordinatorIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsCanMatchOnCoordinatorIntegTests.java
diff --git a/x-pack/plugin/transform/src/internalClusterTest/java/org/elasticsearch/xpack/transform/checkpoint/TransformCCSCanMatchIT.java b/x-pack/plugin/transform/src/internalClusterTest/java/org/elasticsearch/xpack/transform/checkpoint/TransformCCSCanMatchIT.java
diff --git a/x-pack/qa/repository-old-versions/src/test/java/org/elasticsearch/oldrepos/OldRepositoryAccessIT.java b/x-pack/qa/repository-old-versions/src/test/java/org/elasticsearch/oldrepos/OldRepositoryAccessIT.java

Original file line number	Diff line number	Diff line change
`@@ -412,7 +412,7 @@ public void testSkippingShards() throws Exception {`
`412`	`412`	`assertResponse(client().search(searchRequest), searchResponse -> {`
`413`	`413`	`ElasticsearchAssertions.assertNoSearchHits(searchResponse);`
`414`	`414`	`assertThat(searchResponse.getTotalShards(), equalTo(2));`
`415`		`- assertThat(searchResponse.getSkippedShards(), equalTo(1));`
	`415`	`+ assertThat(searchResponse.getSkippedShards(), equalTo(2));`
`416`	`416`	`assertThat(searchResponse.getSuccessfulShards(), equalTo(2));`
`417`	`417`	`});`
`418`	`418`	`}`