From 74d8c75d3a10e03338468069f30466897b1380fc Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Wed, 19 Oct 2016 17:51:10 +0100 Subject: [PATCH] Fixes bug preventing script sort working on top_hits aggregation (#21023) Previous to this change any request using a script sort in a top_hits aggregation would fail because the compilation of the script happened after the QueryShardContext was frozen (after we had worked out if the request is cachable). This change moves the calling of build() on the SortBuilder to the TopHitsAggregationBuilder which means that the script in the script_sort will be compiled before we decide whether to cache the request and freeze the context. Closes #21022 --- .../tophits/TopHitsAggregationBuilder.java | 14 +++++++++++--- .../metrics/tophits/TopHitsAggregatorFactory.java | 15 +++++---------- .../search/aggregations/metrics/TopHitsIT.java | 15 ++++++++++++++- 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregationBuilder.java index 70f1adc771f..4f83bbd08d5 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregationBuilder.java @@ -44,6 +44,7 @@ import org.elasticsearch.search.fetch.subphase.FetchSourceContext; import org.elasticsearch.search.fetch.subphase.ScriptFieldsContext; import org.elasticsearch.search.fetch.subphase.highlight.HighlightBuilder; import org.elasticsearch.search.sort.ScoreSortBuilder; +import org.elasticsearch.search.sort.SortAndFormats; import org.elasticsearch.search.sort.SortBuilder; import org.elasticsearch.search.sort.SortBuilders; import org.elasticsearch.search.sort.SortOrder; @@ -54,6 +55,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Objects; +import java.util.Optional; import java.util.Set; public class TopHitsAggregationBuilder extends AbstractAggregationBuilder { @@ -539,9 +541,15 @@ public class TopHitsAggregationBuilder extends AbstractAggregationBuilder optionalSort; + if (sorts == null) { + optionalSort = Optional.empty(); + } else { + optionalSort = SortBuilder.buildSort(sorts, context.searchContext().getQueryShardContext()); + } + return new TopHitsAggregatorFactory(name, type, from, size, explain, version, trackScores, optionalSort, highlightBuilder, + storedFieldsContext, fieldDataFields, fields, fetchSourceContext, context, parent, subfactoriesBuilder, metaData); } @Override diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorFactory.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorFactory.java index 783c3baa7a4..9c9e94bc9fa 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorFactory.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsAggregatorFactory.java @@ -32,8 +32,6 @@ import org.elasticsearch.search.fetch.subphase.ScriptFieldsContext; import org.elasticsearch.search.fetch.subphase.highlight.HighlightBuilder; import org.elasticsearch.search.internal.SubSearchContext; import org.elasticsearch.search.sort.SortAndFormats; -import org.elasticsearch.search.sort.SortBuilder; - import java.io.IOException; import java.util.List; import java.util.Map; @@ -46,7 +44,7 @@ public class TopHitsAggregatorFactory extends AggregatorFactory> sorts; + private final Optional sort; private final HighlightBuilder highlightBuilder; private final StoredFieldsContext storedFieldsContext; private final List docValueFields; @@ -54,7 +52,7 @@ public class TopHitsAggregatorFactory extends AggregatorFactory> sorts, HighlightBuilder highlightBuilder, StoredFieldsContext storedFieldsContext, + Optional sort, HighlightBuilder highlightBuilder, StoredFieldsContext storedFieldsContext, List docValueFields, List scriptFields, FetchSourceContext fetchSourceContext, AggregationContext context, AggregatorFactory parent, AggregatorFactories.Builder subFactories, Map metaData) throws IOException { @@ -64,7 +62,7 @@ public class TopHitsAggregatorFactory extends AggregatorFactory optionalSort = SortBuilder.buildSort(sorts, subSearchContext.getQueryShardContext()); - if (optionalSort.isPresent()) { - subSearchContext.sort(optionalSort.get()); - } + if (sort.isPresent()) { + subSearchContext.sort(sort.get()); } if (storedFieldsContext != null) { subSearchContext.storedFieldsContext(storedFieldsContext); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/TopHitsIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/TopHitsIT.java index 76b4d831020..4e43ae0dbc7 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/TopHitsIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/TopHitsIT.java @@ -48,6 +48,7 @@ import org.elasticsearch.search.aggregations.metrics.max.Max; import org.elasticsearch.search.aggregations.metrics.tophits.TopHits; import org.elasticsearch.search.fetch.subphase.highlight.HighlightBuilder; import org.elasticsearch.search.fetch.subphase.highlight.HighlightField; +import org.elasticsearch.search.sort.ScriptSortBuilder.ScriptSortType; import org.elasticsearch.search.sort.SortBuilders; import org.elasticsearch.search.sort.SortOrder; import org.elasticsearch.test.ESIntegTestCase; @@ -1010,11 +1011,23 @@ public class TopHitsIT extends ESIntegTestCase { assertThat(client().admin().indices().prepareStats("cache_test_idx").setRequestCache(true).get().getTotal().getRequestCache() .getMissCount(), equalTo(0L)); - // Test that a request using a script does not get cached + // Test that a request using a script field does not get cached SearchResponse r = client().prepareSearch("cache_test_idx").setSize(0) .addAggregation(topHits("foo").scriptField("bar", new Script("5", ScriptType.INLINE, CustomScriptPlugin.NAME, null))).get(); assertSearchResponse(r); + assertThat(client().admin().indices().prepareStats("cache_test_idx").setRequestCache(true).get().getTotal().getRequestCache() + .getHitCount(), equalTo(0L)); + assertThat(client().admin().indices().prepareStats("cache_test_idx").setRequestCache(true).get().getTotal().getRequestCache() + .getMissCount(), equalTo(0L)); + + // Test that a request using a script sort does not get cached + r = client().prepareSearch("cache_test_idx").setSize(0) + .addAggregation(topHits("foo").sort( + SortBuilders.scriptSort(new Script("5", ScriptType.INLINE, CustomScriptPlugin.NAME, null), ScriptSortType.STRING))) + .get(); + assertSearchResponse(r); + assertThat(client().admin().indices().prepareStats("cache_test_idx").setRequestCache(true).get().getTotal().getRequestCache() .getHitCount(), equalTo(0L)); assertThat(client().admin().indices().prepareStats("cache_test_idx").setRequestCache(true).get().getTotal().getRequestCache()