Query rewrite context and search execution context refactoring #96353

salvatore-campagna · 2023-05-25T16:20:15Z

The idea is to cleanup the code in order to make sure we have a better structure around the way we use QueryRewriteContext and SearchExecutionContext. Both of them are used by the rewrite logic to simplify queries and improve query execution latency and resource usage.

We would like to enable the following three scenarios:

Rewrite happening on the coordinator which requires the QueryRewriteContext and a few more information but not requiring a SearchExecutionContext. This is the case for rewrite operations which happen entirely on the coordinator node and spare us from executing the query on the data node.
Rewrite happening on the data node which requires the QueryRewriteContext and some information available in the SearchExecutionContext. In some scenarios an IndexSearcher is not required. Not using it saves us a few unnecessary IO operations and allows us to complete query execution before we even need to read the index.
Rewrite happening on the data node which requires the QueryRewriteContext and the full SearchExecutionContext. In this scenario an IndexSearcher is required for the rewrite logic to be fully exploited.

For this reason we will refactor the SearchExecutionContext pulling up into the QueryRewriteContext a few fields/methods (like getFieldType and getIndexSettings and so on). Moreover, we will need to revisit the implementation of doRewrite for subclasses of AbstractQueryBuilder in such a way to enable the three rewrite logic branches described above.

Resolves #96280

elasticsearchmachine · 2023-05-25T16:21:30Z

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

salvatore-campagna · 2023-05-25T18:31:26Z

@elasticsearchmachine run elasticsearch-ci/full-bwc

javanna

left a small comment, LGTM otherwise

javanna · 2023-05-25T18:50:23Z

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

-                    queryRewriteContext
-                );
+        final MappedFieldType fieldType = queryRewriteContext.getFieldType(fieldName);
+        if (queryRewriteContext instanceof final CoordinatorRewriteContext coordinatorRewriteContext


maybe the convertToCoordinatorRewriteContext was better in that it let us avoid this instanceof check?

Yes I was considering a few more small changes before asking for a review...I wanted to run the full CI on it including full bwc (even if I don't expect bwc issues). Thanks.

I also need to go through the JavaDoc to make sure I update everything to reflect the refactoring.

thanks! ping me for review when ready ;)

salvatore-campagna · 2023-05-26T10:14:40Z

@elasticsearchmachine run elasticsearch-ci/full-bwc

salvatore-campagna · 2023-05-26T10:19:51Z

As it is now it is possible in the QueryBuilder to call both convertToCoordinatorRewriteContext and convertToSearhExecutionContext in a way similar to what we do in RangeQueryBuilder#doRewrite and RangeQueryBuilder#getRelation. This way we can enable rewriting of all queries on the coordinator node and on the data node (after updating PR #96161). We will still need to actually do the rewrite in the SearchService.

salvatore-campagna · 2023-05-26T10:41:15Z

I was wondering I can move the logic to call the two rewrite (coordinator and data node rewrite) up in to the AbstractQueryBuilder and then have subclasses override two methods...doCoordinatorRewrite and doSearchRewrite.

romseygeek

Thanks, this looks great. I like the idea of explicit coordinatorRewrite and searchRewrite methods as well. Maybe a canMatchRewrite too?

romseygeek · 2023-05-26T11:42:00Z

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

+        }
+    }
+
+    public void setAllowUnmappedFields(boolean allowUnmappedFields) {


I wonder if we can remove these two by having a special PercolatorRewriteContext that wraps an incoming rewrite context and overrides failIfFieldMappingNotFound()? For a follow-up maybe.

Need to understand how to deal with SearchExecutionContext#reset calling setAllowUnmappedFields...maybe we can just get rid of reset() by having a specific PercolatorExecutionContext?

To do in a followup PR.

this method (setAllowUnmappedFields) has been bugging me for a while :) sounds good to tackle in a follow-up

martijnvg

LGTM, looks like a clean move of a number of methods and fields to QueryRewriteContext.

martijnvg · 2023-05-26T12:14:08Z

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


