From c3745b03ee032d77290471e391a1336d038b7f66 Mon Sep 17 00:00:00 2001 From: markharwood Date: Fri, 15 Nov 2019 16:50:32 +0000 Subject: [PATCH] Search optimisation - add canMatch early aborts for queries on "_index" field (#49158) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make queries on the “_index” field fast-fail if the target shard is an index that doesn’t match the query expression. Part of the “canMatch” phase optimisations. Closes #48473 --- .../test/multi_cluster/70_skip_shards.yml | 163 ++++++++++++++++++ .../multi_cluster/90_index_name_query.yml | 44 +++++ .../index/query/PrefixQueryBuilder.java | 13 ++ .../index/query/TermQueryBuilder.java | 13 ++ .../index/query/TermsQueryBuilder.java | 15 ++ .../index/query/WildcardQueryBuilder.java | 16 +- .../index/query/PrefixQueryBuilderTests.java | 15 ++ .../index/query/TermQueryBuilderTests.java | 16 +- .../index/query/TermsQueryBuilderTests.java | 17 +- .../query/WildcardQueryBuilderTests.java | 16 ++ .../test/AbstractBuilderTestCase.java | 9 +- 11 files changed, 333 insertions(+), 4 deletions(-) diff --git a/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/70_skip_shards.yml b/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/70_skip_shards.yml index 9242664d9f219..92ae11c712b25 100644 --- a/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/70_skip_shards.yml +++ b/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/70_skip_shards.yml @@ -58,3 +58,166 @@ - match: { _shards.failed: 0 } - match: { hits.total: 1 } +--- +"Test that queries on _index field that don't match alias are skipped": + + - do: + indices.create: + index: skip_shards_local_index + body: + settings: + index: + number_of_shards: 2 + number_of_replicas: 0 + mappings: + properties: + created_at: + type: date + format: "yyyy-MM-dd" + + - do: + bulk: + refresh: true + body: + - '{"index": {"_index": "skip_shards_local_index"}}' + - '{"f1": "local_cluster", "sort_field": 0, "created_at" : "2017-01-01"}' + - '{"index": {"_index": "skip_shards_local_index"}}' + - '{"f1": "local_cluster", "sort_field": 1, "created_at" : "2017-01-02"}' + - do: + indices.put_alias: + index: skip_shards_local_index + name: test_skip_alias + + # check that we match the alias with term query + - do: + search: + track_total_hits: true + index: "skip_shards_local_index" + pre_filter_shard_size: 1 + ccs_minimize_roundtrips: false + body: { "size" : 10, "query" : { "term" : { "_index" : "test_skip_alias" } } } + + - match: { hits.total.value: 2 } + - match: { hits.hits.0._index: "skip_shards_local_index"} + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + - match: { _shards.skipped : 0} + - match: { _shards.failed: 0 } + + # check that we match the alias with terms query + - do: + search: + track_total_hits: true + index: "skip_shards_local_index" + pre_filter_shard_size: 1 + ccs_minimize_roundtrips: false + body: { "size" : 10, "query" : { "terms" : { "_index" : ["test_skip_alias", "does_not_match"] } } } + + - match: { hits.total.value: 2 } + - match: { hits.hits.0._index: "skip_shards_local_index"} + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + - match: { _shards.skipped : 0} + - match: { _shards.failed: 0 } + + # check that we match the alias with prefix query + - do: + search: + track_total_hits: true + index: "skip_shards_local_index" + pre_filter_shard_size: 1 + ccs_minimize_roundtrips: false + body: { "size" : 10, "query" : { "prefix" : { "_index" : "test_skip_ali" } } } + + - match: { hits.total.value: 2 } + - match: { hits.hits.0._index: "skip_shards_local_index"} + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + - match: { _shards.skipped : 0} + - match: { _shards.failed: 0 } + + # check that we match the alias with wildcard query + - do: + search: + track_total_hits: true + index: "skip_shards_local_index" + pre_filter_shard_size: 1 + ccs_minimize_roundtrips: false + body: { "size" : 10, "query" : { "wildcard" : { "_index" : "test_skip_ali*" } } } + + - match: { hits.total.value: 2 } + - match: { hits.hits.0._index: "skip_shards_local_index"} + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + - match: { _shards.skipped : 0} + - match: { _shards.failed: 0 } + + + # check that skipped when we don't match the alias with a term query + - do: + search: + track_total_hits: true + index: "skip_shards_local_index" + pre_filter_shard_size: 1 + ccs_minimize_roundtrips: false + body: { "size" : 10, "query" : { "term" : { "_index" : "does_not_match" } } } + + + - match: { hits.total.value: 0 } + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + # When all shards are skipped current logic returns 1 to produce a valid search result + - match: { _shards.skipped : 1} + - match: { _shards.failed: 0 } + + # check that skipped when we don't match the alias with a terms query + - do: + search: + track_total_hits: true + index: "skip_shards_local_index" + pre_filter_shard_size: 1 + ccs_minimize_roundtrips: false + body: { "size" : 10, "query" : { "terms" : { "_index" : ["does_not_match", "also_does_not_match"] } } } + + + - match: { hits.total.value: 0 } + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + # When all shards are skipped current logic returns 1 to produce a valid search result + - match: { _shards.skipped : 1} + - match: { _shards.failed: 0 } + + # check that skipped when we don't match the alias with a prefix query + - do: + search: + track_total_hits: true + index: "skip_shards_local_index" + pre_filter_shard_size: 1 + ccs_minimize_roundtrips: false + body: { "size" : 10, "query" : { "prefix" : { "_index" : "does_not_matc" } } } + + + - match: { hits.total.value: 0 } + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + # When all shards are skipped current logic returns 1 to produce a valid search result + - match: { _shards.skipped : 1} + - match: { _shards.failed: 0 } + + # check that skipped when we don't match the alias with a wildcard query + - do: + search: + track_total_hits: true + index: "skip_shards_local_index" + pre_filter_shard_size: 1 + ccs_minimize_roundtrips: false + body: { "size" : 10, "query" : { "wildcard" : { "_index" : "does_not_matc*" } } } + + + - match: { hits.total.value: 0 } + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + # When all shards are skipped current logic returns 1 to produce a valid search result + - match: { _shards.skipped : 1} + - match: { _shards.failed: 0 } + diff --git a/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/90_index_name_query.yml b/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/90_index_name_query.yml index 030dad662df59..a60a1b0d812ee 100644 --- a/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/90_index_name_query.yml +++ b/qa/multi-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/90_index_name_query.yml @@ -56,3 +56,47 @@ teardown: - match: { _shards.successful: 2 } - match: { _shards.skipped : 0} - match: { _shards.failed: 0 } + +--- +"Test that queries on _index that don't match are skipped": + + - do: + bulk: + refresh: true + body: + - '{"index": {"_index": "single_doc_index"}}' + - '{"f1": "local_cluster", "sort_field": 0}' + + - do: + search: + ccs_minimize_roundtrips: false + track_total_hits: true + index: "single_doc_index,my_remote_cluster:single_doc_index" + pre_filter_shard_size: 1 + body: + query: + term: + "_index": "does_not_match" + + - match: { hits.total.value: 0 } + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + - match: { _shards.skipped : 1} + - match: { _shards.failed: 0 } + + - do: + search: + ccs_minimize_roundtrips: false + track_total_hits: true + index: "single_doc_index,my_remote_cluster:single_doc_index" + pre_filter_shard_size: 1 + body: + query: + term: + "_index": "my_remote_cluster:does_not_match" + + - match: { hits.total.value: 0 } + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + - match: { _shards.skipped : 1} + - match: { _shards.failed: 0 } diff --git a/server/src/main/java/org/elasticsearch/index/query/PrefixQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/PrefixQueryBuilder.java index eacb2be100c98..db596e2ecfc7b 100644 --- a/server/src/main/java/org/elasticsearch/index/query/PrefixQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/PrefixQueryBuilder.java @@ -168,6 +168,19 @@ public static PrefixQueryBuilder fromXContent(XContentParser parser) throws IOEx public String getWriteableName() { return NAME; } + + @Override + protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { + if ("_index".equals(fieldName)) { + // Special-case optimisation for canMatch phase: + // We can skip querying this shard if the index name doesn't match the value of this query on the "_index" field. + QueryShardContext shardContext = queryRewriteContext.convertToShardContext(); + if (shardContext != null && shardContext.indexMatches(value + "*") == false) { + return new MatchNoneQueryBuilder(); + } + } + return super.doRewrite(queryRewriteContext); + } @Override protected Query doToQuery(QueryShardContext context) throws IOException { diff --git a/server/src/main/java/org/elasticsearch/index/query/TermQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/TermQueryBuilder.java index c35aa9b03d581..262bfb2c6b5b3 100644 --- a/server/src/main/java/org/elasticsearch/index/query/TermQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/TermQueryBuilder.java @@ -129,6 +129,19 @@ public static TermQueryBuilder fromXContent(XContentParser parser) throws IOExce } return termQuery; } + + @Override + protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { + if ("_index".equals(fieldName)) { + // Special-case optimisation for canMatch phase: + // We can skip querying this shard if the index name doesn't match the value of this query on the "_index" field. + QueryShardContext shardContext = queryRewriteContext.convertToShardContext(); + if (shardContext != null && shardContext.indexMatches(BytesRefs.toString(value)) == false) { + return new MatchNoneQueryBuilder(); + } + } + return super.doRewrite(queryRewriteContext); + } @Override protected Query doToQuery(QueryShardContext context) throws IOException { diff --git a/server/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java index 2f4f934727dc7..4cf205ea5b240 100644 --- a/server/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java @@ -499,6 +499,21 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) { }))); return new TermsQueryBuilder(this.fieldName, supplier::get); } + if ("_index".equals(this.fieldName) && values != null) { + // Special-case optimisation for canMatch phase: + // We can skip querying this shard if the index name doesn't match any of the search terms. + QueryShardContext shardContext = queryRewriteContext.convertToShardContext(); + if (shardContext != null) { + for (Object localValue : values) { + if (shardContext.indexMatches(BytesRefs.toString(localValue))) { + // We can match - at least one index name matches + return this; + } + } + // all index names are invalid - no possibility of a match on this shard. + return new MatchNoneQueryBuilder(); + } + } return this; } } diff --git a/server/src/main/java/org/elasticsearch/index/query/WildcardQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/WildcardQueryBuilder.java index 0b855bd50a426..115fa8d476dfd 100644 --- a/server/src/main/java/org/elasticsearch/index/query/WildcardQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/WildcardQueryBuilder.java @@ -27,6 +27,7 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.lucene.BytesRefs; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; @@ -177,7 +178,20 @@ public static WildcardQueryBuilder fromXContent(XContentParser parser) throws IO .rewrite(rewrite) .boost(boost) .queryName(queryName); - } + } + + @Override + protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { + if ("_index".equals(fieldName)) { + // Special-case optimisation for canMatch phase: + // We can skip querying this shard if the index name doesn't match the value of this query on the "_index" field. + QueryShardContext shardContext = queryRewriteContext.convertToShardContext(); + if (shardContext != null && shardContext.indexMatches(BytesRefs.toString(value)) == false) { + return new MatchNoneQueryBuilder(); + } + } + return super.doRewrite(queryRewriteContext); + } @Override protected Query doToQuery(QueryShardContext context) throws IOException { diff --git a/server/src/test/java/org/elasticsearch/index/query/PrefixQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/PrefixQueryBuilderTests.java index ee56a67092d70..dba92d712c107 100644 --- a/server/src/test/java/org/elasticsearch/index/query/PrefixQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/PrefixQueryBuilderTests.java @@ -141,4 +141,19 @@ public void testParseFailsWithMultipleFields() throws IOException { e = expectThrows(ParsingException.class, () -> parseQuery(shortJson)); assertEquals("[prefix] query doesn't support multiple fields, found [user1] and [user2]", e.getMessage()); } + + public void testRewriteIndexQueryToMatchNone() throws Exception { + PrefixQueryBuilder query = prefixQuery("_index", "does_not_exist"); + QueryShardContext queryShardContext = createShardContext(); + QueryBuilder rewritten = query.rewrite(queryShardContext); + assertThat(rewritten, instanceOf(MatchNoneQueryBuilder.class)); + } + + public void testRewriteIndexQueryToNotMatchNone() throws Exception { + PrefixQueryBuilder query = prefixQuery("_index", getIndex().getName()); + QueryShardContext queryShardContext = createShardContext(); + QueryBuilder rewritten = query.rewrite(queryShardContext); + assertThat(rewritten, instanceOf(PrefixQueryBuilder.class)); + } + } diff --git a/server/src/test/java/org/elasticsearch/index/query/TermQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/TermQueryBuilderTests.java index 48137b2726fe2..0bf6ddbc57438 100644 --- a/server/src/test/java/org/elasticsearch/index/query/TermQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/TermQueryBuilderTests.java @@ -172,5 +172,19 @@ public void testTypeField() throws IOException { TermQueryBuilder builder = QueryBuilders.termQuery("_type", "value1"); builder.doToQuery(createShardContext()); assertWarnings(QueryShardContext.TYPES_DEPRECATION_MESSAGE); - } + } + + public void testRewriteIndexQueryToMatchNone() throws IOException { + TermQueryBuilder query = QueryBuilders.termQuery("_index", "does_not_exist"); + QueryShardContext queryShardContext = createShardContext(); + QueryBuilder rewritten = query.rewrite(queryShardContext); + assertThat(rewritten, instanceOf(MatchNoneQueryBuilder.class)); + } + + public void testRewriteIndexQueryToNotMatchNone() throws IOException { + TermQueryBuilder query = QueryBuilders.termQuery("_index", getIndex().getName()); + QueryShardContext queryShardContext = createShardContext(); + QueryBuilder rewritten = query.rewrite(queryShardContext); + assertThat(rewritten, instanceOf(TermQueryBuilder.class)); + } } diff --git a/server/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java index a1df3241c8e92..4725961deceed 100644 --- a/server/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java @@ -316,7 +316,22 @@ public void testTypeField() throws IOException { builder.doToQuery(createShardContext()); assertWarnings(QueryShardContext.TYPES_DEPRECATION_MESSAGE); } - + + public void testRewriteIndexQueryToMatchNone() throws IOException { + TermsQueryBuilder query = new TermsQueryBuilder("_index", "does_not_exist", "also_does_not_exist"); + QueryShardContext queryShardContext = createShardContext(); + QueryBuilder rewritten = query.rewrite(queryShardContext); + assertThat(rewritten, instanceOf(MatchNoneQueryBuilder.class)); + } + + public void testRewriteIndexQueryToNotMatchNone() throws IOException { + // At least one name is good + TermsQueryBuilder query = new TermsQueryBuilder("_index", "does_not_exist", getIndex().getName()); + QueryShardContext queryShardContext = createShardContext(); + QueryBuilder rewritten = query.rewrite(queryShardContext); + assertThat(rewritten, instanceOf(TermsQueryBuilder.class)); + } + @Override protected QueryBuilder parseQuery(XContentParser parser) throws IOException { QueryBuilder query = super.parseQuery(parser); diff --git a/server/src/test/java/org/elasticsearch/index/query/WildcardQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/WildcardQueryBuilderTests.java index acb4d4f25cdcc..bf88ab9ee2da6 100644 --- a/server/src/test/java/org/elasticsearch/index/query/WildcardQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/WildcardQueryBuilderTests.java @@ -138,4 +138,20 @@ public void testTypeField() throws IOException { builder.doToQuery(createShardContext()); assertWarnings(QueryShardContext.TYPES_DEPRECATION_MESSAGE); } + + public void testRewriteIndexQueryToMatchNone() throws IOException { + WildcardQueryBuilder query = new WildcardQueryBuilder("_index", "does_not_exist"); + QueryShardContext queryShardContext = createShardContext(); + QueryBuilder rewritten = query.rewrite(queryShardContext); + assertThat(rewritten, instanceOf(MatchNoneQueryBuilder.class)); + } + + public void testRewriteIndexQueryNotMatchNone() throws IOException { + String fullIndexName = getIndex().getName(); + String firstHalfOfIndexName = fullIndexName.substring(0,fullIndexName.length()/2); + WildcardQueryBuilder query = new WildcardQueryBuilder("_index", firstHalfOfIndexName +"*"); + QueryShardContext queryShardContext = createShardContext(); + QueryBuilder rewritten = query.rewrite(queryShardContext); + assertThat(rewritten, instanceOf(WildcardQueryBuilder.class)); + } } diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java index 8465d48936fda..5367be61fa346 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java @@ -36,6 +36,7 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; +import org.elasticsearch.common.regex.Regex; import org.elasticsearch.common.settings.IndexScopedSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; @@ -92,6 +93,7 @@ import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.function.Function; +import java.util.function.Predicate; import java.util.stream.Stream; import static java.util.Collections.emptyList; @@ -399,6 +401,11 @@ public void onRemoval(ShardId shardId, Accountable accountable) { testCase.initializeAdditionalMappings(mapperService); } } + + public static Predicate indexNameMatcher() { + // Simplistic index name matcher used for testing + return pattern -> Regex.simpleMatch(pattern, index.getName()); + } @Override public void close() throws IOException { @@ -407,7 +414,7 @@ public void close() throws IOException { QueryShardContext createShardContext(IndexSearcher searcher) { return new QueryShardContext(0, idxSettings, BigArrays.NON_RECYCLING_INSTANCE, bitsetFilterCache, indexFieldDataService::getForField, mapperService, similarityService, scriptService, xContentRegistry, - namedWriteableRegistry, this.client, searcher, () -> nowInMillis, null, null); + namedWriteableRegistry, this.client, searcher, () -> nowInMillis, null, indexNameMatcher()); } ScriptModule createScriptModule(List scriptPlugins) {