Add retriever to the query rewrite phase by jimczi · Pull Request #110482 · elastic/elasticsearch

jimczi · 2024-07-04T14:18:41Z

This change moves the extraction of retrievers into the search source builder during the coordinator rewrite phase.
By delaying the extraction, retrievers can apply asynchronous actions during the rewrite before executing the search request.

Additionally, a compound mode for retrievers is introduced, creating an internal point in time for the duration of the request. This allows retrievers to perform queries during the rewrite on the same reader that will be used for the main search request.
In a subsequent PR, this capability will be used to implement retrievers that combine multiple retrievers.

This PR consists of three commits. The last commit contains the main change to the search action, which involves creating and deleting a PIT before search execution if a compound retriever is present.

…side the search request

elasticsearchmachine · 2024-07-04T14:19:05Z

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

pmpailis · 2024-07-08T10:58:53Z

-                if (request.source() != null && request.source().pointInTimeBuilder() != null) {
+                if (request.source() != null
+                    && request.source().pointInTimeBuilder() != null
+                    && request.source().pointInTimeBuilder().singleSession() == false) {


Is it safe to rely on the keepAlive to determine explicit releasing in this case? Could this possible cause any side effects with any user provided requests with -1 as keep_alive ?

This is just about returning the pit id in the response. The logic to release the pit automatically is restricted to the retriever builder case. It might be surprising for existing pit users though since -1 is a valid keep-alive value. We should be able to restrict this behavior to retrievers entirely.

pmpailis · 2024-07-08T11:01:14Z

Thanks so much for this @jimczi ! Added some minor initial comments!

mayya-sharipova · 2024-07-08T17:55:25Z

+                    @Override
+                    public void onResponse(SearchResponse response) {
+                        String query = response.getHits().getAt(0).field(QUERY_FIELD).getValue();
+                        StandardRetrieverBuilder standard = new StandardRetrieverBuilder();


I assume in the main code we want retrievers to be rewritten into some kind of ScoreDocQuery, but not to another retriever?

Retrievers will always rewrite to a retriever, that's enforced by the design. Using some kind of ScoreDocQuery will be internally done by the rewritten retriever when extractToSourceBuilder is called (when the retriever is guaranteed to be fully rewritten).

mayya-sharipova

@jimczi Thanks Jim, makes sense

javanna

I left some nits and suggestions around splitting this change up and making it a bit easier to follow.

This change moves the handling of the retriever to the rewrite phase. It also adds validation of the search source builder after extracting the retriever into the source builder. Relates elastic#110482

This change moves the handling of the retriever to the rewrite phase. It also adds validation of the search source builder after extracting the retriever into the source builder. Relates #110482

…art1

pmpailis · 2024-07-15T10:38:39Z

Hey @javanna! I'm picking up this PR, so whenever you have some time I'd appreciate some feedback after the latest changes :)

benwtrent · 2024-07-17T14:52:13Z

@pmpailis I haven't forgotten about this. If you think its ready, I will go through a review. I will likely ask for more tests :)

elasticsearchmachine · 2024-07-17T19:25:17Z

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

pmpailis · 2024-07-23T08:19:13Z

Closing as a fix for this has been merged through #111153.

pmpailis · 2024-07-23T08:20:07Z

Please ignore the above! Wrong PR to close 😅

…etrievers

pmpailis · 2024-07-25T07:42:21Z

Another thing to consider is what if somebody has set "allow_partial_search_results". Doesn't the PIT opening fail on a missing shard, thus rendering that option useless?

Hey @benwtrent ; added a take on this in 29266a7.

The idea now is that if we specify a compound retriever (which would require opening a PIT) and explicitly define allow_partial_search_results , then we'd throw a validation error when parsing the search request. This is a bit harsh now, and we could be more lenient, as all shards might be present, but it makes it clear for the user that this option is not supported yet.

We haven't updated PIT's logic yet, so if any shards are missing we'd throw an exception, the message of which has also been slightly updated to not suggest to use allow_partial_search_results which is not respected yet by PIT (this could be a follow up work as discussed).

… enabled scrolling

pmpailis · 2024-07-25T11:32:36Z

@elasticmachine update branch

benwtrent · 2024-07-25T13:23:12Z

+                @Override
+                protected void doCheckNoMissingShards(
+                    String phaseName,
+                    SearchRequest request,
+                    GroupShardsIterator<SearchShardIterator> shardsIts
+                ) {
+                    final StringBuilder missingShards = new StringBuilder();
+                    // Fail-fast verification of all shards being available
+                    for (int index = 0; index < shardsIts.size(); index++) {
+                        final SearchShardIterator shardRoutings = shardsIts.get(index);
+                        if (shardRoutings.size() == 0) {
+                            if (missingShards.isEmpty() == false) {
+                                missingShards.append(", ");
+                            }
+                            missingShards.append(shardRoutings.shardId());
+                        }
+                    }
+                    if (missingShards.isEmpty() == false) {
+                        // Status red - shard is missing all copies and would produce partial results for an index search
+                        final String msg = "Cannot execute [open_point_in_time] action due to missing shards [" + missingShards + "].";
+                        throw new SearchPhaseExecutionException(phaseName, msg, null, ShardSearchFailure.EMPTY_ARRAY);
+                    }
+                }


woah, I am unsure about this. This is a fairly big change in behavior for PIT isn't it?

Tbh I think that the change is not that big since we're overriding the (previously static)SearchPhase#doCheckNoMissingShards with just a couple of changes:

we don't have the if (request.allowPartialSearchResults() == false) check anymore as this is explicitly set by the PIT action (in TransportOpenPointInTimeAction#doExecute)

we have a more specific error message, w/o suggesting the allow_partial_search_results as a solution anymore

The only reason for this addition was to update the error message, so if you think that this isn't needed right now (as this is something that we plan to revisit soon-ish) I can omit this from the PR.

@pmpailis I would prefer this change be in a separate, focused PR. This way foundations, et. al. can focus their review on it.

Opened #111320

benwtrent · 2024-07-29T13:15:28Z

    public ActionRequestValidationException validate() {
        ActionRequestValidationException validationException = null;
        boolean scroll = scroll() != null;
+        boolean allowPartialSearchResults = allowPartialSearchResults() != null && allowPartialSearchResults();


benwtrent

Thank you @pmpailis for going through all this.

Let's make sure we can support partial search results as soon as possible in the future :).

pmpailis · 2024-07-29T13:29:16Z

@elasticmachine update branch

pmpailis · 2024-07-30T07:17:51Z

@elasticmachine update branch

This change moves the handling of the retriever to the rewrite phase. It also adds validation of the search source builder after extracting the retriever into the source builder. Relates #110482

jimczi added 3 commits July 4, 2024 14:41

Add the search request PIT in the query rewrite context

7aa49d1

Consume retrievers during the query rewrite

83a0fe4

Add a compound mode in retrievers that give access to a shared pit in…

ae28c7b

…side the search request

jimczi added >enhancement :Search/Search Search-related issues that do not fall into other categories v8.16.0 labels Jul 4, 2024

jimczi requested review from jdconrad and pmpailis July 4, 2024 14:18

elasticsearchmachine added the Team:Search Meta label for search team label Jul 4, 2024

jimczi added 5 commits July 4, 2024 15:24

fix typo

aa09fb2

fix another typo in validation

f127e7b

fix retriever tests

165604d

fix parsing tests

c4d6750

release the search response in tests

3762bdd

pmpailis reviewed Jul 8, 2024

View reviewed changes

mayya-sharipova reviewed Jul 8, 2024

View reviewed changes

Comment thread server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

mayya-sharipova reviewed Jul 8, 2024

View reviewed changes

mayya-sharipova approved these changes Jul 8, 2024

View reviewed changes

javanna reviewed Jul 9, 2024

View reviewed changes

jimczi mentioned this pull request Jul 9, 2024

Move Retriever Handling to Rewrite Phase #110641

Merged

jimczi added 2 commits July 10, 2024 17:49

Merge remote-tracking branch 'upstream/main' into retriever_rewrite_p…

e8f00b7

…art1

address review comments

a370cac

benwtrent reviewed Jul 15, 2024

View reviewed changes

Comment thread server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

benwtrent self-requested a review July 17, 2024 14:51

javanna added :Search Relevance/Search Catch all for Search Relevance and removed :Search/Search Search-related issues that do not fall into other categories labels Jul 17, 2024

elasticsearchmachine added Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch and removed Team:Search Meta label for search team labels Jul 17, 2024

pmpailis closed this Jul 23, 2024

pmpailis reopened this Jul 23, 2024

ensuring that allow_search_partial_results is disabled for compound r…

29266a7

…etrievers

pmpailis added 2 commits July 25, 2024 13:40

addressing test failures in search request validation due to randomly…

a75e885

… enabled scrolling

updating error message in tests

cc6204a

Merge branch 'main' into retriever_rewrite_part1

c6682ec

benwtrent reviewed Jul 25, 2024

View reviewed changes

pmpailis mentioned this pull request Jul 26, 2024

Updating error message for missing shards in open_point_in_time action #111320

Merged

reverting updating error message for opening PIT

effb879

benwtrent reviewed Jul 29, 2024

View reviewed changes

benwtrent approved these changes Jul 29, 2024

View reviewed changes

fixing log message

1700df5

elasticmachine and others added 2 commits July 29, 2024 23:29

Merge branch 'main' into retriever_rewrite_part1

a52e530

fixing test after merging main

46689a3

Merge branch 'main' into retriever_rewrite_part1

910ce92

pmpailis merged commit 3420354 into elastic:main Jul 30, 2024

pmpailis mentioned this pull request Aug 1, 2024

Adding support for allow_partial_search_results in PIT #111516

Merged

Conversation

jimczi commented Jul 4, 2024

Uh oh!

elasticsearchmachine commented Jul 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmpailis commented Jul 8, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova left a comment

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pmpailis commented Jul 15, 2024

Uh oh!

Uh oh!

benwtrent commented Jul 17, 2024

Uh oh!

elasticsearchmachine commented Jul 17, 2024

Uh oh!

pmpailis commented Jul 23, 2024

Uh oh!

pmpailis commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmpailis commented Jul 25, 2024

Uh oh!

pmpailis commented Jul 25, 2024

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!

benwtrent left a comment

Choose a reason for hiding this comment

Uh oh!

pmpailis commented Jul 29, 2024

Uh oh!

pmpailis commented Jul 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pmpailis commented Jul 23, 2024 •

edited

Loading