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()

docs/changelog/115314.yaml

@@ -0,0 +1,5 @@
+pr: 115314
+summary: Only aggregations require at least one shard request
+area: Search
+type: enhancement
+issues: []

modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBIndexingIT.java

@@ -412,7 +412,7 @@ public class TSDBIndexingIT extends ESSingleNodeTestCase {
             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));
             });
         }

qa/multi-cluster-search/src/test/java/org/elasticsearch/search/CCSDuelIT.java

@@ -43,6 +43,7 @@ import org.elasticsearch.join.query.HasParentQueryBuilder;
 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 class CCSDuelIT extends ESRestTestCase {
 
     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");

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 : 2}
-  - match: { _shards.skipped : 1}
   - 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 : 2}
-  - match: { _shards.skipped : 1}
   - 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 : 2}
-  - match: { _shards.skipped : 1}
   - 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 : 2}
-  - match: { _shards.skipped : 1}
   - match: { _shards.failed: 0 }
 

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 }

server/src/internalClusterTest/java/org/elasticsearch/search/ccs/CrossClusterSearchIT.java

@@ -214,7 +214,7 @@ public class CrossClusterSearchIT extends AbstractMultiClustersTestCase {
                 // 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 class CrossClusterSearchIT extends AbstractMultiClustersTestCase {
             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));
             }

server/src/internalClusterTest/java/org/elasticsearch/search/profile/query/QueryProfilerIT.java

@@ -68,7 +68,11 @@ public class QueryProfilerIT extends ESIntegTestCase {
                 prepareSearch().setQuery(q).setTrackTotalHits(true).setProfile(true).setSearchType(SearchType.QUERY_THEN_FETCH),
                 response -> {
                     assertNotNull("Profile response element should not be null", response.getProfileResults());
+                    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()) {

server/src/internalClusterTest/java/org/elasticsearch/search/stats/FieldUsageStatsIT.java

@@ -158,11 +158,15 @@ public class FieldUsageStatsIT extends ESIntegTestCase {
 
         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
+        long expectedShards = 2L * numShards;
-        // to produce a valid search result with all the aggs etc., so we hit one of the two shards
+        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)

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

@@ -1458,6 +1458,8 @@ public class TransportSearchAction extends HandledTransportAction<SearchRequest,
             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,
@@ -1469,7 +1471,7 @@ public class TransportSearchAction extends HandledTransportAction<SearchRequest,
                     shardIterators,
                     timeProvider,
                     task,
-                    true,
+                    requireAtLeastOneMatch,
                     searchService.getCoordinatorRewriteContextProvider(timeProvider::absoluteStartMillis),
                     listener.delegateFailureAndWrap(
                         (l, iters) -> newSearchPhase(

x-pack/plugin/async-search/src/internalClusterTest/java/org/elasticsearch/xpack/search/CrossClusterAsyncSearchIT.java

@@ -274,6 +274,8 @@ public class CrossClusterAsyncSearchIT extends AbstractMultiClustersTestCase {
         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));
@@ -288,20 +290,30 @@ public class CrossClusterAsyncSearchIT extends AbstractMultiClustersTestCase {
             assertTrue(response.isRunning());
             SearchResponse.Clusters clusters = response.getSearchResponse().getClusters();
             assertThat(clusters.getTotal(), equalTo(2));
+            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);
+            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();
         }
-
+        if (dfs) {
             SearchListenerPlugin.waitSearchStarted();
+        }
         SearchListenerPlugin.allowQueryPhase();
 
         waitForSearchTasksToFinish();
@@ -331,7 +343,7 @@ public class CrossClusterAsyncSearchIT extends AbstractMultiClustersTestCase {
                 // 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 +353,7 @@ public class CrossClusterAsyncSearchIT extends AbstractMultiClustersTestCase {
             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 +389,7 @@ public class CrossClusterAsyncSearchIT extends AbstractMultiClustersTestCase {
                 // 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 +399,7 @@ public class CrossClusterAsyncSearchIT extends AbstractMultiClustersTestCase {
             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));
             }

x-pack/plugin/mapper-constant-keyword/src/test/java/org/elasticsearch/xpack/constantkeyword/mapper/SearchIdleTests.java

@@ -42,7 +42,6 @@ import java.util.concurrent.TimeUnit;
 
 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 class SearchIdleTests extends ESSingleNodeTestCase {
         // 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, searchResponse.getSkippedShards());
-            assertEquals(idleIndexShardsCount + activeIndexShardsCount - 1, searchResponse.getSkippedShards());
             assertEquals(0, searchResponse.getFailedShards());
             assertEquals(0, searchResponse.getHits().getHits().length);
         });
@@ -144,12 +142,8 @@ public class SearchIdleTests extends ESSingleNodeTestCase {
 
         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, hasSize(0));
