Update MatchPhrase- and TermQueryBuilder to be able to rewrite without a SearchExecutionContext. #96905
Conversation
… rewrite without a SearchExecutionContext. With this change, both query builders can rewrite without using a search context, because QueryRewriteContext often has all the mapping and other index metadata available. The `TermQueryBuilder` can with this resolve to a `MatchAllQueryBuilder` with needing a `SearchExecutionContext`, which during the can_match phase means that no searcher needs to be acquired and therefor avoiding making a shard search active / potentially refresh. The `AbstractQueryBuilder#doRewrite(...)` method is altered to by default attempt a coordination rewrite, then fall back to attempt a search rewrite, then finally fall back to do an index metadata aware rewrite. This was forgotten as part of elastic#96161 and is needed to complete elastic#95776.
martijnvg
changed the title
…pdated to remove its dependency on SearchExecutionContext
|
(labelling as non-issue, because this should have been added as part of #96161 that already has a changelog entry) |
martijnvg
added
the
:Search/Search
|
@elasticmachine run elasticsearch-ci/part-1 (Unrelated test failure: https://gradle-enterprise.elastic.co/s/nj6lilqwi46xe/tests/:server:test/org.elasticsearch.search.internal.ShardSearchRequestTests/testForceSyntheticUnsupported?top-execution=1) |
|
Pinging @elastic/es-search (Team:Search) |
| @@ -299,6 +299,10 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws | |||
| if (sec != null) { | |||
| return doSearchRewrite(sec); | |||
| } | |||
| final QueryRewriteContext context = queryRewriteContext.convertToMappingMetadataAwareContext(); | |||
There was a problem hiding this comment.
I am wondering if this if statement should go after convertToCoordinatorRewriteContext but before convertToSearchExecutionContext. Rewriting without an index searcher happens before we have a SearhcExecutionContext...
There was a problem hiding this comment.
I picked this order because it was suggested this way in this comment.
Also the real order depends on which what kind of context the doRewrite() method is called?
There was a problem hiding this comment.
Also only one kind of rewrite will be performed based on the context. In the case a SearchExecutionContext is provided then we should prefer performing a search based rewrite, because often it is more accurately than a query context rewrite. Also we already paid the price of create a search execution context (opening searches, potential refresh etc.)
A SearchExectionContext can also do a metadata rewrite, but I think we should prefer the search rewrite?
| * Returns the index settings for this context. This might return null if the | ||
| * context has not index scope. | ||
| */ | ||
| public IndexSettings getIndexSettings() { |
There was a problem hiding this comment.
I think I already extracted these changes in my refactoring...maybe the PR just needs update.
There was a problem hiding this comment.
I think your change moved the indexSettings field to QueryRewriteContext but not the getter?
This is why I needed to add the getter, so that query builders can access the index settings using query rewrite context.
| TermQueryBuilder tqb = (TermQueryBuilder) rewritten; | ||
| assertEquals(KEYWORD_FIELD_NAME, tqb.fieldName); | ||
| assertEquals(new BytesRef("value"), tqb.value); | ||
| for (QueryRewriteContext context : new QueryRewriteContext[] { createSearchExecutionContext(), createQueryRewriteContext() }) { |
There was a problem hiding this comment.
If I understand correctly here we test if the rewrite happens no matter when the actual rewrite is called. This is to make sure that we do the rewrite as soon as possible and in the worst case scenario we do it as before, rewriting with a SearchExecutionContext including an IndexSearcher. In our K8s tests this rewrite will happen before anyway due to the new logic, but if for some reason (for instance condition on shard pre-filtering is not met) then we will at least rewrite it later.
There was a problem hiding this comment.
Right, this is to ensure that both rewrite with search execution context and just a rewrite context are able to rewrite these queries correctly. The k8s o11y case we can skip a lot of shards, because the rewrite query context rewrite was already able to rewrite to a match no docs query.
|
Thanks, LGTM. |
With this change, both query builders can rewrite without using a search context, because QueryRewriteContext often has all the mapping and other index metadata available.
The
TermQueryBuildercan with this change resolve to aMatchNoneQueryBuilderwithout needing aSearchExecutionContext, which during the can_match phase means that no searcher needs to be acquired and therefor avoid making a shard search active and doing a potentially refresh.The
AbstractQueryBuilder#doRewrite(...)method is altered to by default attempt a coordination rewrite, then fall back to attempt a search rewrite, then finally fall back to do an index metadata aware rewrite. This is inline with what was discussed here: #96161 (comment)This change was forgotten as part of #96161 and is needed to complete #95776.