From 9cdcea0e7a8c5c3d2b7b227379eebaf020d17f90 Mon Sep 17 00:00:00 2001 From: jimczi Date: Tue, 1 Oct 2019 17:10:20 +0200 Subject: [PATCH 1/2] Fix alias field resolution in match query Synonym queries (when two tokens/paths start at the same position) use the alias field instead of the concrete field to build Lucene queries. This commit fixes this bug by resolving the alias field upfront in order to provide the concrete field to the actual query parser. --- .../index/search/MatchQuery.java | 20 +++++++++---------- .../index/search/MultiMatchQuery.java | 2 +- .../index/search/QueryParserHelper.java | 19 +++++------------- .../index/query/MatchQueryBuilderTests.java | 13 ++++++++++++ 4 files changed, 29 insertions(+), 25 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/search/MatchQuery.java b/server/src/main/java/org/elasticsearch/index/search/MatchQuery.java index 72ffe019324f9..06b8fc86a2fc7 100644 --- a/server/src/main/java/org/elasticsearch/index/search/MatchQuery.java +++ b/server/src/main/java/org/elasticsearch/index/search/MatchQuery.java @@ -247,16 +247,16 @@ public Query parse(Type type, String fieldName, Object value) throws IOException * a prefix query instead */ if (analyzer == Lucene.KEYWORD_ANALYZER && type != Type.PHRASE_PREFIX) { - final Term term = new Term(fieldName, value.toString()); - if ((fieldType instanceof TextFieldMapper.TextFieldType || fieldType instanceof KeywordFieldMapper.KeywordFieldType) - && type == Type.BOOLEAN_PREFIX) { - return builder.newPrefixQuery(fieldName, term); + final Term term = new Term(fieldType.name(), value.toString()); + if (type == Type.BOOLEAN_PREFIX + && (fieldType instanceof TextFieldMapper.TextFieldType || fieldType instanceof KeywordFieldMapper.KeywordFieldType)) { + return builder.newPrefixQuery(term); } else { return builder.newTermQuery(term); } } - return parseInternal(type, fieldName, builder, value); + return parseInternal(type, fieldType.name(), builder, value); } protected final Query parseInternal(Type type, String fieldName, MatchQueryBuilder builder, Object value) throws IOException { @@ -540,12 +540,12 @@ protected Query newTermQuery(Term term) { /** * Builds a new prefix query instance. */ - protected Query newPrefixQuery(String field, Term term) { + protected Query newPrefixQuery(Term term) { try { return fieldType.prefixQuery(term.text(), null, context); } catch (RuntimeException e) { if (lenient) { - return newLenientFieldQuery(field, e); + return newLenientFieldQuery(term.field(), e); } throw e; } @@ -562,7 +562,7 @@ private Query analyzeTerm(String field, TokenStream stream, boolean isPrefix) th final Term term = new Term(field, termAtt.getBytesRef()); int lastOffset = offsetAtt.endOffset(); stream.end(); - return isPrefix && lastOffset == offsetAtt.endOffset() ? newPrefixQuery(field, term) : newTermQuery(term); + return isPrefix && lastOffset == offsetAtt.endOffset() ? newPrefixQuery(term) : newTermQuery(term); } private void add(BooleanQuery.Builder q, String field, List current, BooleanClause.Occur operator, boolean isPrefix) { @@ -571,7 +571,7 @@ private void add(BooleanQuery.Builder q, String field, List current, Boole } if (current.size() == 1) { if (isPrefix) { - q.add(newPrefixQuery(field, current.get(0)), operator); + q.add(newPrefixQuery(current.get(0)), operator); } else { q.add(newTermQuery(current.get(0)), operator); } @@ -688,7 +688,7 @@ public Query next() { Term[] terms = graph.getTerms(field, start); assert terms.length > 0; if (terms.length == 1) { - queryPos = usePrefix ? newPrefixQuery(field, terms[0]) : newTermQuery(terms[0]); + queryPos = usePrefix ? newPrefixQuery(terms[0]) : newTermQuery(terms[0]); } else { // We don't apply prefix on synonyms queryPos = newSynonymQuery(terms); diff --git a/server/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java b/server/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java index cbc06a6ff081d..8e33425b3981e 100644 --- a/server/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java +++ b/server/src/main/java/org/elasticsearch/index/search/MultiMatchQuery.java @@ -194,7 +194,7 @@ protected Query newTermQuery(Term term) { } @Override - protected Query newPrefixQuery(String field, Term term) { + protected Query newPrefixQuery(Term term) { List disjunctions = new ArrayList<>(); for (FieldAndBoost fieldType : blendedFields) { Query query = fieldType.fieldType.prefixQuery(term.text(), null, context); diff --git a/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java b/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java index 8d6198e17e2cc..ba509797c043e 100644 --- a/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java +++ b/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java @@ -30,7 +30,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Set; /** * Helpers to extract and expand field names and boosts @@ -52,7 +51,7 @@ public static Map parseFieldsAndWeights(List fields) { float boost = 1.0f; if (boostIndex != -1) { fieldName = field.substring(0, boostIndex); - boost = Float.parseFloat(field.substring(boostIndex+1, field.length())); + boost = Float.parseFloat(field.substring(boostIndex+1)); } else { fieldName = field; } @@ -131,10 +130,8 @@ public static Map resolveMappingField(QueryShardContext context, */ public static Map resolveMappingField(QueryShardContext context, String fieldOrPattern, float weight, boolean acceptAllTypes, boolean acceptMetadataField, String fieldSuffix) { - Set allFields = context.simpleMatchToIndexNames(fieldOrPattern); - Map fields = new HashMap<>(); - - for (String fieldName : allFields) { + final Map fields = new HashMap<>(); + for (String fieldName : context.simpleMatchToIndexNames(fieldOrPattern)) { if (fieldSuffix != null && context.fieldMapper(fieldName + fieldSuffix) != null) { fieldName = fieldName + fieldSuffix; } @@ -162,14 +159,8 @@ public static Map resolveMappingField(QueryShardContext context, } } - // Deduplicate aliases and their concrete fields. - String resolvedFieldName = fieldType.name(); - if (allFields.contains(resolvedFieldName)) { - fieldName = resolvedFieldName; - } - - float w = fields.getOrDefault(fieldName, 1.0F); - fields.put(fieldName, w * weight); + float w = fields.getOrDefault(fieldType.name(), 1.0F); + fields.put(fieldType.name(), w * weight); } checkForTooManyFields(fields, context); diff --git a/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java index 7d15c98af6a18..3425ce0a5e268 100644 --- a/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java @@ -31,6 +31,7 @@ import org.apache.lucene.search.PhraseQuery; import org.apache.lucene.search.PointRangeQuery; import org.apache.lucene.search.Query; +import org.apache.lucene.search.SynonymQuery; import org.apache.lucene.search.TermQuery; import org.apache.lucene.search.spans.SpanNearQuery; import org.apache.lucene.search.spans.SpanOrQuery; @@ -475,6 +476,18 @@ public void testMultiWordSynonymsPhrase() throws Exception { assertEquals(expected, actual); } + + public void testAliasWithSynonyms() throws Exception { + final MatchQuery matchQuery = new MatchQuery(createShardContext()); + matchQuery.setAnalyzer(new MockSynonymAnalyzer()); + final Query actual = matchQuery.parse(Type.PHRASE, STRING_ALIAS_FIELD_NAME, "dogs"); + Query expected = new SynonymQuery.Builder(STRING_FIELD_NAME) + .addTerm(new Term(STRING_FIELD_NAME, "dogs")) + .addTerm(new Term(STRING_FIELD_NAME, "dog")) + .build(); + assertEquals(expected, actual); + } + public void testMaxBooleanClause() { MatchQuery query = new MatchQuery(createShardContext()); query.setAnalyzer(new MockGraphAnalyzer(createGiantGraph(40))); From 7324bde4958b5ef934f0b3dfcaccd144c66367bb Mon Sep 17 00:00:00 2001 From: jimczi Date: Tue, 1 Oct 2019 19:42:38 +0200 Subject: [PATCH 2/2] fix handling of flattened type --- .../elasticsearch/index/search/MatchQuery.java | 13 +++++++++++-- .../index/search/QueryParserHelper.java | 17 +++++++++++++---- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/search/MatchQuery.java b/server/src/main/java/org/elasticsearch/index/search/MatchQuery.java index 06b8fc86a2fc7..d42466b5ebe66 100644 --- a/server/src/main/java/org/elasticsearch/index/search/MatchQuery.java +++ b/server/src/main/java/org/elasticsearch/index/search/MatchQuery.java @@ -59,6 +59,7 @@ import java.util.ArrayList; import java.util.Iterator; import java.util.List; +import java.util.Set; import java.util.function.Supplier; import static org.elasticsearch.common.lucene.search.Queries.newLenientFieldQuery; @@ -235,6 +236,14 @@ public Query parse(Type type, String fieldName, Object value) throws IOException if (fieldType == null) { return newUnmappedFieldQuery(fieldName); } + Set fields = context.simpleMatchToIndexNames(fieldName); + if (fields.contains(fieldName)) { + assert fields.size() == 1; + // this field is a concrete field or an alias so we use the + // field type name directly + fieldName = fieldType.name(); + } + Analyzer analyzer = getAnalyzer(fieldType, type == Type.PHRASE || type == Type.PHRASE_PREFIX); assert analyzer != null; @@ -247,7 +256,7 @@ public Query parse(Type type, String fieldName, Object value) throws IOException * a prefix query instead */ if (analyzer == Lucene.KEYWORD_ANALYZER && type != Type.PHRASE_PREFIX) { - final Term term = new Term(fieldType.name(), value.toString()); + final Term term = new Term(fieldName, value.toString()); if (type == Type.BOOLEAN_PREFIX && (fieldType instanceof TextFieldMapper.TextFieldType || fieldType instanceof KeywordFieldMapper.KeywordFieldType)) { return builder.newPrefixQuery(term); @@ -256,7 +265,7 @@ public Query parse(Type type, String fieldName, Object value) throws IOException } } - return parseInternal(type, fieldType.name(), builder, value); + return parseInternal(type, fieldName, builder, value); } protected final Query parseInternal(Type type, String fieldName, MatchQueryBuilder builder, Object value) throws IOException { diff --git a/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java b/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java index ba509797c043e..e4b397c9a538b 100644 --- a/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java +++ b/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java @@ -30,6 +30,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; /** * Helpers to extract and expand field names and boosts @@ -130,8 +131,10 @@ public static Map resolveMappingField(QueryShardContext context, */ public static Map resolveMappingField(QueryShardContext context, String fieldOrPattern, float weight, boolean acceptAllTypes, boolean acceptMetadataField, String fieldSuffix) { - final Map fields = new HashMap<>(); - for (String fieldName : context.simpleMatchToIndexNames(fieldOrPattern)) { + Set allFields = context.simpleMatchToIndexNames(fieldOrPattern); + Map fields = new HashMap<>(); + + for (String fieldName : allFields) { if (fieldSuffix != null && context.fieldMapper(fieldName + fieldSuffix) != null) { fieldName = fieldName + fieldSuffix; } @@ -159,8 +162,14 @@ public static Map resolveMappingField(QueryShardContext context, } } - float w = fields.getOrDefault(fieldType.name(), 1.0F); - fields.put(fieldType.name(), w * weight); + // Deduplicate aliases and their concrete fields. + String resolvedFieldName = fieldType.name(); + if (allFields.contains(resolvedFieldName)) { + fieldName = resolvedFieldName; + } + + float w = fields.getOrDefault(fieldName, 1.0F); + fields.put(fieldName, w * weight); } checkForTooManyFields(fields, context);