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.

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

Exists queries to MatchNoneQueryBuilder when the field is unmapped #54857

Merged
merged 13 commits into from
Apr 27, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,18 @@ public String fieldName() {
return this.fieldName;
}

@Override
protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOException {
QueryShardContext context = queryShardContext.convertToShardContext();
if (context != null) {
Collection<String> fields = getMappedField(context, fieldName);
if (fields.isEmpty()) {
return new MatchNoneQueryBuilder();
}
}
return super.doRewrite(queryShardContext);
}

@Override
protected void doXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(NAME);
Expand Down Expand Up @@ -129,20 +141,10 @@ protected Query doToQuery(QueryShardContext context) throws IOException {

public static Query newFilter(QueryShardContext context, String fieldPattern) {

final FieldNamesFieldMapper.FieldNamesFieldType fieldNamesFieldType = (FieldNamesFieldMapper.FieldNamesFieldType) context
.getMapperService().fieldType(FieldNamesFieldMapper.NAME);
if (fieldNamesFieldType == null) {
// can only happen when no types exist, so no docs exist either
return Queries.newMatchNoDocsQuery("Missing types in \"" + NAME + "\" query.");
}
Collection<String> fields = getMappedField(context, fieldPattern);

final Collection<String> fields;
if (context.getObjectMapper(fieldPattern) != null) {
// the _field_names field also indexes objects, so we don't have to
// do any more work to support exists queries on whole objects
fields = Collections.singleton(fieldPattern);
} else {
fields = context.simpleMatchToIndexNames(fieldPattern);
if (fields.isEmpty()) {
throw new IllegalStateException("Rewrite first");
}

if (fields.size() == 1) {
Expand All @@ -165,7 +167,7 @@ private static Query newFieldExistsQuery(QueryShardContext context, String field
if (context.getObjectMapper(field) != null) {
return newObjectFieldExistsQuery(context, field);
}
return Queries.newMatchNoDocsQuery("No field \"" + field + "\" exists in mappings.");
return Queries.newMatchNoDocsQuery("User requested \"match_none\" query.");
jpountz marked this conversation as resolved.
Show resolved Hide resolved
}
Query filter = fieldType.existsQuery(context);
return new ConstantScoreQuery(filter);
Expand All @@ -181,6 +183,43 @@ private static Query newObjectFieldExistsQuery(QueryShardContext context, String
return new ConstantScoreQuery(booleanQuery.build());
}

/**
* Helper method to get field mapped to this fieldPattern
* @return return collection of fields if exists else return empty.
*/
private static Collection<String> getMappedField(QueryShardContext context, String fieldPattern) {
final FieldNamesFieldMapper.FieldNamesFieldType fieldNamesFieldType = (FieldNamesFieldMapper.FieldNamesFieldType) context
.getMapperService().fieldType(FieldNamesFieldMapper.NAME);

if (fieldNamesFieldType == null) {
// can only happen when no types exist, so no docs exist either
return Collections.emptySet();
}

final Collection<String> fields;
if (context.getObjectMapper(fieldPattern) != null) {
// the _field_names field also indexes objects, so we don't have to
// do any more work to support exists queries on whole objects
fields = Collections.singleton(fieldPattern);
} else {
fields = context.simpleMatchToIndexNames(fieldPattern);
}

if (fields.size() == 1) {
String field = fields.iterator().next();
MappedFieldType fieldType = context.getMapperService().fieldType(field);
if (fieldType == null) {
// The field does not exist as a leaf but could be an object so
// check for an object mapper
if (context.getObjectMapper(field) == null) {
return Collections.emptySet();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that this entire if statement can be removed without affecting correctness. It is not possible to have a field that is neither and object nor a field at this stage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in this, if statement we are checking the field type.
MappedFieldType fieldType = context.getMapperService().fieldType(field);

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks I had the wrong assumption that simpleMatchToIndexName would return an empty set if the field is not mapped.


return fields;
}
SivagurunathanV marked this conversation as resolved.
Show resolved Hide resolved

@Override
protected int doHashCode() {
return Objects.hash(fieldName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import java.util.List;
import java.util.stream.Collectors;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.instanceOf;

Expand Down Expand Up @@ -63,11 +62,9 @@ protected void doAssertLuceneQuery(ExistsQueryBuilder queryBuilder, Query query,
Collection<String> fields = context.simpleMatchToIndexNames(fieldPattern);
Collection<String> mappedFields = fields.stream().filter((field) -> context.getObjectMapper(field) != null
|| context.getMapperService().fieldType(field) != null).collect(Collectors.toList());
if (fields.size() == 1 && mappedFields.size() == 0) {
if (mappedFields.size() == 0) {
assertThat(query, instanceOf(MatchNoDocsQuery.class));
MatchNoDocsQuery matchNoDocsQuery = (MatchNoDocsQuery) query;
assertThat(matchNoDocsQuery.toString(null),
containsString("No field \"" + fields.iterator().next() + "\" exists in mappings."));
} else if (fields.size() == 1) {
assertThat(query, instanceOf(ConstantScoreQuery.class));
ConstantScoreQuery constantScoreQuery = (ConstantScoreQuery) query;
Expand Down Expand Up @@ -108,6 +105,16 @@ protected void doAssertLuceneQuery(ExistsQueryBuilder queryBuilder, Query query,
}
}

@Override
public void testMustRewrite() throws IOException {
QueryShardContext context = createShardContext();
context.setAllowUnmappedFields(true);
ExistsQueryBuilder queryBuilder = new ExistsQueryBuilder("foo");
IllegalStateException e = expectThrows(IllegalStateException.class,
() -> queryBuilder.toQuery(context));
assertEquals("Rewrite first", e.getMessage());
}

public void testIllegalArguments() {
expectThrows(IllegalArgumentException.class, () -> new ExistsQueryBuilder((String) null));
expectThrows(IllegalArgumentException.class, () -> new ExistsQueryBuilder(""));
Expand Down