From 2b1c26b08b98484db999217eb77e093e9ae380c8 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Tue, 1 Apr 2025 09:27:43 +0200 Subject: [PATCH] Re-enable parallel collection for field sorted top hits (#125916) With #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 #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. --- docs/changelog/125916.yaml | 5 +++++ .../metrics/TopHitsAggregationBuilder.java | 15 --------------- .../search/sort/FieldSortBuilder.java | 7 +++++++ .../search/sort/GeoDistanceSortBuilder.java | 7 +++++++ .../search/sort/ScoreSortBuilder.java | 5 ----- .../search/sort/ScriptSortBuilder.java | 5 ----- .../elasticsearch/search/sort/SortBuilder.java | 2 +- .../search/builder/SearchSourceBuilderTests.java | 2 +- 8 files changed, 21 insertions(+), 27 deletions(-) create mode 100644 docs/changelog/125916.yaml diff --git a/docs/changelog/125916.yaml b/docs/changelog/125916.yaml new file mode 100644 index 0000000000000..57741e4f0870a --- /dev/null +++ b/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: [] diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java index b8bd95dcdf9e9..ac37b287736aa 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TopHitsAggregationBuilder.java +++ b/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 { 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 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); - } } diff --git a/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java index cd597f3328c0f..f1ca48b7d3dda 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -749,4 +749,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; + } } diff --git a/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java index 9bd5d4b1a23fa..ab96638efe626 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortBuilder.java @@ -746,4 +746,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; + } } diff --git a/server/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java index a6fd4ef90693d..0c2fc68e7e856 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/ScoreSortBuilder.java @@ -173,9 +173,4 @@ public TransportVersion getMinimalSupportedVersion() { public ScoreSortBuilder rewrite(QueryRewriteContext ctx) { return this; } - - @Override - public boolean supportsParallelCollection() { - return true; - } } diff --git a/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java index a1920d3ead600..c5ebf97183601 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/ScriptSortBuilder.java @@ -511,9 +511,4 @@ public ScriptSortBuilder rewrite(QueryRewriteContext ctx) throws IOException { } return new ScriptSortBuilder(this).setNestedSort(rewrite); } - - @Override - public boolean supportsParallelCollection() { - return true; - } } diff --git a/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java index 7ed3abe7daa1a..17da9abd3952b 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java @@ -290,6 +290,6 @@ public String toString() { } public boolean supportsParallelCollection() { - return false; + return true; } } diff --git a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index a2c9119d0d5cf..9fef56ba16396 100644 --- a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -1030,7 +1030,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();