Skip to content

Commit

Permalink
Generalize how queries on _index are handled at rewrite time (elast…
Browse files Browse the repository at this point in the history
…ic#52486)

Since this change refactors rewrites, I also took it as an opportunity to adrress elastic#49254: instead of returning the same queries you would get on a keyword field when a field is unmapped, queries get rewritten to a MatchNoDocsQueryBuilder.

This change exposed a couple bugs, like the fact that the percolator doesn't rewrite queries at query time, or that the significant_terms aggregation doesn't rewrite its inner filter, which I fixed.

Closes elastic#49254
  • Loading branch information
jpountz committed Feb 26, 2020
1 parent 37be695 commit 78f792c
Show file tree
Hide file tree
Showing 37 changed files with 622 additions and 347 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.apache.lucene.analysis.DelegatingAnalyzerWrapper;
import org.apache.lucene.index.BinaryDocValues;
import org.apache.lucene.index.DirectoryReader;
import org.apache.lucene.index.IndexReader;
import org.apache.lucene.index.IndexReaderContext;
import org.apache.lucene.index.IndexWriter;
import org.apache.lucene.index.IndexWriterConfig;
Expand Down Expand Up @@ -78,6 +79,7 @@
import org.elasticsearch.index.query.QueryRewriteContext;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.index.query.QueryShardException;
import org.elasticsearch.index.query.Rewriteable;
import org.elasticsearch.indices.breaker.CircuitBreakerService;
import org.elasticsearch.indices.breaker.NoneCircuitBreakerService;

Expand All @@ -91,7 +93,6 @@
import java.util.Objects;
import java.util.function.Supplier;

import static org.elasticsearch.percolator.PercolatorFieldMapper.parseQuery;
import static org.elasticsearch.search.SearchService.ALLOW_EXPENSIVE_QUERIES;

public class PercolateQueryBuilder extends AbstractQueryBuilder<PercolateQueryBuilder> {
Expand Down Expand Up @@ -646,9 +647,9 @@ protected Analyzer getWrappedAnalyzer(String fieldName) {
PercolatorFieldMapper.FieldType pft = (PercolatorFieldMapper.FieldType) fieldType;
String name = this.name != null ? this.name : pft.name();
QueryShardContext percolateShardContext = wrap(context);
PercolatorFieldMapper.configureContext(percolateShardContext, pft.mapUnmappedFieldsAsText);;
PercolateQuery.QueryStore queryStore = createStore(pft.queryBuilderField,
percolateShardContext,
pft.mapUnmappedFieldsAsText);
percolateShardContext);

return pft.percolateQuery(name, queryStore, documents, docSearcher, excludeNestedDocuments, context.indexVersionCreated());
}
Expand Down Expand Up @@ -695,8 +696,7 @@ static IndexSearcher createMultiDocumentSearcher(Analyzer analyzer, Collection<P
}

static PercolateQuery.QueryStore createStore(MappedFieldType queryBuilderFieldType,
QueryShardContext context,
boolean mapUnmappedFieldsAsString) {
QueryShardContext context) {
Version indexVersion = context.indexVersionCreated();
NamedWriteableRegistry registry = context.getWriteableRegistry();
return ctx -> {
Expand All @@ -723,7 +723,8 @@ static PercolateQuery.QueryStore createStore(MappedFieldType queryBuilderFieldTy
assert valueLength > 0;
QueryBuilder queryBuilder = input.readNamedWriteable(QueryBuilder.class);
assert in.read() == -1;
return PercolatorFieldMapper.toQuery(context, mapUnmappedFieldsAsString, queryBuilder);
queryBuilder = Rewriteable.rewrite(queryBuilder, context);
return queryBuilder.toQuery(context);
}
}
} else {
Expand All @@ -739,7 +740,10 @@ static PercolateQuery.QueryStore createStore(MappedFieldType queryBuilderFieldTy
try (XContentParser sourceParser = xContent
.createParser(context.getXContentRegistry(), LoggingDeprecationHandler.INSTANCE,
qbSource.bytes, qbSource.offset, qbSource.length)) {
return parseQuery(context, mapUnmappedFieldsAsString, sourceParser);
QueryBuilder queryBuilder = PercolatorFieldMapper.parseQueryBuilder(sourceParser,
sourceParser.getTokenLocation());
queryBuilder = Rewriteable.rewrite(queryBuilder, context);
return queryBuilder.toQuery(context);
}
} else {
return null;
Expand All @@ -755,6 +759,13 @@ static PercolateQuery.QueryStore createStore(MappedFieldType queryBuilderFieldTy
static QueryShardContext wrap(QueryShardContext shardContext) {
return new QueryShardContext(shardContext) {

@Override
public IndexReader getIndexReader() {
// The reader that matters in this context is not the reader of the shard but
// the reader of the MemoryIndex. We just use `null` for simplicity.
return null;
}

@Override
public BitSetProducer bitsetFilter(Query query) {
return context -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,8 @@ public void parse(ParseContext context) throws IOException {
throw new IllegalArgumentException("a document can only contain one percolator query");
}

configureContext(queryShardContext, isMapUnmappedFieldAsText());

XContentParser parser = context.parser();
QueryBuilder queryBuilder = parseQueryBuilder(
parser, parser.getTokenLocation()
Expand All @@ -410,7 +412,8 @@ public void parse(ParseContext context) throws IOException {
Version indexVersion = context.mapperService().getIndexSettings().getIndexVersionCreated();
createQueryBuilderField(indexVersion, queryBuilderField, queryBuilder, context);

Query query = toQuery(queryShardContext, isMapUnmappedFieldAsText(), queryBuilder);
QueryBuilder queryBuilderForProcessing = queryBuilder.rewrite(new QueryShardContext(queryShardContext));
Query query = queryBuilderForProcessing.toQuery(queryShardContext);
processQuery(query, context);
}

Expand Down Expand Up @@ -480,11 +483,7 @@ void processQuery(Query query, ParseContext context) {
}
}

static Query parseQuery(QueryShardContext context, boolean mapUnmappedFieldsAsString, XContentParser parser) throws IOException {
return toQuery(context, mapUnmappedFieldsAsString, parseQueryBuilder(parser, parser.getTokenLocation()));
}

static Query toQuery(QueryShardContext context, boolean mapUnmappedFieldsAsString, QueryBuilder queryBuilder) throws IOException {
static void configureContext(QueryShardContext context, boolean mapUnmappedFieldsAsString) {
// This means that fields in the query need to exist in the mapping prior to registering this query
// The reason that this is required, is that if a field doesn't exist then the query assumes defaults, which may be undesired.
//
Expand All @@ -499,7 +498,6 @@ static Query toQuery(QueryShardContext context, boolean mapUnmappedFieldsAsStrin
// as an analyzed string.
context.setAllowUnmappedFields(false);
context.setMapUnmappedFieldAsString(mapUnmappedFieldsAsString);
return queryBuilder.toQuery(context);
}

private static QueryBuilder parseQueryBuilder(XContentParser parser, XContentLocation location) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,12 @@
import org.elasticsearch.index.fielddata.plain.BytesBinaryDVIndexFieldData;
import org.elasticsearch.index.mapper.BinaryFieldMapper;
import org.elasticsearch.index.mapper.ContentPath;
import org.elasticsearch.index.mapper.KeywordFieldMapper;
import org.elasticsearch.index.mapper.Mapper;
import org.elasticsearch.index.mapper.ParseContext;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.index.query.TermQueryBuilder;
import org.elasticsearch.mock.orig.Mockito;
import org.elasticsearch.search.SearchModule;
import org.elasticsearch.test.ESTestCase;

Expand Down Expand Up @@ -93,7 +95,14 @@ public void testStoringQueryBuilders() throws IOException {
when(queryShardContext.getXContentRegistry()).thenReturn(xContentRegistry());
when(queryShardContext.getForField(fieldMapper.fieldType()))
.thenReturn(new BytesBinaryDVIndexFieldData(new Index("index", "uuid"), fieldMapper.name()));
PercolateQuery.QueryStore queryStore = PercolateQueryBuilder.createStore(fieldMapper.fieldType(), queryShardContext, false);
when(queryShardContext.fieldMapper(Mockito.anyString())).thenAnswer(invocation -> {
final String fieldName = (String) invocation.getArguments()[0];
KeywordFieldMapper.KeywordFieldType ft = new KeywordFieldMapper.KeywordFieldType();
ft.setName(fieldName);
ft.freeze();
return ft;
});
PercolateQuery.QueryStore queryStore = PercolateQueryBuilder.createStore(fieldMapper.fieldType(), queryShardContext);

try (IndexReader indexReader = DirectoryReader.open(directory)) {
LeafReaderContext leafContext = indexReader.leaves().get(0);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.index.mapper;

import org.apache.lucene.search.MatchAllDocsQuery;
import org.apache.lucene.search.MatchNoDocsQuery;
import org.apache.lucene.search.MultiTermQuery;
import org.apache.lucene.search.Query;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.lucene.search.Queries;
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.index.query.QueryShardContext;

import java.util.List;

/**
* A {@link MappedFieldType} that has the same value for all documents.
* Factory methods for queries are called at rewrite time so they should be
* cheap. In particular they should not read data from disk or perform a
* network call. Furthermore they may only return a {@link MatchAllDocsQuery}
* or a {@link MatchNoDocsQuery}.
*/
public abstract class ConstantFieldType extends MappedFieldType {

public ConstantFieldType() {
super();
}

public ConstantFieldType(ConstantFieldType other) {
super(other);
}

@Override
public final boolean isSearchable() {
return true;
}

@Override
public final boolean isAggregatable() {
return true;
}

@Override
public final Query existsQuery(QueryShardContext context) {
return new MatchAllDocsQuery();
}

/**
* Return whether the constant value of this field matches the provided {@code pattern}
* as documented in {@link Regex#simpleMatch}.
*/
protected abstract boolean matches(String pattern, QueryShardContext context);

private static String valueToString(Object value) {
return value instanceof BytesRef
? ((BytesRef) value).utf8ToString()
: value.toString();
}

@Override
public final Query termQuery(Object value, QueryShardContext context) {
String pattern = valueToString(value);
if (matches(pattern, context)) {
return Queries.newMatchAllQuery();
} else {
return new MatchNoDocsQuery();
}
}

@Override
public final Query termsQuery(List<?> values, QueryShardContext context) {
for (Object value : values) {
String pattern = valueToString(value);
if (matches(pattern, context)) {
// `terms` queries are a disjunction, so one matching term is enough
return Queries.newMatchAllQuery();
}
}
return new MatchNoDocsQuery();
}

@Override
public final Query prefixQuery(String prefix,
@Nullable MultiTermQuery.RewriteMethod method,
QueryShardContext context) {
String pattern = prefix + "*";
if (matches(pattern, context)) {
return Queries.newMatchAllQuery();
} else {
return new MatchNoDocsQuery();
}
}

@Override
public final Query wildcardQuery(String value,
@Nullable MultiTermQuery.RewriteMethod method,
QueryShardContext context) {
if (matches(value, context)) {
return Queries.newMatchAllQuery();
} else {
return new MatchNoDocsQuery();
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,7 @@

import org.apache.lucene.index.IndexOptions;
import org.apache.lucene.index.IndexableField;
import org.apache.lucene.search.MatchAllDocsQuery;
import org.apache.lucene.search.MultiTermQuery;
import org.apache.lucene.search.Query;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.lucene.Lucene;
import org.elasticsearch.common.lucene.search.Queries;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.index.fielddata.IndexFieldData;
Expand Down Expand Up @@ -89,7 +83,7 @@ public MetadataFieldMapper getDefault(MappedFieldType fieldType, ParserContext c
}
}

static final class IndexFieldType extends MappedFieldType {
static final class IndexFieldType extends ConstantFieldType {

IndexFieldType() {}

Expand All @@ -108,81 +102,8 @@ public String typeName() {
}

@Override
public boolean isSearchable() {
// The _index field is always searchable.
return true;
}

@Override
public Query existsQuery(QueryShardContext context) {
return new MatchAllDocsQuery();
}

/**
* This termQuery impl looks at the context to determine the index that
* is being queried and then returns a MATCH_ALL_QUERY or MATCH_NO_QUERY
* if the value matches this index. This can be useful if aliases or
* wildcards are used but the aim is to restrict the query to specific
* indices
*/
@Override
public Query termQuery(Object value, @Nullable QueryShardContext context) {
String pattern = value instanceof BytesRef
? ((BytesRef) value).utf8ToString()
: value.toString();
if (context.indexMatches(pattern)) {
// No need to OR these clauses - we can only logically be
// running in the context of just one of these index names.
return Queries.newMatchAllQuery();
} else {
return Queries.newMatchNoDocsQuery("The index [" + context.getFullyQualifiedIndex().getName() +
"] doesn't match the provided value [" + value + "].");
}
}

@Override
public Query termsQuery(List values, QueryShardContext context) {
if (context == null) {
return super.termsQuery(values, context);
}
for (Object value : values) {
String pattern = value instanceof BytesRef
? ((BytesRef) value).utf8ToString()
: value.toString();
if (context.indexMatches(pattern)) {
// No need to OR these clauses - we can only logically be
// running in the context of just one of these index names.
return Queries.newMatchAllQuery();
}
}
// None of the listed index names are this one
return Queries.newMatchNoDocsQuery("The index [" + context.getFullyQualifiedIndex().getName() +
"] doesn't match the provided values [" + values + "].");
}

@Override
public Query prefixQuery(String value,
@Nullable MultiTermQuery.RewriteMethod method,
QueryShardContext context) {
String pattern = value + "*";
if (context.indexMatches(pattern)) {
return Queries.newMatchAllQuery();
} else {
return Queries.newMatchNoDocsQuery("The index [" + context.getFullyQualifiedIndex().getName() +
"] doesn't match the provided prefix [" + value + "].");
}
}

@Override
public Query wildcardQuery(String value,
@Nullable MultiTermQuery.RewriteMethod method,
QueryShardContext context) {
if (context.indexMatches(value)) {
return Queries.newMatchAllQuery();
} else {
return Queries.newMatchNoDocsQuery("The index [" + context.getFullyQualifiedIndex().getName()
+ "] doesn't match the provided pattern [" + value + "].");
}
protected boolean matches(String pattern, QueryShardContext context) {
return context.indexMatches(pattern);
}

@Override
Expand Down
Loading

0 comments on commit 78f792c

Please sign in to comment.