From 757de805d345cc6f86dfb9212304adca165295a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Thu, 28 Jul 2016 11:57:17 +0200 Subject: [PATCH 1/3] `multi_match` query should produce MatchNoDocs query on unknown fieldname Currently when the `fields` parameter used in a `multi_match` query contains a wildcard expression that doesn't resolve to any field name in the target index, MultiMatchQueryBuilder produces a `null` query. This change changes it to be a MatchNoDocs query, since returning no documents for this case is already the current behaviour. Also adding missing field names (with and without wildcards) to the unit and integration test. --- .../index/query/MultiMatchQueryBuilder.java | 7 ++++++- .../org/elasticsearch/index/search/MatchQuery.java | 1 - .../index/query/MultiMatchQueryBuilderTests.java | 13 ++++++++++++- .../search/query/MultiMatchQueryIT.java | 6 +++++- 4 files changed, 23 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java index e2d34db0b4a..41b6829994e 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java @@ -29,6 +29,7 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.lucene.search.MatchNoDocsQuery; import org.elasticsearch.common.regex.Regex; import org.elasticsearch.common.unit.Fuzziness; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -747,7 +748,11 @@ public class MultiMatchQueryBuilder extends AbstractQueryBuilder { + private static final String MISSING_WILDCARD_FIELD_NAME = "missing_*"; + private static final String MISSING_FIELD_NAME = "missing"; + @Override protected MultiMatchQueryBuilder doCreateTestQueryBuilder() { - String fieldName = randomFrom(STRING_FIELD_NAME, INT_FIELD_NAME, DOUBLE_FIELD_NAME, BOOLEAN_FIELD_NAME, DATE_FIELD_NAME); + String fieldName = randomFrom(STRING_FIELD_NAME, INT_FIELD_NAME, DOUBLE_FIELD_NAME, BOOLEAN_FIELD_NAME, DATE_FIELD_NAME, + MISSING_FIELD_NAME, MISSING_WILDCARD_FIELD_NAME); if (fieldName.equals(DATE_FIELD_NAME)) { assumeTrue("test with date fields runs only when at least a type is registered", getCurrentTypes().length > 0); } + // creates the query with random value and field name Object value; if (fieldName.equals(STRING_FIELD_NAME)) { @@ -238,6 +243,12 @@ public class MultiMatchQueryBuilderTests extends AbstractQueryTestCase 0); + assertThat(multiMatchQuery("test").field(MISSING_WILDCARD_FIELD_NAME).toQuery(createShardContext()), instanceOf(MatchNoDocsQuery.class)); + assertThat(multiMatchQuery("test").field(MISSING_FIELD_NAME).toQuery(createShardContext()), instanceOf(TermQuery.class)); + } + public void testFromJson() throws IOException { String json = "{\n" + diff --git a/core/src/test/java/org/elasticsearch/search/query/MultiMatchQueryIT.java b/core/src/test/java/org/elasticsearch/search/query/MultiMatchQueryIT.java index 766aff8d274..cb55f88ff80 100644 --- a/core/src/test/java/org/elasticsearch/search/query/MultiMatchQueryIT.java +++ b/core/src/test/java/org/elasticsearch/search/query/MultiMatchQueryIT.java @@ -237,7 +237,8 @@ public class MultiMatchQueryIT extends ESIntegTestCase { assertNoFailures(searchResponse); assertFirstHit(searchResponse, hasId("theone")); - String[] fields = {"full_name", "first_name", "last_name", "last_name_phrase", "first_name_phrase", "category_phrase", "category"}; + String[] fields = { "full_name", "first_name", "last_name", "last_name_phrase", "first_name_phrase", "category_phrase", "category", + "missing_field", "missing_fields*" }; String[] query = {"marvel","hero", "captain", "america", "15", "17", "1", "5", "ultimate", "Man", "marvel", "wolferine", "ninja"}; @@ -269,6 +270,9 @@ public class MultiMatchQueryIT extends ESIntegTestCase { .setQuery(matchQueryBuilder).get(); assertThat("field: " + field + " query: " + builder.toString(), multiMatchResp.getHits().getTotalHits(), equalTo(matchResp.getHits().getTotalHits())); SearchHits hits = multiMatchResp.getHits(); + if (field.startsWith("missing")) { + assertEquals(0, hits.hits().length); + } for (int j = 0; j < hits.hits().length; j++) { assertThat(hits.getHits()[j].score(), equalTo(matchResp.getHits().getHits()[j].score())); assertThat(hits.getHits()[j].getId(), equalTo(matchResp.getHits().getHits()[j].getId())); From 4450039cf6e6996cee60aabe06a29467b6f16e19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 29 Jul 2016 18:20:41 +0200 Subject: [PATCH 2/3] Try catching potential null query results and convert to MatchNoDocsQuery --- .../index/query/MultiMatchQueryBuilder.java | 8 ------- .../index/search/MatchQuery.java | 4 ++++ .../index/search/MultiMatchQuery.java | 24 +++++++++++-------- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java index 41b6829994e..1026717adcb 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java @@ -29,7 +29,6 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.lucene.search.MatchNoDocsQuery; import org.elasticsearch.common.regex.Regex; import org.elasticsearch.common.unit.Fuzziness; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -747,13 +746,6 @@ public class MultiMatchQueryBuilder extends AbstractQueryBuilder newFieldsBoosts = handleFieldsMatchPattern(context.getMapperService(), fieldsBoosts); Query query = multiMatchQuery.parse(type, newFieldsBoosts, value, minimumShouldMatch); - if (query == null) { - if (newFieldsBoosts.isEmpty()) { - return new MatchNoDocsQuery("[" + NAME + "] could not resolve any field names"); - } else { - return new MatchNoDocsQuery("[" + NAME + "] parsing resolved to null query"); - } - } return query; } diff --git a/core/src/main/java/org/elasticsearch/index/search/MatchQuery.java b/core/src/main/java/org/elasticsearch/index/search/MatchQuery.java index 835ec8e143f..b38d2101163 100644 --- a/core/src/main/java/org/elasticsearch/index/search/MatchQuery.java +++ b/core/src/main/java/org/elasticsearch/index/search/MatchQuery.java @@ -37,6 +37,7 @@ import org.elasticsearch.common.Nullable; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.lucene.search.MatchNoDocsQuery; import org.elasticsearch.common.lucene.search.MultiPhrasePrefixQuery; import org.elasticsearch.common.lucene.search.Queries; import org.elasticsearch.common.unit.Fuzziness; @@ -354,6 +355,9 @@ public class MatchQuery { if (query instanceof FuzzyQuery) { QueryParsers.setRewriteMethod((FuzzyQuery) query, fuzzyRewriteMethod); } + if (query == null) { + return new MatchNoDocsQuery("could not create fuzzy query for field type " + fieldType.name()); + } return query; } catch (RuntimeException e) { if (lenient) { diff --git a/core/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java b/core/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java index 115938a2c06..ce6b2cec706 100644 --- a/core/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java +++ b/core/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java @@ -23,14 +23,15 @@ import org.apache.lucene.analysis.Analyzer; import org.apache.lucene.index.Term; import org.apache.lucene.queries.BlendedTermQuery; import org.apache.lucene.search.BooleanClause; +import org.apache.lucene.search.BooleanClause.Occur; import org.apache.lucene.search.BooleanQuery; import org.apache.lucene.search.BoostQuery; import org.apache.lucene.search.DisjunctionMaxQuery; import org.apache.lucene.search.Query; import org.apache.lucene.search.TermQuery; import org.apache.lucene.util.BytesRef; -import org.apache.lucene.search.BooleanClause.Occur; import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.lucene.search.MatchNoDocsQuery; import org.elasticsearch.common.lucene.search.Queries; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.query.AbstractQueryBuilder; @@ -72,14 +73,14 @@ public class MultiMatchQuery extends MatchQuery { } public Query parse(MultiMatchQueryBuilder.Type type, Map fieldNames, Object value, String minimumShouldMatch) throws IOException { + Query result; if (fieldNames.size() == 1) { Map.Entry fieldBoost = fieldNames.entrySet().iterator().next(); Float boostValue = fieldBoost.getValue(); - return parseAndApply(type.matchQueryType(), fieldBoost.getKey(), value, minimumShouldMatch, boostValue); - } - - final float tieBreaker = groupTieBreaker == null ? type.tieBreaker() : groupTieBreaker; - switch (type) { + result = parseAndApply(type.matchQueryType(), fieldBoost.getKey(), value, minimumShouldMatch, boostValue); + } else { + final float tieBreaker = groupTieBreaker == null ? type.tieBreaker() : groupTieBreaker; + switch (type) { case PHRASE: case PHRASE_PREFIX: case BEST_FIELDS: @@ -91,9 +92,12 @@ public class MultiMatchQuery extends MatchQuery { break; default: throw new IllegalStateException("No such type: " + type); + } + final List queries = queryBuilder.buildGroupedQueries(type, fieldNames, value, minimumShouldMatch); + result = queryBuilder.combineGrouped(queries); } - final List queries = queryBuilder.buildGroupedQueries(type, fieldNames, value, minimumShouldMatch); - return queryBuilder.combineGrouped(queries); + assert result != null; + return result; } private QueryBuilder queryBuilder; @@ -127,9 +131,9 @@ public class MultiMatchQuery extends MatchQuery { return parseAndApply(type, field, value, minimumShouldMatch, boostValue); } - public Query combineGrouped(List groupQuery) { + private Query combineGrouped(List groupQuery) { if (groupQuery == null || groupQuery.isEmpty()) { - return null; + return new MatchNoDocsQuery("[multi_match] list of group queries was empty"); } if (groupQuery.size() == 1) { return groupQuery.get(0); From 0d7c289f4c23f7193678616814f7d5abe36b0315 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 29 Jul 2016 20:28:17 +0200 Subject: [PATCH 3/3] Adressing review comments --- .../index/query/MultiMatchQueryBuilder.java | 3 +-- .../index/search/MatchQuery.java | 4 ---- .../index/search/MultiMatchQuery.java | 22 +++++++++---------- 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java index 1026717adcb..6ffcb19ea3a 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/MultiMatchQueryBuilder.java @@ -745,8 +745,7 @@ public class MultiMatchQueryBuilder extends AbstractQueryBuilder newFieldsBoosts = handleFieldsMatchPattern(context.getMapperService(), fieldsBoosts); - Query query = multiMatchQuery.parse(type, newFieldsBoosts, value, minimumShouldMatch); - return query; + return multiMatchQuery.parse(type, newFieldsBoosts, value, minimumShouldMatch); } private static Map handleFieldsMatchPattern(MapperService mapperService, Map fieldsBoosts) { diff --git a/core/src/main/java/org/elasticsearch/index/search/MatchQuery.java b/core/src/main/java/org/elasticsearch/index/search/MatchQuery.java index b38d2101163..835ec8e143f 100644 --- a/core/src/main/java/org/elasticsearch/index/search/MatchQuery.java +++ b/core/src/main/java/org/elasticsearch/index/search/MatchQuery.java @@ -37,7 +37,6 @@ import org.elasticsearch.common.Nullable; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.lucene.search.MatchNoDocsQuery; import org.elasticsearch.common.lucene.search.MultiPhrasePrefixQuery; import org.elasticsearch.common.lucene.search.Queries; import org.elasticsearch.common.unit.Fuzziness; @@ -355,9 +354,6 @@ public class MatchQuery { if (query instanceof FuzzyQuery) { QueryParsers.setRewriteMethod((FuzzyQuery) query, fuzzyRewriteMethod); } - if (query == null) { - return new MatchNoDocsQuery("could not create fuzzy query for field type " + fieldType.name()); - } return query; } catch (RuntimeException e) { if (lenient) { diff --git a/core/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java b/core/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java index ce6b2cec706..1a5b3c02e10 100644 --- a/core/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java +++ b/core/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java @@ -81,17 +81,17 @@ public class MultiMatchQuery extends MatchQuery { } else { final float tieBreaker = groupTieBreaker == null ? type.tieBreaker() : groupTieBreaker; switch (type) { - case PHRASE: - case PHRASE_PREFIX: - case BEST_FIELDS: - case MOST_FIELDS: - queryBuilder = new QueryBuilder(tieBreaker); - break; - case CROSS_FIELDS: - queryBuilder = new CrossFieldsQueryBuilder(tieBreaker); - break; - default: - throw new IllegalStateException("No such type: " + type); + case PHRASE: + case PHRASE_PREFIX: + case BEST_FIELDS: + case MOST_FIELDS: + queryBuilder = new QueryBuilder(tieBreaker); + break; + case CROSS_FIELDS: + queryBuilder = new CrossFieldsQueryBuilder(tieBreaker); + break; + default: + throw new IllegalStateException("No such type: " + type); } final List queries = queryBuilder.buildGroupedQueries(type, fieldNames, value, minimumShouldMatch); result = queryBuilder.combineGrouped(queries);