Search optimisation - add canMatch early aborts for queries on "_index" field by markharwood · Pull Request #48681 · elastic/elasticsearch

markharwood · 2019-10-30T10:44:53Z

Make queries on the “_index” field fast-fail if the target shard is an index name that doesn’t match the query expression. Part of the “canMatch” phase optimisations.

Closes #48473

elasticmachine · 2019-10-30T10:45:25Z

Pinging @elastic/es-search (:Search/Search)

jimczi

The logic looks good to me. The original issue also mentions terms query and it should also be possible to add the logic to the prefix query ?

server/src/main/java/org/elasticsearch/index/query/TermQueryBuilder.java

jpountz

The approach looks good to me in general, can you add tests with aliases too?

jpountz · 2019-11-02T17:24:58Z

server/src/main/java/org/elasticsearch/index/query/PrefixQueryBuilder.java

We should try to use the indexNameMatcher so that it works with aliases too?

I did wonder about supporting aliases but wouldn't that give false negatives? I assume the indexed term in the _index field is only the physical index name so it would be a mistake to fail the canMatch phase if a query term happens to match an alias? My assumption was this issue is about optimising existing index name matching behaviour, not changing it to add alias support.

Queries on _index already support matching aliases since #46640, so this wouldn't change the behavior. See #23306 for the motivation.

Got it, thanks

Should it be:

Suggested change

if (shardContext != null && shardContext.index().getName().startsWith(value) == false) {

if (shardContext != null && shardContext.indexMatches(value + "*") == false) {

?

markharwood · 2019-11-13T14:10:38Z

I've added tests but a couple of anomalies exist:

Prefix and wildcard queries on "_index" field work with physical index names but not alias names (term and terms queries work with both physical and alias names). A known limitation?
"Skipped" counting logic in results is a bit of a mystery - always looks to be one less than what I would expect to see. The 70_skip_shards yaml test index needed 2 shards before it would count just one as skipped. What is the expected behaviour?

markharwood · 2019-11-13T14:12:18Z

test this please

jimczi

I left more comments.

Prefix and wildcard queries on "_index" field work with physical index names but not alias names (term and terms queries work with both physical and alias names). A known limitation?

It should work the same. I added a comment on the prefix query rewrite, maybe that's the reason you see it not working ?

"Skipped" counting logic in results is a bit of a mystery - always looks to be one less than what I would expect to see. The 70_skip_shards yaml test index needed 2 shards before it would count just one as skipped. What is the expected behaviour?

When all shards are skipped we use one to produce an empty result. The logic is explained here:

elasticsearch/server/src/main/java/org/elasticsearch/action/search/CanMatchPreFilterSearchPhase.java

Line 85 in 4e6756f

    
           // this is a special case where we have no hit but we need to get at least one search response in order

I guess we could avoid doing this if there are no aggregations but that's out of scope for this pr.

jimczi · 2019-11-13T15:08:40Z

server/src/main/java/org/elasticsearch/index/query/PrefixQueryBuilder.java

Should it be:

Suggested change

if (shardContext != null && shardContext.index().getName().startsWith(value) == false) {

if (shardContext != null && shardContext.indexMatches(value + "*") == false) {

?

jimczi · 2019-11-13T15:14:35Z

server/src/main/java/org/elasticsearch/index/query/TermQueryBuilder.java

BytesRefs.toString is not needed ?

Suggested change

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

if (shardContext != null && shardContext.indexMatches(value) == false) {

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

The prefix change suggestion looks to have fixed things, thanks

jpountz

I left two minor comments but it looks good to me otherwise.

jpountz · 2019-11-14T13:17:28Z

...i-cluster-search/src/test/resources/rest-api-spec/test/multi_cluster/90_index_name_query.yml

this parameter is supposed to be for bw compat of the response format only, can you use track_total_hits=true instead?

jpountz · 2019-11-14T13:20:45Z

test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java

any reason not to reuse Regex.simpleMatch?

markharwood · 2019-11-14T15:07:01Z

test this please

jimczi

LGTM too

…n index that doesn’t match the query expression. Part of the “canMatch” phase optimisations. Closes elastic#48473

* rest_total_hits_as_int -> track_total_hits * Use Regex.simpleMatch

markharwood · 2019-11-15T11:10:43Z

test this please

…x" field (elastic#48681) 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 elastic#48473

markharwood added the :Search/Search Search-related issues that do not fall into other categories label Oct 30, 2019

markharwood added >enhancement v8.0.0 labels Oct 30, 2019

jimczi requested changes Oct 30, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/index/query/TermQueryBuilder.java Outdated Show resolved Hide resolved

markharwood force-pushed the fix/48473 branch 5 times, most recently from d7487dc to b8338e2 Compare November 1, 2019 09:17

jpountz reviewed Nov 2, 2019

View reviewed changes

markharwood force-pushed the fix/48473 branch from b8338e2 to 64814e9 Compare November 12, 2019 09:13

markharwood force-pushed the fix/48473 branch from f8f37d5 to dde93fd Compare November 13, 2019 15:08

jimczi requested changes Nov 13, 2019

View reviewed changes

markharwood force-pushed the fix/48473 branch from 1b3e0b3 to 9fe82da Compare November 14, 2019 10:52

jpountz suggested changes Nov 14, 2019

View reviewed changes

markharwood force-pushed the fix/48473 branch 2 times, most recently from 961344f to e21a561 Compare November 14, 2019 14:50

jpountz approved these changes Nov 14, 2019

View reviewed changes

jimczi approved these changes Nov 14, 2019

View reviewed changes

markharwood added 4 commits November 15, 2019 09:23

Make queries on the “_index” field fast-fail if the target shard is a…

090a672

…n index that doesn’t match the query expression. Part of the “canMatch” phase optimisations. Closes elastic#48473

Addressing review comments - better test support for prefix and wildcard

6d86cc3

Updated comments with explanation of “skipped” result

8e42ac4

Address jpountz review comments:

6ac08b1

* rest_total_hits_as_int -> track_total_hits * Use Regex.simpleMatch

markharwood force-pushed the fix/48473 branch from 5a79628 to 6ac08b1 Compare November 15, 2019 09:23

markharwood merged commit 82c00f0 into elastic:master Nov 15, 2019

markharwood added the backport pending label Nov 15, 2019

jimczi added the v7.6.0 label Nov 15, 2019

markharwood mentioned this pull request Nov 15, 2019

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

Merged

markharwood removed the backport pending label Nov 15, 2019

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

mdaudali added a commit to mdaudali/elasticsearch that referenced this pull request Oct 27, 2021

feat: cherry pick from elastic#48681

a4d9ce4

mdaudali mentioned this pull request Oct 28, 2021

[6.8] Backport: Search optimisation - add canMatch early aborts for queries on "_index" field #80005

Closed

	if (shardContext != null && shardContext.index().getName().startsWith(value) == false) {
	if (shardContext != null && shardContext.indexMatches(value + "*") == false) {

Conversation

markharwood commented Oct 30, 2019

Uh oh!

elasticmachine commented Oct 30, 2019

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markharwood commented Nov 13, 2019

Uh oh!

markharwood commented Nov 13, 2019

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markharwood commented Nov 14, 2019

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

markharwood commented Nov 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants