From 39d4c7c26f7f6acbe9423de4889e23a148eaee69 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Mon, 6 May 2019 10:58:31 +0200 Subject: [PATCH] Skip explain fetch sub phase when request holds only suggestions (#41739) In case a search request holds only the suggest section, the query phase is skipped and only the suggest phase is executed instead. There will never be hits returned, and in case the explain flag is set to true, the explain sub phase throws a null pointer exception as the query is null. Usually a null query is replaced with a match all query as part of SearchContext#preProcess which is though skipped as well with suggest only searches. To address this, we skip the explain sub fetch phase for search requests that only requested suggestions. Closes #31260 --- .../fetch/subphase/ExplainFetchSubPhase.java | 2 +- .../search/query/QueryPhase.java | 1 - .../suggest/CompletionSuggestSearchIT.java | 120 +++++++++--------- 3 files changed, 62 insertions(+), 61 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/fetch/subphase/ExplainFetchSubPhase.java b/server/src/main/java/org/elasticsearch/search/fetch/subphase/ExplainFetchSubPhase.java index 57d2ca9048de0..c177cc8c3aed0 100644 --- a/server/src/main/java/org/elasticsearch/search/fetch/subphase/ExplainFetchSubPhase.java +++ b/server/src/main/java/org/elasticsearch/search/fetch/subphase/ExplainFetchSubPhase.java @@ -33,7 +33,7 @@ public final class ExplainFetchSubPhase implements FetchSubPhase { @Override public void hitExecute(SearchContext context, HitContext hitContext) { - if (context.explain() == false) { + if (context.explain() == false || context.hasOnlySuggest()) { return; } try { diff --git a/server/src/main/java/org/elasticsearch/search/query/QueryPhase.java b/server/src/main/java/org/elasticsearch/search/query/QueryPhase.java index 82d3528695c74..64621277f6e6f 100644 --- a/server/src/main/java/org/elasticsearch/search/query/QueryPhase.java +++ b/server/src/main/java/org/elasticsearch/search/query/QueryPhase.java @@ -95,7 +95,6 @@ public void preProcess(SearchContext context) { public void execute(SearchContext searchContext) throws QueryPhaseExecutionException { if (searchContext.hasOnlySuggest()) { suggestPhase.execute(searchContext); - // TODO: fix this once we can fetch docs for suggestions searchContext.queryResult().topDocs(new TopDocsAndMaxScore( new TopDocs(new TotalHits(0, TotalHits.Relation.EQUAL_TO), Lucene.EMPTY_SCORE_DOCS), Float.NaN), new DocValueFormat[0]); diff --git a/server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java b/server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java index f8a333cda6941..c59b342b7c0d8 100644 --- a/server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java +++ b/server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java @@ -379,17 +379,13 @@ public void testThatWeightsAreWorking() throws Exception { public void testThatWeightMustBeAnInteger() throws Exception { createIndexAndMapping(completionMappingBuilder); - try { - client().prepareIndex(INDEX, TYPE, "1").setSource(jsonBuilder() - .startObject().startObject(FIELD) - .startArray("input").value("sth").endArray() - .field("weight", 2.5) - .endObject().endObject() - ).get(); - fail("Indexing with a float weight was successful, but should not be"); - } catch (MapperParsingException e) { - assertThat(e.toString(), containsString("2.5")); - } + MapperParsingException e = expectThrows(MapperParsingException.class, + () -> client().prepareIndex(INDEX, TYPE, "1").setSource(jsonBuilder() + .startObject().startObject(FIELD) + .startArray("input").value("sth").endArray() + .field("weight", 2.5) + .endObject().endObject()).get()); + assertThat(e.toString(), containsString("2.5")); } public void testThatWeightCanBeAString() throws Exception { @@ -422,34 +418,28 @@ public void testThatWeightCanBeAString() throws Exception { public void testThatWeightMustNotBeANonNumberString() throws Exception { createIndexAndMapping(completionMappingBuilder); - try { - client().prepareIndex(INDEX, TYPE, "1").setSource(jsonBuilder() - .startObject().startObject(FIELD) - .startArray("input").value("sth").endArray() - .field("weight", "thisIsNotValid") - .endObject().endObject() - ).get(); - fail("Indexing with a non-number representing string as weight was successful, but should not be"); - } catch (MapperParsingException e) { - assertThat(e.toString(), containsString("thisIsNotValid")); - } + MapperParsingException e = expectThrows(MapperParsingException.class, + () -> client().prepareIndex(INDEX, TYPE, "1").setSource(jsonBuilder() + .startObject().startObject(FIELD) + .startArray("input").value("sth").endArray() + .field("weight", "thisIsNotValid") + .endObject().endObject() + ).get()); + assertThat(e.toString(), containsString("thisIsNotValid")); } public void testThatWeightAsStringMustBeInt() throws Exception { createIndexAndMapping(completionMappingBuilder); String weight = String.valueOf(Long.MAX_VALUE - 4); - try { - client().prepareIndex(INDEX, TYPE, "1").setSource(jsonBuilder() - .startObject().startObject(FIELD) - .startArray("input").value("testing").endArray() - .field("weight", weight) - .endObject().endObject() - ).get(); - fail("Indexing with weight string representing value > Int.MAX_VALUE was successful, but should not be"); - } catch (MapperParsingException e) { - assertThat(e.toString(), containsString(weight)); - } + + MapperParsingException e = expectThrows(MapperParsingException.class, + () -> client().prepareIndex(INDEX, TYPE, "1").setSource(jsonBuilder() + .startObject().startObject(FIELD) + .startArray("input").value("testing").endArray() + .field("weight", weight) + .endObject().endObject()).get()); + assertThat(e.toString(), containsString(weight)); } public void testThatInputCanBeAStringInsteadOfAnArray() throws Exception { @@ -821,13 +811,11 @@ public void testThatSortingOnCompletionFieldReturnsUsefulException() throws Exce ).get(); refresh(); - try { - client().prepareSearch(INDEX).setTypes(TYPE).addSort(new FieldSortBuilder(FIELD)).get(); - fail("Expected an exception due to trying to sort on completion field, but did not happen"); - } catch (SearchPhaseExecutionException e) { - assertThat(e.status().getStatus(), is(400)); - assertThat(e.toString(), containsString("Fielddata is not supported on field [" + FIELD + "] of type [completion]")); - } + + SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, + () -> client().prepareSearch(INDEX).setTypes(TYPE).addSort(new FieldSortBuilder(FIELD)).get()); + assertThat(e.status().getStatus(), is(400)); + assertThat(e.toString(), containsString("Fielddata is not supported on field [" + FIELD + "] of type [completion]")); } public void testThatSuggestStopFilterWorks() throws Exception { @@ -1118,17 +1106,12 @@ public void testReservedChars() throws IOException { .endObject()).get()); // can cause stack overflow without the default max_input_length String string = "foo" + (char) 0x00 + "bar"; - try { - client().prepareIndex(INDEX, TYPE, "1").setSource(jsonBuilder() - .startObject().startObject(FIELD) - .startArray("input").value(string).endArray() - .field("output", "foobar") - .endObject().endObject() - ).get(); - fail("Expected MapperParsingException"); - } catch (MapperParsingException e) { - assertThat(e.getMessage(), containsString("failed to parse")); - } + MapperParsingException e = expectThrows(MapperParsingException.class, + () -> client().prepareIndex(INDEX, TYPE, "1").setSource(jsonBuilder().startObject().startObject(FIELD) + .startArray("input").value(string).endArray() + .field("output", "foobar") + .endObject().endObject()).get()); + assertThat(e.getMessage(), containsString("failed to parse")); } // see #5930 @@ -1147,14 +1130,10 @@ public void testIssue5930() throws IOException { .endObject() ).setRefreshPolicy(IMMEDIATE).get(); - try { - client().prepareSearch(INDEX).addAggregation(AggregationBuilders.terms("suggest_agg").field(FIELD) - .collectMode(randomFrom(SubAggCollectionMode.values()))).get(); - // Exception must be thrown - assertFalse(true); - } catch (SearchPhaseExecutionException e) { - assertThat(e.toString(), containsString("Fielddata is not supported on field [" + FIELD + "] of type [completion]")); - } + SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, + () -> client().prepareSearch(INDEX).addAggregation(AggregationBuilders.terms("suggest_agg").field(FIELD) + .collectMode(randomFrom(SubAggCollectionMode.values()))).get()); + assertThat(e.toString(), containsString("Fielddata is not supported on field [" + FIELD + "] of type [completion]")); } public void testMultiDocSuggestions() throws Exception { @@ -1205,6 +1184,29 @@ public void testSuggestWithFieldAlias() throws Exception { assertSuggestions("suggestion", suggestionBuilder, "apple"); } + public void testSuggestOnlyExplain() throws Exception { + final CompletionMappingBuilder mapping = new CompletionMappingBuilder(); + createIndexAndMapping(mapping); + int numDocs = 10; + List indexRequestBuilders = new ArrayList<>(); + for (int i = 1; i <= numDocs; i++) { + indexRequestBuilders.add(client().prepareIndex(INDEX, TYPE, "" + i) + .setSource(jsonBuilder() + .startObject() + .startObject(FIELD) + .field("input", "suggestion" + i) + .field("weight", i) + .endObject() + .endObject() + )); + } + indexRandom(true, indexRequestBuilders); + CompletionSuggestionBuilder prefix = SuggestBuilders.completionSuggestion(FIELD).prefix("sugg"); + SearchResponse searchResponse = client().prepareSearch(INDEX).setExplain(true) + .suggest(new SuggestBuilder().addSuggestion("foo", prefix)).get(); + assertSuggestions(searchResponse, "foo", "suggestion10", "suggestion9", "suggestion8", "suggestion7", "suggestion6"); + } + public static boolean isReservedChar(char c) { switch (c) { case '\u001F':