From 090a672fddf9bc7bd3806d6578a4139834b6a6c1 Mon Sep 17 00:00:00 2001 From: markharwood Date: Wed, 30 Oct 2019 10:42:51 +0000 Subject: [PATCH 1/4] =?UTF-8?q?Make=20queries=20on=20the=20=E2=80=9C=5Find?= =?UTF-8?q?ex=E2=80=9D=20field=20fast-fail=20if=20the=20target=20shard=20i?= =?UTF-8?q?s=20an=20index=20that=20doesn=E2=80=99t=20match=20the=20query?= =?UTF-8?q?=20expression.=20Part=20of=20the=20=E2=80=9CcanMatch=E2=80=9D?= =?UTF-8?q?=20phase=20optimisations.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #48473 --- .../test/multi_cluster/70_skip_shards.yml | 98 +++++++++++++++++++ .../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 | 14 +++ .../test/AbstractBuilderTestCase.java | 8 +- 11 files changed, 265 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..3ab24cc6fa637 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,101 @@ - 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: + rest_total_hits_as_int: 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: 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: + rest_total_hits_as_int: 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: 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: + rest_total_hits_as_int: 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: 0 } + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + # Not sure why skipped is 1 instead of 2 - seems we always fully query at least one shard? + # Testing against index with single shard never skips and 2 shards only reports 1 shard skipped. + - match: { _shards.skipped : 1} + - match: { _shards.failed: 0 } + + # check that skipped when we don't match the alias with a terms query + - do: + search: + rest_total_hits_as_int: 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: 0 } + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + # Not sure why skipped is 1 instead of 2 - seems we always fully query at least one shard? + # Testing against index with single shard never skips and 2 shards only reports 1 shard skipped. + - 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..d0d752952bc48 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 + rest_total_hits_as_int: 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: 0 } + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + - match: { _shards.skipped : 1} + - match: { _shards.failed: 0 } + + - do: + search: + ccs_minimize_roundtrips: false + rest_total_hits_as_int: 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: 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..d5fd2bc3eb4fd 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.index().getName().startsWith(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 15e895ecf134d..2cececd041b18 100644 --- a/server/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java @@ -482,6 +482,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 0d1d39eae575d..b4a42e65e5f8a 100644 --- a/server/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java @@ -312,7 +312,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..df0a89ddd3cc7 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,18 @@ 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 testRewriteIndexQueryTNotoMatchNone() throws IOException { + WildcardQueryBuilder query = new WildcardQueryBuilder("_index", getIndex().getName()); + 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 28e058947d4c5..2078c000bbdc7 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java @@ -92,6 +92,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; @@ -398,6 +399,11 @@ public void onRemoval(ShardId shardId, Accountable accountable) { testCase.initializeAdditionalMappings(mapperService); } } + + public static Predicate indexNameMatcher() { + // Simplistic index name matcher used for testing + return pattern -> pattern.equals(index.getName()); + } @Override public void close() throws IOException { @@ -406,7 +412,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) { From 6d86cc3b1511d6d2b6e0e3638ece2bc083d59c67 Mon Sep 17 00:00:00 2001 From: markharwood Date: Wed, 13 Nov 2019 17:22:24 +0000 Subject: [PATCH 2/4] Addressing review comments - better test support for prefix and wildcard --- .../test/multi_cluster/70_skip_shards.yml | 69 +++++++++++++++++++ .../index/query/PrefixQueryBuilder.java | 2 +- .../query/WildcardQueryBuilderTests.java | 6 +- .../test/AbstractBuilderTestCase.java | 7 +- 4 files changed, 80 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 3ab24cc6fa637..f5778e6ca3a6b 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 @@ -120,6 +120,38 @@ - match: { _shards.skipped : 0} - match: { _shards.failed: 0 } + # check that we match the alias with prefix query + - do: + search: + rest_total_hits_as_int: 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: 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: + rest_total_hits_as_int: 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: 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: @@ -156,3 +188,40 @@ # Testing against index with single shard never skips and 2 shards only reports 1 shard skipped. - match: { _shards.skipped : 1} - match: { _shards.failed: 0 } + + # check that skipped when we don't match the alias with a prefix query + - do: + search: + rest_total_hits_as_int: 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: 0 } + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + # Not sure why skipped is 1 instead of 2 - seems we always fully query at least one shard? + # Testing against index with single shard never skips and 2 shards only reports 1 shard skipped. + - match: { _shards.skipped : 1} + - match: { _shards.failed: 0 } + + # check that skipped when we don't match the alias with a wildcard query + - do: + search: + rest_total_hits_as_int: 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: 0 } + - match: { _shards.total: 2 } + - match: { _shards.successful: 2 } + # Not sure why skipped is 1 instead of 2 - seems we always fully query at least one shard? + # Testing against index with single shard never skips and 2 shards only reports 1 shard skipped. + - 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 d5fd2bc3eb4fd..db596e2ecfc7b 100644 --- a/server/src/main/java/org/elasticsearch/index/query/PrefixQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/PrefixQueryBuilder.java @@ -175,7 +175,7 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws // 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.index().getName().startsWith(value) == false) { + if (shardContext != null && shardContext.indexMatches(value + "*") == false) { return new MatchNoneQueryBuilder(); } } 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 df0a89ddd3cc7..bf88ab9ee2da6 100644 --- a/server/src/test/java/org/elasticsearch/index/query/WildcardQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/WildcardQueryBuilderTests.java @@ -146,8 +146,10 @@ public void testRewriteIndexQueryToMatchNone() throws IOException { assertThat(rewritten, instanceOf(MatchNoneQueryBuilder.class)); } - public void testRewriteIndexQueryTNotoMatchNone() throws IOException { - WildcardQueryBuilder query = new WildcardQueryBuilder("_index", getIndex().getName()); + 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 2078c000bbdc7..619e2253109de 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java @@ -402,7 +402,12 @@ public void onRemoval(ShardId shardId, Accountable accountable) { public static Predicate indexNameMatcher() { // Simplistic index name matcher used for testing - return pattern -> pattern.equals(index.getName()); + return pattern ->{ + if (pattern.endsWith("*")) { + return index.getName().startsWith(pattern.substring(0, pattern.length()-1)); + } + return pattern.equals(index.getName()); + }; } @Override From 8e42ac47146d72f234248143a9dbdc2078202067 Mon Sep 17 00:00:00 2001 From: markharwood Date: Thu, 14 Nov 2019 10:04:46 +0000 Subject: [PATCH 3/4] =?UTF-8?q?Updated=20comments=20with=20explanation=20o?= =?UTF-8?q?f=20=E2=80=9Cskipped=E2=80=9D=20result?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../test/multi_cluster/70_skip_shards.yml | 12 ++++-------- 1 file changed, 4 insertions(+), 8 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 f5778e6ca3a6b..e549648e862c0 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 @@ -166,8 +166,7 @@ - match: { hits.total: 0 } - match: { _shards.total: 2 } - match: { _shards.successful: 2 } - # Not sure why skipped is 1 instead of 2 - seems we always fully query at least one shard? - # Testing against index with single shard never skips and 2 shards only reports 1 shard skipped. + # When all shards are skipped current logic returns 1 to produce a valid search result - match: { _shards.skipped : 1} - match: { _shards.failed: 0 } @@ -184,8 +183,7 @@ - match: { hits.total: 0 } - match: { _shards.total: 2 } - match: { _shards.successful: 2 } - # Not sure why skipped is 1 instead of 2 - seems we always fully query at least one shard? - # Testing against index with single shard never skips and 2 shards only reports 1 shard skipped. + # When all shards are skipped current logic returns 1 to produce a valid search result - match: { _shards.skipped : 1} - match: { _shards.failed: 0 } @@ -202,8 +200,7 @@ - match: { hits.total: 0 } - match: { _shards.total: 2 } - match: { _shards.successful: 2 } - # Not sure why skipped is 1 instead of 2 - seems we always fully query at least one shard? - # Testing against index with single shard never skips and 2 shards only reports 1 shard skipped. + # When all shards are skipped current logic returns 1 to produce a valid search result - match: { _shards.skipped : 1} - match: { _shards.failed: 0 } @@ -220,8 +217,7 @@ - match: { hits.total: 0 } - match: { _shards.total: 2 } - match: { _shards.successful: 2 } - # Not sure why skipped is 1 instead of 2 - seems we always fully query at least one shard? - # Testing against index with single shard never skips and 2 shards only reports 1 shard skipped. + # When all shards are skipped current logic returns 1 to produce a valid search result - match: { _shards.skipped : 1} - match: { _shards.failed: 0 } From 6ac08b17bb4f2fdccfca7ab9fdaa618fec7e7751 Mon Sep 17 00:00:00 2001 From: markharwood Date: Thu, 14 Nov 2019 17:10:52 +0000 Subject: [PATCH 4/4] Address jpountz review comments: * rest_total_hits_as_int -> track_total_hits * Use Regex.simpleMatch --- .../test/multi_cluster/70_skip_shards.yml | 32 +++++++++---------- .../multi_cluster/90_index_name_query.yml | 10 +++--- .../test/AbstractBuilderTestCase.java | 8 ++--- 3 files changed, 23 insertions(+), 27 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 e549648e862c0..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 @@ -91,13 +91,13 @@ # check that we match the alias with term query - do: search: - rest_total_hits_as_int: true + 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: 2 } + - match: { hits.total.value: 2 } - match: { hits.hits.0._index: "skip_shards_local_index"} - match: { _shards.total: 2 } - match: { _shards.successful: 2 } @@ -107,13 +107,13 @@ # check that we match the alias with terms query - do: search: - rest_total_hits_as_int: true + 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: 2 } + - match: { hits.total.value: 2 } - match: { hits.hits.0._index: "skip_shards_local_index"} - match: { _shards.total: 2 } - match: { _shards.successful: 2 } @@ -123,13 +123,13 @@ # check that we match the alias with prefix query - do: search: - rest_total_hits_as_int: true + 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: 2 } + - match: { hits.total.value: 2 } - match: { hits.hits.0._index: "skip_shards_local_index"} - match: { _shards.total: 2 } - match: { _shards.successful: 2 } @@ -139,13 +139,13 @@ # check that we match the alias with wildcard query - do: search: - rest_total_hits_as_int: true + 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: 2 } + - match: { hits.total.value: 2 } - match: { hits.hits.0._index: "skip_shards_local_index"} - match: { _shards.total: 2 } - match: { _shards.successful: 2 } @@ -156,14 +156,14 @@ # check that skipped when we don't match the alias with a term query - do: search: - rest_total_hits_as_int: true + 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: 0 } + - 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 @@ -173,14 +173,14 @@ # check that skipped when we don't match the alias with a terms query - do: search: - rest_total_hits_as_int: true + 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: 0 } + - 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 @@ -190,14 +190,14 @@ # check that skipped when we don't match the alias with a prefix query - do: search: - rest_total_hits_as_int: true + 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: 0 } + - 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 @@ -207,14 +207,14 @@ # check that skipped when we don't match the alias with a wildcard query - do: search: - rest_total_hits_as_int: true + 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: 0 } + - 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 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 d0d752952bc48..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 @@ -70,7 +70,7 @@ teardown: - do: search: ccs_minimize_roundtrips: false - rest_total_hits_as_int: true + track_total_hits: true index: "single_doc_index,my_remote_cluster:single_doc_index" pre_filter_shard_size: 1 body: @@ -78,7 +78,7 @@ teardown: term: "_index": "does_not_match" - - match: { hits.total: 0 } + - match: { hits.total.value: 0 } - match: { _shards.total: 2 } - match: { _shards.successful: 2 } - match: { _shards.skipped : 1} @@ -87,7 +87,7 @@ teardown: - do: search: ccs_minimize_roundtrips: false - rest_total_hits_as_int: true + track_total_hits: true index: "single_doc_index,my_remote_cluster:single_doc_index" pre_filter_shard_size: 1 body: @@ -95,8 +95,8 @@ teardown: term: "_index": "my_remote_cluster:does_not_match" - - match: { hits.total: 0 } + - match: { hits.total.value: 0 } - match: { _shards.total: 2 } - match: { _shards.successful: 2 } - match: { _shards.skipped : 1} - - match: { _shards.failed: 0 } + - match: { _shards.failed: 0 } 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 619e2253109de..e726b4dc15094 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; @@ -402,12 +403,7 @@ public void onRemoval(ShardId shardId, Accountable accountable) { public static Predicate indexNameMatcher() { // Simplistic index name matcher used for testing - return pattern ->{ - if (pattern.endsWith("*")) { - return index.getName().startsWith(pattern.substring(0, pattern.length()-1)); - } - return pattern.equals(index.getName()); - }; + return pattern -> Regex.simpleMatch(pattern, index.getName()); } @Override