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

Search optimisation - add canMatch early aborts for queries on "_index" field #48681

Merged
merged 4 commits into from
Nov 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

BytesRefs.toString is not needed ?

Suggested change
if (shardContext != null && shardContext.indexMatches(BytesRefs.toString(value)) == false) {
if (shardContext != null && shardContext.indexMatches(value) == false) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unlike wildcardquery value is an object not a string. I remember having a test failure where I assumed String and it was a BytesRef so had to add this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The prefix change suggestion looks to have fixed things, thanks

return new MatchNoneQueryBuilder();
}
}
return super.doRewrite(queryRewriteContext);
}

@Override
protected Query doToQuery(QueryShardContext context) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
Loading