Use SearchRequest copy constructor in ExpandSearchPhase #36772
Conversation
javanna
added
>enhancement
:Search/Search
|
Pinging @elastic/es-search |
| @@ -91,7 +91,8 @@ public void run() throws IOException { | |||
| SearchSourceBuilder sourceBuilder = buildExpandSearchSourceBuilder(innerHitBuilder, innerCollapseBuilder) | |||
| .query(groupQuery) | |||
| .postFilter(searchRequest.source().postFilter()); | |||
| SearchRequest groupRequest = buildExpandSearchRequest(searchRequest, sourceBuilder); | |||
| SearchRequest groupRequest = new SearchRequest(searchRequest); | |||
| groupRequest.source(sourceBuilder); | |||
There was a problem hiding this comment.
Looking at https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java#L112 I think this extra line can be removed, or am I missing sth?
There was a problem hiding this comment.
if we remove it the copy constructor would not be a correct copy constructor, meaning it would never copy the source? I think it's fine that it copies it and we override it when needed? WDYT?
There was a problem hiding this comment.
Oh, I wasn't suggesting removing it in the copy constructor. I was missing the part that sourceBuilder here seems not be part of searchRequest yet when calling the copy constructor. All good now, thanks for the clarification.
Using the copy constructor introduced with #36641 guarantees that whenever new fields are added to
SearchRequestthey are properly propagated to its copies. Copies created inExpandSearchPhasewere missing a couple of recently added fields.