Re-enable parallel collection for field sorted top hits (elastic#125916) (elastic#126013)

With elastic#123610 we disabled parallel collection for field and script sorted top hits,
aligning its behaviour with that of top level search. This was mainly to work around
a bug in script sorting that did not support inter-segment concurrency.

The bug with script sort has been fixed with elastic#123757 and concurrency re-enabled for it.

While sort by field is not optimized for search concurrency, top hits benefits from it
and disabling concurrency for sort by field in top hits has caused performance
regressions in our nightly benchmarks.

This commit re-enables concurrency for top hits with sort by field is used. This
introduces back a discrepancy between top level search and top hits, in that concurrency
is applied for top hits despite sort by field normally disables it. The key difference
is the context where sorting is applied, and the fact that concurrency is disabled
only for performance reasons on top level searches and not for functional reasons.

Author: javanna

docs/changelog/125916.yaml

@@ -0,0 +1,5 @@
+pr: 125916
+summary: Re-enable parallel collection for field sorted top hits
+area: Search
+type: bug
+issues: []

server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java

@@ -49,7 +49,6 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
-import java.util.function.ToLongFunction;
 
 public class TopHitsAggregationBuilder extends AbstractAggregationBuilder<TopHitsAggregationBuilder> {
     public static final String NAME = "top_hits";
@@ -823,18 +822,4 @@ public String getType() {
     public TransportVersion getMinimalSupportedVersion() {
         return TransportVersions.ZERO;
     }
-
-    @Override
-    public boolean supportsParallelCollection(ToLongFunction<String> fieldCardinalityResolver) {
-        if (sorts != null) {
-            // the implicit sorting is by _score, which supports parallel collection
-            for (SortBuilder<?> sortBuilder : sorts) {
-                if (sortBuilder.supportsParallelCollection() == false) {
-                    return false;
-                }
-            }
-        }
-
-        return super.supportsParallelCollection(fieldCardinalityResolver);
-    }
 }

server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java

@@ -741,4 +741,11 @@ public FieldSortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
         }
         return new FieldSortBuilder(this).setNestedSort(rewrite);
     }
+
+    @Override
+    public boolean supportsParallelCollection() {
+        // Disable parallel collection for sort by field.
+        // It is supported but not optimized on the Lucene side to share info across collectors, and can cause regressions.
+        return false;
+    }
 }

server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java

@@ -721,4 +721,11 @@ public GeoDistanceSortBuilder rewrite(QueryRewriteContext ctx) throws IOExceptio
         }
         return new GeoDistanceSortBuilder(this).setNestedSort(rewrite);
     }
+
+    @Override
+    public boolean supportsParallelCollection() {
+        // Disable parallel collection for sort by field.
+        // It is supported but not optimized on the Lucene side to share info across collectors, and can cause regressions.
+        return false;
+    }
 }

server/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java

@@ -172,9 +172,4 @@ public TransportVersion getMinimalSupportedVersion() {
     public ScoreSortBuilder rewrite(QueryRewriteContext ctx) {
         return this;
     }
-
-    @Override
-    public boolean supportsParallelCollection() {
-        return true;
-    }
 }

server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java

@@ -502,9 +502,4 @@ public ScriptSortBuilder rewrite(QueryRewriteContext ctx) throws IOException {
         }
         return new ScriptSortBuilder(this).setNestedSort(rewrite);
     }
-
-    @Override
-    public boolean supportsParallelCollection() {
-        return true;
-    }
 }

server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java

@@ -285,6 +285,6 @@ public String toString() {
     }
 
     public boolean supportsParallelCollection() {
-        return false;
+        return true;
     }
 }

server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java

@@ -1007,7 +1007,7 @@ public void testSupportsParallelCollection() {
         {
             SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
             searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits").sort(SortBuilders.fieldSort("field")));
-            assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
+            assertTrue(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
         }
         {
             SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();