From 25f70bbfc4e68298748819f01a738cb14e8e41e8 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Thu, 8 Mar 2018 11:20:29 +0100 Subject: [PATCH] Revert "Rescore collapsed documents (#28521)" This reverts commit f057fc294af900078c1bc6b8bbb29c4ed0c8cc8d. The rescorer does not resort the collapsed values inside the top docs during rescoring. For this reason the Lucene rescorer is not compatible with collapsing. Relates #27243 --- .../test/search/110_field_collapsing.yml | 22 +++++++ .../search/collapse/CollapseBuilder.java | 3 + .../search/query/TopDocsCollectorContext.java | 19 ++---- .../search/functionscore/QueryRescorerIT.java | 66 ------------------- 4 files changed, 31 insertions(+), 79 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/110_field_collapsing.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/110_field_collapsing.yml index e37e5d7b0cb31..76bd3a1ba7286 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search/110_field_collapsing.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/110_field_collapsing.yml @@ -237,6 +237,28 @@ setup: search_after: [6] sort: [{ sort: desc }] +--- +"field collapsing and rescore": + + - skip: + version: " - 5.2.99" + reason: this uses a new API that has been added in 5.3 + + - do: + catch: /cannot use \`collapse\` in conjunction with \`rescore\`/ + search: + index: test + type: test + body: + collapse: { field: numeric_group } + rescore: + window_size: 20 + query: + rescore_query: + match_all: {} + query_weight: 1 + rescore_query_weight: 2 + --- "no hits and inner_hits": diff --git a/server/src/main/java/org/elasticsearch/search/collapse/CollapseBuilder.java b/server/src/main/java/org/elasticsearch/search/collapse/CollapseBuilder.java index b9983dbf359a6..90e35c34e28f8 100644 --- a/server/src/main/java/org/elasticsearch/search/collapse/CollapseBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/collapse/CollapseBuilder.java @@ -225,6 +225,9 @@ public CollapseContext build(SearchContext context) { if (context.searchAfter() != null) { throw new SearchContextException(context, "cannot use `collapse` in conjunction with `search_after`"); } + if (context.rescore() != null && context.rescore().isEmpty() == false) { + throw new SearchContextException(context, "cannot use `collapse` in conjunction with `rescore`"); + } MappedFieldType fieldType = context.getQueryShardContext().fieldMapper(field); if (fieldType == null) { diff --git a/server/src/main/java/org/elasticsearch/search/query/TopDocsCollectorContext.java b/server/src/main/java/org/elasticsearch/search/query/TopDocsCollectorContext.java index 2f7e1890266a4..cf4ff6c77b823 100644 --- a/server/src/main/java/org/elasticsearch/search/query/TopDocsCollectorContext.java +++ b/server/src/main/java/org/elasticsearch/search/query/TopDocsCollectorContext.java @@ -128,7 +128,6 @@ void postProcess(QuerySearchResult result) { static class CollapsingTopDocsCollectorContext extends TopDocsCollectorContext { private final DocValueFormat[] sortFmt; private final CollapsingTopDocsCollector topDocsCollector; - private final boolean rescore; /** * Ctr @@ -140,14 +139,13 @@ static class CollapsingTopDocsCollectorContext extends TopDocsCollectorContext { private CollapsingTopDocsCollectorContext(CollapseContext collapseContext, @Nullable SortAndFormats sortAndFormats, int numHits, - boolean trackMaxScore, boolean rescore) { + boolean trackMaxScore) { super(REASON_SEARCH_TOP_HITS, numHits); assert numHits > 0; assert collapseContext != null; Sort sort = sortAndFormats == null ? Sort.RELEVANCE : sortAndFormats.sort; this.sortFmt = sortAndFormats == null ? new DocValueFormat[] { DocValueFormat.RAW } : sortAndFormats.formats; this.topDocsCollector = collapseContext.createTopDocs(sort, numHits, trackMaxScore); - this.rescore = rescore; } @Override @@ -160,11 +158,6 @@ Collector create(Collector in) throws IOException { void postProcess(QuerySearchResult result) throws IOException { result.topDocs(topDocsCollector.getTopDocs(), sortFmt); } - - @Override - boolean shouldRescore() { - return rescore; - } } abstract static class SimpleTopDocsCollectorContext extends TopDocsCollectorContext { @@ -339,6 +332,11 @@ static TopDocsCollectorContext createTopDocsCollectorContext(SearchContext searc return new ScrollingTopDocsCollectorContext(reader, query, searchContext.scrollContext(), searchContext.sort(), numDocs, searchContext.trackScores(), searchContext.numberOfShards(), searchContext.trackTotalHits(), hasFilterCollector); + } else if (searchContext.collapse() != null) { + boolean trackScores = searchContext.sort() == null ? true : searchContext.trackScores(); + int numDocs = Math.min(searchContext.from() + searchContext.size(), totalNumDocs); + return new CollapsingTopDocsCollectorContext(searchContext.collapse(), + searchContext.sort(), numDocs, trackScores); } else { int numDocs = Math.min(searchContext.from() + searchContext.size(), totalNumDocs); final boolean rescore = searchContext.rescore().isEmpty() == false; @@ -348,11 +346,6 @@ static TopDocsCollectorContext createTopDocsCollectorContext(SearchContext searc numDocs = Math.max(numDocs, rescoreContext.getWindowSize()); } } - if (searchContext.collapse() != null) { - boolean trackScores = searchContext.sort() == null ? true : searchContext.trackScores(); - return new CollapsingTopDocsCollectorContext(searchContext.collapse(), - searchContext.sort(), numDocs, trackScores, rescore); - } return new SimpleTopDocsCollectorContext(reader, query, searchContext.sort(), searchContext.searchAfter(), numDocs, searchContext.trackScores(), searchContext.trackTotalHits(), hasFilterCollector) { @Override diff --git a/server/src/test/java/org/elasticsearch/search/functionscore/QueryRescorerIT.java b/server/src/test/java/org/elasticsearch/search/functionscore/QueryRescorerIT.java index 98f14c1aa6e74..58565b5f264b7 100644 --- a/server/src/test/java/org/elasticsearch/search/functionscore/QueryRescorerIT.java +++ b/server/src/test/java/org/elasticsearch/search/functionscore/QueryRescorerIT.java @@ -36,17 +36,13 @@ import org.elasticsearch.index.query.functionscore.ScoreFunctionBuilders; import org.elasticsearch.search.SearchHit; import org.elasticsearch.search.SearchHits; -import org.elasticsearch.search.collapse.CollapseBuilder; import org.elasticsearch.search.rescore.QueryRescoreMode; import org.elasticsearch.search.rescore.QueryRescorerBuilder; import org.elasticsearch.search.sort.SortBuilders; import org.elasticsearch.test.ESIntegTestCase; -import java.io.IOException; import java.util.Arrays; import java.util.Comparator; -import java.util.Map; -import java.util.stream.Collectors; import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_REPLICAS; import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF_SHARDS; @@ -71,7 +67,6 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThirdHit; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.hasId; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.hasScore; -import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; @@ -753,65 +748,4 @@ public void testRescorePhaseWithInvalidSort() throws Exception { assertThat(hit.getScore(), equalTo(101f)); } } - - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/28932") - public void testRescoreAfterCollapse() throws Exception { - assertAcked(prepareCreate("test") - .addMapping( - "type1", - jsonBuilder() - .startObject() - .startObject("properties") - .startObject("group") - .field("type", "keyword") - .endObject() - .endObject() - .endObject()) - ); - - ensureGreen("test"); - - indexDocument(1, "miss", "a", 1, 10); - indexDocument(2, "name", "a", 2, 20); - indexDocument(3, "name", "b", 2, 30); - // should be highest on rescore, but filtered out during collapse - indexDocument(4, "name", "b", 1, 40); - - refresh("test"); - - SearchResponse searchResponse = client().prepareSearch("test") - .setTypes("type1") - .setQuery(staticScoreQuery("static_score")) - .addRescorer(new QueryRescorerBuilder(staticScoreQuery("static_rescore"))) - .setCollapse(new CollapseBuilder("group")) - .get(); - - assertThat(searchResponse.getHits().totalHits, equalTo(3L)); - assertThat(searchResponse.getHits().getHits().length, equalTo(2)); - - Map collapsedHits = Arrays - .stream(searchResponse.getHits().getHits()) - .collect(Collectors.toMap(SearchHit::getId, SearchHit::getScore)); - - assertThat(collapsedHits.keySet(), containsInAnyOrder("2", "3")); - assertThat(collapsedHits.get("2"), equalTo(22F)); - assertThat(collapsedHits.get("3"), equalTo(32F)); - } - - private QueryBuilder staticScoreQuery(String scoreField) { - return functionScoreQuery(termQuery("name", "name"), ScoreFunctionBuilders.fieldValueFactorFunction(scoreField)) - .boostMode(CombineFunction.REPLACE); - } - - private void indexDocument(int id, String name, String group, int score, int rescore) throws IOException { - XContentBuilder docBuilder =jsonBuilder() - .startObject() - .field("name", name) - .field("group", group) - .field("static_score", score) - .field("static_rescore", rescore) - .endObject(); - - client().prepareIndex("test", "type1", Integer.toString(id)).setSource(docBuilder).get(); - } }