Skip to content

Commit 1a068ed

Browse files
committed
Disable concurrency when top_hits sorts on anything but _score (elastic#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.
1 parent 065f8a1 commit 1a068ed

File tree

3 files changed

+55
-0
lines changed

3 files changed

+55
-0
lines changed

docs/changelog/123610.yaml

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 123610
2+
summary: Disable concurrency when `top_hits` sorts on anything but `_score`
3+
area: "Aggregations"
4+
type: bug
5+
issues: []

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

+15
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import java.util.Objects;
5050
import java.util.Optional;
5151
import java.util.Set;
52+
import java.util.function.ToLongFunction;
5253

5354
public class TopHitsAggregationBuilder extends AbstractAggregationBuilder<TopHitsAggregationBuilder> {
5455
public static final String NAME = "top_hits";
@@ -822,4 +823,18 @@ public String getType() {
822823
public TransportVersion getMinimalSupportedVersion() {
823824
return TransportVersions.ZERO;
824825
}
826+
827+
@Override
828+
public boolean supportsParallelCollection(ToLongFunction<String> fieldCardinalityResolver) {
829+
if (sorts != null) {
830+
// the implicit sorting is by _score, which supports parallel collection
831+
for (SortBuilder<?> sortBuilder : sorts) {
832+
if (sortBuilder.supportsParallelCollection() == false) {
833+
return false;
834+
}
835+
}
836+
}
837+
838+
return super.supportsParallelCollection(fieldCardinalityResolver);
839+
}
825840
}

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

+35
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import org.elasticsearch.search.SearchExtBuilder;
3838
import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder;
3939
import org.elasticsearch.search.aggregations.metrics.MaxAggregationBuilder;
40+
import org.elasticsearch.search.aggregations.metrics.TopHitsAggregationBuilder;
4041
import org.elasticsearch.search.collapse.CollapseBuilder;
4142
import org.elasticsearch.search.collapse.CollapseBuilderTests;
4243
import org.elasticsearch.search.fetch.subphase.highlight.HighlightBuilder;
@@ -1016,6 +1017,40 @@ public void testSupportsParallelCollection() {
10161017
searchSourceBuilder.aggregation(new TermsAggregationBuilder("terms"));
10171018
assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
10181019
}
1020+
{
1021+
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
1022+
searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits"));
1023+
assertTrue(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
1024+
}
1025+
{
1026+
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
1027+
searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits").sort("_score"));
1028+
assertTrue(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
1029+
}
1030+
{
1031+
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
1032+
searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits").sort(SortBuilders.fieldSort("field")));
1033+
assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
1034+
}
1035+
{
1036+
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
1037+
searchSourceBuilder.aggregation(new TermsAggregationBuilder("terms").subAggregation(new TopHitsAggregationBuilder("tophits")));
1038+
assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
1039+
}
1040+
{
1041+
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
1042+
searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits").subAggregation(new TermsAggregationBuilder("terms")));
1043+
assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
1044+
}
1045+
{
1046+
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
1047+
searchSourceBuilder.aggregation(
1048+
new TopHitsAggregationBuilder("terms").sort(
1049+
SortBuilders.scriptSort(new Script("id"), ScriptSortBuilder.ScriptSortType.NUMBER)
1050+
)
1051+
);
1052+
assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
1053+
}
10191054
{
10201055
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
10211056
searchSourceBuilder.collapse(CollapseBuilderTests.randomCollapseBuilder());

0 commit comments

Comments
 (0)