-    public QueryRewriteContext(XContentParserConfiguration parserConfiguration, Client client, LongSupplier nowInMillis) {
+    public QueryRewriteContext(


Is the idea to create a new instance of this class with mapper service etc. in #96161?

The idea here is to just move stuff and later on for the other PR just:

remove unnecessary things that we have here as a result of moving fields into the QueryRewriteContext

include the rewrite in the SearchService so to actually trigger the rewrite in the data node

eventually change the code for at least MatchPhaseQueryBuilder to actually implement the shard skipping logic for the query used by Observability integrations.

Not sure why we need to create a new class here.

Ideally here I would like to have everything excluding what is strictly needed for skipping shards. I intend a refactor as "change structure without changing behavior". Ideally this means green tests -> refactor -> green tests (even if I will probably include a couple of tests to test the two new methods doCoordinatorRewrite and doSearchRewrite.

The main difference will be in creating a QueryRewriteContext where in the other PR we create a SearchExecutionContext. This way we will not need to set the IndexSearcher to null, which was one of the issues there.

javanna

LGTM it's great to see this cleanup. Thanks!

salvatore-campagna · 2023-05-29T07:03:33Z

@elasticsearchmachine run elasticsearch-ci/full-bwc

salvatore-campagna · 2023-05-29T09:43:32Z

@javanna @romseygeek do you see any reason why full bwc test (specifically against version 8.8.0) fail? I did that "just to be sure" without expecting any of them to fail...the test fails because of a timeout waiting for the cluster to to go from yellow to green...

…m SearchExecutionContext

salvatore-campagna · 2023-05-30T13:38:02Z

@elasticsearchmachine test this please

salvatore-campagna · 2023-05-30T15:20:22Z

So it looks like the PercolatorExecutionContext refactoring breaks BWC. I didn't get where is the point where serialization happens. I discussed with Martijn and I understand seriealization kicks in when we store the percolator query. I will revert the Percolator changes and leave them out of this PR. We can take care of it in another PR.

This reverts commit 4d079da.

This reverts commit 63c85df.

…tionContext" This reverts commit 209ef04.

This reverts commit e9f7770.

This reverts commit 65dda6b.

This reverts commit acb0a29.

This reverts commit f825743.

This reverts commit 1068b4b.

…ring from SearchExecutionContext" This reverts commit cca67ac.

salvatore-campagna · 2023-05-30T16:17:06Z

@elasticsearchmachine run elasticsearch-ci/part-1 please

javanna

left a couple of questions, LGTM otherwise.

javanna · 2023-05-31T07:56:31Z

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

@@ -286,6 +286,36 @@ public final QueryBuilder rewrite(QueryRewriteContext queryRewriteContext) throw
    }

    protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException {
+        if (queryRewriteContext == null) {


when can this be null?

There is at least case when this can be null and it is uncovered by a test that deals with fuzzy queries.

./gradlew ':rest-api-spec:yamlRestTest' --tests "org.elasticsearch.test.rest.ClientYamlTestSuiteIT.test {yaml=search/320_disallow_queries/Test disallow expensive queries}" -Dtests.seed=C6EE7229D969ECFD -Dtests.locale=es-SV -Dtests.timezone=Asia/Novosibirsk -Druntime.java=20

I will try to debug it and see if I can understand why it is null in such a scenario.

Also when we do convertToSearchExecutionContext the default implementation of this method returns null. Probably if a rewrite is tried later, after invoking another rewrite somewhere else which returns null (because of no override) we might end up in a situation where this is null.

Unfortunately the dafult implementation of convertToSearchExecutionContext cannot return this because a SearchExecutionContext is expected instead of a QueryRewriteContext as a return value...so we need to return null there.

ok, maybe add a comment on why this is needed? Seems like it's a question we may ask ourselves again soon :)

javanna · 2023-05-31T07:58:09Z

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

+        }
+    }
+
+    public void setAllowUnmappedFields(boolean allowUnmappedFields) {


this method (setAllowUnmappedFields) has been bugging me for a while :) sounds good to tackle in a follow-up

javanna · 2023-05-31T08:00:55Z

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

-                );
-            }
-        }
+    // Overridable for testing onl


missing y at the end of only

javanna · 2023-05-31T08:11:18Z

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

 import java.util.function.BiConsumer;
 import java.util.function.LongSupplier;
+import java.util.function.Predicate;

 /**
 * Context object used to rewrite {@link QueryBuilder} instances into simplified version.
 */


It would be great (as a followup) to clarify javadocs for these 3 contexts. e.g. what's the difference between coordinator rewrite context and query rewrite context if both may happen on the coordinating node?

salvatore-campagna · 2023-05-31T15:34:30Z

@elasticsearchmachine run elasticsearch-ci/part-1 please

martijnvg

LGTM

salvatore-campagna · 2023-06-01T08:39:32Z

@elasticsearchmachine run elasticsearch-ci/part-1 please

salvatore-campagna · 2023-06-01T10:14:15Z

@elasticsearchmachine update branch

salvatore-campagna · 2023-06-01T10:14:57Z

@elasticsearchmachine run elasticsearch-ci/part-1 please

salvatore-campagna added 2 commits May 24, 2023 19:07

fix: push some fields and methods into QUeryRewriteContext

911040c

refactor: remove convertToCoordinatorRewriteContext

a4beed1

salvatore-campagna self-assigned this May 25, 2023

salvatore-campagna added :Search/Search Search-related issues that do not fall into other categories >refactoring v8.9.0 test-full-bwc Trigger full BWC version matrix tests labels May 25, 2023

