Skip to content

Commit 9052226

Browse files
authored
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.
1 parent 79c388a commit 9052226

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
@@ -36,6 +36,7 @@
3636
import org.elasticsearch.search.SearchExtBuilder;
3737
import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder;
3838
import org.elasticsearch.search.aggregations.metrics.MaxAggregationBuilder;
39+
import org.elasticsearch.search.aggregations.metrics.TopHitsAggregationBuilder;
3940
import org.elasticsearch.search.collapse.CollapseBuilder;
4041
import org.elasticsearch.search.collapse.CollapseBuilderTests;
4142
import org.elasticsearch.search.fetch.subphase.highlight.HighlightBuilder;
@@ -993,6 +994,40 @@ public void testSupportsParallelCollection() {
993994
searchSourceBuilder.aggregation(new TermsAggregationBuilder("terms"));
994995
assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
995996
}
997+
{
998+
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
999+
searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits"));
1000+
assertTrue(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
1001+
}
1002+
{
1003+
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
1004+
searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits").sort("_score"));
1005+
assertTrue(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
1006+
}
1007+
{
1008+
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
1009+
searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits").sort(SortBuilders.fieldSort("field")));
1010+
assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
1011+
}
1012+
{
1013+
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
1014+
searchSourceBuilder.aggregation(new TermsAggregationBuilder("terms").subAggregation(new TopHitsAggregationBuilder("tophits")));
1015+
assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
1016+
}
1017+
{
1018+
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
1019+
searchSourceBuilder.aggregation(new TopHitsAggregationBuilder("tophits").subAggregation(new TermsAggregationBuilder("terms")));
1020+
assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
1021+
}
1022+
{
1023+
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
1024+
searchSourceBuilder.aggregation(
1025+
new TopHitsAggregationBuilder("terms").sort(
1026+
SortBuilders.scriptSort(new Script("id"), ScriptSortBuilder.ScriptSortType.NUMBER)
1027+
)
1028+
);
1029+
assertFalse(searchSourceBuilder.supportsParallelCollection(fieldCardinality));
1030+
}
9961031
{
9971032
SearchSourceBuilder searchSourceBuilder = newSearchSourceBuilder.get();
9981033
searchSourceBuilder.collapse(CollapseBuilderTests.randomCollapseBuilder());

0 commit comments

Comments
 (0)