Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change query field expansion #33020

Merged
merged 3 commits into from
Aug 23, 2018
Merged

Change query field expansion #33020

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
45197d4
Change query field expansion
jimczi Aug 21, 2018
cac2e94
Catch IAE instead of RuntimeException
jimczi Aug 21, 2018
b38d605
add ElasticsearchParseException
jimczi Aug 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -314,7 +315,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
Expand Down
71 changes: 14 additions & 57 deletions server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,46 +20,19 @@
package org.elasticsearch.index.search;

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() {}

/**
Expand All @@ -85,22 +58,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) {
DocumentMapper mapper = mapperService.documentMapper();
if (mapper != null) {
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);
Expand Down Expand Up @@ -138,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,
Expand All @@ -154,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
Expand All @@ -177,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 (RuntimeException e) {
// other exceptions are parsing errors or not indexed fields: keep
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not be throwing the exception here and reporting the problem back to the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this case is for field mappers that need a special input (numbers, boolean, ...) so this is consistent with the current behavior. The goal here is to select fields that can handle a term query so we silently ignore those that can't. Previously this selection was based on an hardcoded list of field types.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we narrow down the exception type here, eg. IAE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True thanks, it should be IAE only. I pushed the change.

}
}
fields.put(fieldName, weight);
}
Expand Down
4 changes: 2 additions & 2 deletions server/src/test/java/org/elasticsearch/search/query/QueryStringIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,8 +430,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();
Expand Down
8 changes: 4 additions & 4 deletions server/src/test/resources/org/elasticsearch/search/query/all-query-index.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand All @@ -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"}
}
}
Expand Down