-        assertThat(active.get(0).getShardRouting().getIndexName(), equalTo("test1"));
-        assertThat(active.get(0).getShardRouting().id(), equalTo(0));
     }
 
     public void testSearchIdleConstantKeywordMatchOneIndex() throws InterruptedException {

x-pack/plugin/rank-rrf/src/internalClusterTest/java/org/elasticsearch/xpack/rank/rrf/RRFRankCoordinatorCanMatchIT.java

@@ -10,6 +10,7 @@ package org.elasticsearch.xpack.rank.rrf;
 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 class RRFRankCoordinatorCanMatchIT extends ESIntegTestCase {
                 )
                 .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());
             }
         );
 

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 class RRFRankShardCanMatchIT extends ESIntegTestCase {
                 )
                 .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());
             }
         );
 

x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsCanMatchOnCoordinatorIntegTests.java

@@ -384,11 +384,9 @@ public class SearchableSnapshotsCanMatchOnCoordinatorIntegTests extends BaseFroz
                 }
             } else {
                 assertResponse(client().search(request), newSearchResponse -> {
-                    // When all shards are skipped, at least one of them should be queried in order to
+                    assertThat(newSearchResponse.getSkippedShards(), equalTo(indexOutsideSearchRangeShardCount));
-                    // provide a proper search response.
+                    assertThat(newSearchResponse.getSuccessfulShards(), equalTo(indexOutsideSearchRangeShardCount));
-                    assertThat(newSearchResponse.getSkippedShards(), equalTo(indexOutsideSearchRangeShardCount - 1));
+                    assertThat(newSearchResponse.getFailedShards(), equalTo(0));
-                    assertThat(newSearchResponse.getSuccessfulShards(), equalTo(indexOutsideSearchRangeShardCount - 1));
-                    assertThat(newSearchResponse.getFailedShards(), equalTo(1));
                     assertThat(newSearchResponse.getTotalShards(), equalTo(indexOutsideSearchRangeShardCount));
                 });
 
@@ -748,9 +746,7 @@ public class SearchableSnapshotsCanMatchOnCoordinatorIntegTests extends BaseFroz
                 // All the regular index searches succeeded
                 assertThat(newSearchResponse.getSuccessfulShards(), equalTo(totalShards));
                 assertThat(newSearchResponse.getFailedShards(), equalTo(0));
-                // We have to query at least one node to construct a valid response, and we pick
+                assertThat(newSearchResponse.getSkippedShards(), equalTo(totalShards));
-                // a shard that's available in order to construct the search response
-                assertThat(newSearchResponse.getSkippedShards(), equalTo(totalShards - 1));
                 assertThat(newSearchResponse.getTotalShards(), equalTo(totalShards));
                 assertThat(newSearchResponse.getHits().getTotalHits().value(), equalTo(0L));
             });

x-pack/plugin/transform/src/internalClusterTest/java/org/elasticsearch/xpack/transform/checkpoint/TransformCCSCanMatchIT.java

@@ -197,15 +197,13 @@ public class TransformCCSCanMatchIT extends AbstractMultiClustersTestCase {
             QueryBuilders.rangeQuery("@timestamp").from(100_000_000),  // This query matches no documents
             true,
             0,
-            // All but 2 shards are skipped. TBH I don't know why this 2 shards are not skipped
+            oldLocalNumShards + newLocalNumShards + oldRemoteNumShards + newRemoteNumShards
-            oldLocalNumShards + newLocalNumShards + oldRemoteNumShards + newRemoteNumShards - 2
         );
         testSearchAction(
             QueryBuilders.rangeQuery("@timestamp").from(100_000_000),  // This query matches no documents
             false,
             0,
-            // All but 1 shards are skipped. TBH I don't know why this 1 shard is not skipped
+            oldLocalNumShards + newLocalNumShards + oldRemoteNumShards + newRemoteNumShards
-            oldLocalNumShards + newLocalNumShards + oldRemoteNumShards + newRemoteNumShards - 1
         );
     }
 

x-pack/qa/repository-old-versions/src/test/java/org/elasticsearch/oldrepos/OldRepositoryAccessIT.java

@@ -484,8 +484,7 @@ public class OldRepositoryAccessIT extends ESRestTestCase {
                 logger.info(searchResponse);
                 assertEquals(0, searchResponse.getHits().getTotalHits().value());
                 assertEquals(numberOfShards, searchResponse.getSuccessfulShards());
-                // When all shards are skipped, at least one of them is queried in order to provide a proper search response.
+                assertEquals(numberOfShards, searchResponse.getSkippedShards());
-                assertEquals(numberOfShards - 1, searchResponse.getSkippedShards());
             } finally {
                 searchResponse.decRef();
             }