elasticsearchmachine added the Team:Search Meta label for search team label May 25, 2023

Merge branch 'main' into refactor/96280-search-execution-context

4ae325c

javanna approved these changes May 25, 2023

View reviewed changes

salvatore-campagna added 3 commits May 26, 2023 09:55

fix: include convertToCoordinatorRewriteContext to avoid instanceof

1854aba

fix: move a couple more methods in QueryRewriteContext

75d90a5

docs: fix a couple of comments

bf10980

salvatore-campagna requested review from javanna, romseygeek and martijnvg May 26, 2023 10:16

romseygeek approved these changes May 26, 2023

View reviewed changes

martijnvg approved these changes May 26, 2023

View reviewed changes

javanna approved these changes May 26, 2023

View reviewed changes

fix: move rewrite logic up in AbstractQueryBuilder

91441b5

Merge branch 'main' into refactor/96280-search-execution-context

d2dd2e8

salvatore-campagna added 2 commits May 29, 2023 15:16

refactor: remove allowUnmappedFields and mapUnmappedFieldAsString fro…

cca67ac

…m SearchExecutionContext

fix: make member variables private

1068b4b

salvatore-campagna removed the test-full-bwc Trigger full BWC version matrix tests label May 30, 2023

fix: simplify getMappedFieldType

4d079da

salvatore-campagna added 10 commits May 30, 2023 17:41

Revert "fix: simplify getMappedFieldType"

a07baaf

This reverts commit 4d079da.

Revert "refactor: move PercolatorExecutionContext to percolator module"

e3e236b

This reverts commit 63c85df.

Revert "fix: wrap a PercolatorExecutionContext instead of SearchExecu…

17ddf8a

…tionContext" This reverts commit 209ef04.

Revert "fix: move configuration of context after early exit"

29c9f2d

This reverts commit e9f7770.

Revert "fix: first create the context and then wrap it"

7df4b13

This reverts commit 65dda6b.

Revert "fix: null MappingLookup"

3d1ccf2

This reverts commit acb0a29.

Revert "fix: prevent possible NPE"

101c18c

This reverts commit f825743.

Revert "fix: make member variables private"

3cd4995

This reverts commit 1068b4b.

Revert "refactor: remove allowUnmappedFields and mapUnmappedFieldAsSt…

472dbc0

…ring from SearchExecutionContext" This reverts commit cca67ac.

fix: use MappingLookup.EMPTY instead of null

f00c6be

salvatore-campagna mentioned this pull request May 31, 2023

Create a percolator-specific PercolatorExecutionContext #96447

Open

javanna approved these changes May 31, 2023

View reviewed changes

javanna reviewed May 31, 2023

View reviewed changes

refactor: pull some more members up into QueryRewriteContext

989f87f

martijnvg approved these changes Jun 1, 2023

View reviewed changes

docs: add a note explaining null check

3917eec

salvatore-campagna merged commit 8b2ac4e into elastic:main Jun 1, 2023

This was referenced Jun 2, 2023

[CI] TimeSeriesAggregationsIT testStandAloneTimeSeriesAggWithMetricFilter failing #96501

Closed

[CI] TimeSeriesAggregationsIT testRetrievingHits failing #96487

Closed

salvatore-campagna mentioned this pull request Jun 5, 2023

Skip shards when querying constant keyword fields #96161

Merged


		public QueryRewriteContext(XContentParserConfiguration parserConfiguration, Client client, LongSupplier nowInMillis) {
		public QueryRewriteContext(

Query rewrite context and search execution context refactoring #96353

Query rewrite context and search execution context refactoring #96353

Conversation

salvatore-campagna commented May 25, 2023 • edited Loading

elasticsearchmachine commented May 25, 2023

salvatore-campagna commented May 25, 2023

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna commented May 26, 2023

salvatore-campagna commented May 26, 2023 • edited Loading

salvatore-campagna commented May 26, 2023

romseygeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

salvatore-campagna commented May 29, 2023

salvatore-campagna commented May 29, 2023

salvatore-campagna commented May 30, 2023

salvatore-campagna commented May 30, 2023 • edited Loading

salvatore-campagna commented May 30, 2023

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvatore-campagna commented May 31, 2023

martijnvg left a comment

Choose a reason for hiding this comment

salvatore-campagna commented Jun 1, 2023

salvatore-campagna commented Jun 1, 2023

salvatore-campagna commented Jun 1, 2023

salvatore-campagna commented May 25, 2023 •

edited

Loading

salvatore-campagna commented May 26, 2023 •

edited

Loading

salvatore-campagna May 26, 2023 •

edited

Loading

salvatore-campagna commented May 30, 2023 •

edited

Loading

salvatore-campagna May 31, 2023 •

edited

Loading