Change query field expansion (#33020)

jimczi · jimczi · commit 500d533fa31c · 2018-08-23T10:27:42.000+02:00
This commit changes the query field expansion for query parsers to not rely on an hardcoded list of field types. Instead we rely on the type of exception that is thrown by MappedFieldType#termQuery to include/exclude an expanded field. Supersedes #31655 Closes #31798
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java
@@ -35,6 +35,7 @@
 import org.apache.lucene.search.TermInSetQuery;
 import org.apache.lucene.search.TermQuery;
 import org.apache.lucene.util.BytesRef;
+import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.geo.ShapeRelation;
 import org.elasticsearch.common.joda.DateMathParser;
@@ -336,7 +337,13 @@ public boolean isAggregatable() {
     /** Generates a query that will only match documents that contain the given value.
      *  The default implementation returns a {@link TermQuery} over the value bytes,
      *  boosted by {@link #boost()}.
-     *  @throws IllegalArgumentException if {@code value} cannot be converted to the expected data type */
+     *  @throws IllegalArgumentException if {@code value} cannot be converted to the expected data type or if the field is not searchable
+     *      due to the way it is configured (eg. not indexed)
+     *  @throws ElasticsearchParseException if {@code value} cannot be converted to the expected data type
+     *  @throws UnsupportedOperationException if the field is not searchable regardless of options
+     *  @throws QueryShardException if the field is not searchable regardless of options
+     */
+    // TODO: Standardize exception types
     public abstract Query termQuery(Object value, @Nullable QueryShardContext context);
 
     /** Build a constant-scoring query that matches all values. The default implementation uses a
diff --git a/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java b/server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java
@@ -19,47 +19,21 @@
 
 package org.elasticsearch.index.search;
 
+import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.regex.Regex;
-import org.elasticsearch.index.mapper.DateFieldMapper;
-import org.elasticsearch.index.mapper.DocumentMapper;
-import org.elasticsearch.index.mapper.FieldMapper;
-import org.elasticsearch.index.mapper.IpFieldMapper;
-import org.elasticsearch.index.mapper.KeywordFieldMapper;
 import org.elasticsearch.index.mapper.MappedFieldType;
-import org.elasticsearch.index.mapper.Mapper;
-import org.elasticsearch.index.mapper.MapperService;
-import org.elasticsearch.index.mapper.MetadataFieldMapper;
-import org.elasticsearch.index.mapper.NumberFieldMapper;
-import org.elasticsearch.index.mapper.TextFieldMapper;
 import org.elasticsearch.index.query.QueryShardContext;
+import org.elasticsearch.index.query.QueryShardException;
 
 import java.util.Collection;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 /**
  * Helpers to extract and expand field names and boosts
  */
 public final class QueryParserHelper {
-    // Mapping types the "all-ish" query can be executed against
-    // TODO: Fix the API so that we don't need a hardcoded list of types
-    private static final Set<String> ALLOWED_QUERY_MAPPER_TYPES;
-
-    static {
-        ALLOWED_QUERY_MAPPER_TYPES = new HashSet<>();
-        ALLOWED_QUERY_MAPPER_TYPES.add(DateFieldMapper.CONTENT_TYPE);
-        ALLOWED_QUERY_MAPPER_TYPES.add(IpFieldMapper.CONTENT_TYPE);
-        ALLOWED_QUERY_MAPPER_TYPES.add(KeywordFieldMapper.CONTENT_TYPE);
-        for (NumberFieldMapper.NumberType nt : NumberFieldMapper.NumberType.values()) {
-            ALLOWED_QUERY_MAPPER_TYPES.add(nt.typeName());
-        }
-        ALLOWED_QUERY_MAPPER_TYPES.add("scaled_float");
-        ALLOWED_QUERY_MAPPER_TYPES.add(TextFieldMapper.CONTENT_TYPE);
-    }
-
     private QueryParserHelper() {}
 
     /**
@@ -85,21 +59,6 @@ public static Map<String, Float> parseFieldsAndWeights(List<String> fields) {
         return fieldsAndWeights;
     }
 
-    /**
-     * Get a {@link FieldMapper} associated with a field name or null.
-     * @param mapperService The mapper service where to find the mapping.
-     * @param field The field name to search.
-     */
-    public static Mapper getFieldMapper(MapperService mapperService, String field) {
-        for (DocumentMapper mapper : mapperService.docMappers(true)) {
-            Mapper fieldMapper = mapper.mappers().getMapper(field);
-            if (fieldMapper != null) {
-                return fieldMapper;
-            }
-        }
-        return null;
-    }
-
     public static Map<String, Float> resolveMappingFields(QueryShardContext context,
                                                           Map<String, Float> fieldsAndWeights) {
         return resolveMappingFields(context, fieldsAndWeights, null);
@@ -136,8 +95,7 @@ public static Map<String, Float> resolveMappingFields(QueryShardContext context,
      * @param fieldOrPattern The field name or the pattern to resolve
      * @param weight The weight for the field
      * @param acceptAllTypes Whether all field type should be added when a pattern is expanded.
-     *                       If false, only {@link #ALLOWED_QUERY_MAPPER_TYPES} are accepted and other field types
-     *                       are discarded from the query.
+     *                       If false, only searchable field types are added.
      * @param acceptMetadataField Whether metadata fields should be added when a pattern is expanded.
      */
     public static Map<String, Float> resolveMappingField(QueryShardContext context, String fieldOrPattern, float weight,
@@ -152,8 +110,7 @@ public static Map<String, Float> resolveMappingField(QueryShardContext context,
      * @param fieldOrPattern The field name or the pattern to resolve
      * @param weight The weight for the field
      * @param acceptAllTypes Whether all field type should be added when a pattern is expanded.
-     *                       If false, only {@link #ALLOWED_QUERY_MAPPER_TYPES} are accepted and other field types
-     *                       are discarded from the query.
+     *                       If false, only searchable field types are added.
      * @param acceptMetadataField Whether metadata fields should be added when a pattern is expanded.
      * @param fieldSuffix The suffix name to add to the expanded field names if a mapping exists for that name.
      *                    The original name of the field is kept if adding the suffix to the field name does not point to a valid field
@@ -175,18 +132,20 @@ public static Map<String, Float> resolveMappingField(QueryShardContext context,
                 continue;
             }
 
-            // Ignore fields that are not in the allowed mapper types. Some
-            // types do not support term queries, and thus we cannot generate
-            // a special query for them.
-            String mappingType = fieldType.typeName();
-            if (acceptAllTypes == false && ALLOWED_QUERY_MAPPER_TYPES.contains(mappingType) == false) {
+            if (acceptMetadataField == false && fieldType.name().startsWith("_")) {
+                // Ignore metadata fields
                 continue;
             }
 
-            // Ignore metadata fields.
-            Mapper mapper = getFieldMapper(context.getMapperService(), fieldName);
-            if (acceptMetadataField == false && mapper instanceof MetadataFieldMapper) {
-                continue;
+            if (acceptAllTypes == false) {
+                try {
+                    fieldType.termQuery("", context);
+                } catch (QueryShardException |UnsupportedOperationException e) {
+                    // field type is never searchable with term queries (eg. geo point): ignore
+                    continue;
+                } catch (IllegalArgumentException |ElasticsearchParseException e) {
+                    // other exceptions are parsing errors or not indexed fields: keep
+                }
             }
             fields.put(fieldName, weight);
         }
diff --git a/server/src/test/java/org/elasticsearch/search/query/QueryStringIT.java b/server/src/test/java/org/elasticsearch/search/query/QueryStringIT.java
@@ -409,8 +409,8 @@ public void testFieldAliasOnDisallowedFieldType() throws Exception {
         indexRequests.add(client().prepareIndex("test", "_doc", "1").setSource("f3", "text", "f2", "one"));
         indexRandom(true, false, indexRequests);
 
-        // The wildcard field matches aliases for both a text and boolean field.
-        // By default, the boolean field should be ignored when building the query.
+        // The wildcard field matches aliases for both a text and geo_point field.
+        // By default, the geo_point field should be ignored when building the query.
         SearchResponse response = client().prepareSearch("test")
             .setQuery(queryStringQuery("text").field("f*_alias"))
             .execute().actionGet();
diff --git a/server/src/test/resources/org/elasticsearch/search/query/all-query-index.json b/server/src/test/resources/org/elasticsearch/search/query/all-query-index.json
@@ -46,10 +46,6 @@
           "format": "yyyy/MM/dd||epoch_millis"
         },
         "f_bool": {"type": "boolean"},
-        "f_bool_alias": {
-          "type": "alias",
-          "path": "f_bool"
-        },
         "f_byte": {"type": "byte"},
         "f_short": {"type": "short"},
         "f_int": {"type": "integer"},
@@ -60,6 +56,10 @@
         "f_binary": {"type": "binary"},
         "f_suggest": {"type": "completion"},
         "f_geop": {"type": "geo_point"},
+        "f_geop_alias": {
+          "type": "alias",
+          "path": "f_geop"
+        },
         "f_geos": {"type": "geo_shape"}
       }
     }
