From 1a068edea49271fafd0888eefa0fae3aa2711338 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Thu, 27 Feb 2025 21:22:17 +0100 Subject: [PATCH] Disable concurrency when top_hits sorts on anything but _score (#123610) We already disable inter-segment concurrency in SearchSourceBuilder whenever the top-level sort provided is not _score. We shoudl apply the same rules in top_hits. We recenly stumbled upon non deterministic behaviour caused by script sorting defined within top hits. That is to be expected given that script sorting does not support search concurrency. The sort script can be replaced with a runtime field, either defined in the mapping or in the search request, which does support concurrency and guarantees predictable behaviour. --- docs/changelog/123610.yaml | 5 +++ .../metrics/TopHitsAggregationBuilder.java | 15 ++++++++ .../builder/SearchSourceBuilderTests.java | 35 +++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 docs/changelog/123610.yaml diff --git a/docs/changelog/123610.yaml b/docs/changelog/123610.yaml new file mode 100644 index 0000000000000..628d832f903dc --- /dev/null +++ b/docs/changelog/123610.yaml @@ -0,0 +1,5 @@ +pr: 123610 +summary: Disable concurrency when `top_hits` sorts on anything but `_score` +area: "Aggregations" +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 ac37b287736aa..b8bd95dcdf9e9 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,6 +49,7 @@ 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"; @@ -822,4 +823,18 @@ 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/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index cdbf4cdff15a7..873b923d8f755 100644 --- a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -37,6 +37,7 @@ import org.elasticsearch.search.SearchExtBuilder; import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder; import org.elasticsearch.search.aggregations.metrics.MaxAggregationBuilder; +import org.elasticsearch.search.aggregations.metrics.TopHitsAggregationBuilder; import org.elasticsearch.search.collapse.CollapseBuilder; import org.elasticsearch.search.collapse.CollapseBuilderTests; import org.elasticsearch.search.fetch.subphase.highlight.HighlightBuilder; @@ -1016,6 +1017,40 @@ public void testSupportsParallelCollection() { searchSourceBuilder.aggregation(new TermsAggregationBuilder("terms")); assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality)); } + { + SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get(); + searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits")); + assertTrue(searchSourceBuilder.supportsParallelCollection(fieldCardinality)); + } + { + SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get(); + searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits").sort("_score")); + assertTrue(searchSourceBuilder.supportsParallelCollection(fieldCardinality)); + } + { + SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get(); + searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits").sort(SortBuilders.fieldSort("field"))); + assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality)); + } + { + SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get(); + searchSourceBuilder.aggregation(new TermsAggregationBuilder("terms").subAggregation(new TopHitsAggregationBuilder("tophits"))); + assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality)); + } + { + SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get(); + searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits").subAggregation(new TermsAggregationBuilder("terms"))); + assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality)); + } + { + SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get(); + searchSourceBuilder.aggregation( + new TopHitsAggregationBuilder("terms").sort( + SortBuilders.scriptSort(new Script("id"), ScriptSortBuilder.ScriptSortType.NUMBER) + ) + ); + assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality)); + } { SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get(); searchSourceBuilder.collapse(CollapseBuilderTests.randomCollapseBuilder());