Refactor SearchRequest to be parsed on the coordinating node #13859

colings86 · 2015-09-29T16:52:50Z

This change refactors the SearchRequest so it is parsed on the coordinating node and then sent as a serialised object to the shards. The SearchSourceBuilder encompasses the state of the bpdy of the search request (know in the code as source) and is embedded in the SearchRequest. On the shard the SearchSourceBuilder is used to populate the SearchContext where we used to parse the source JSON.

A few things to note:

The Java API has been changed to only accept builders in the SearchRequest, SearchRequestBuilder, SearchSourceBuilder and other similar classes. This means that all methods that accepted BytesReference, byte[], String, BytesArray etc. in the search API (and derivatives) should have been removed
Some variables inside the SearchSourceBuilder are stored as BytesReference object as their builder objects have not yet been refactored. The SearchSourceBuilder converts the incoming Builder into the BytesReference to serialise to the shards and the parsing of this JSON is still handled in SearchService.parseSource(). Over time these will be refactored so these builders can also be stored as objects in SearchSourceBuilder and serialised to the shards properly.

…a unit test

…urceBuilder to SearchSourceBuilder

…ourceBuilder

Lots of NOCOMMITS are in the code as lots of compile errors have been temporarily commented out

This commit removes all the opaque bytes for extra_source and template_source. Instead source and extra_source etc. are represented as SearchSourceBuilder which can in-place be modified and is updated with the content of the request parameters. Template Source is parsed and evaluated which in-turn replaces the actual source.

Not the neatest implementation in the world. Maybe we should consider changing the builders so it is a single builder for sort and a single builder for rescore instead of a list of builders for each?

…Builder

…refactoring # Conflicts: # core/src/test/java/org/elasticsearch/search/functionscore/DecayFunctionScoreIT.java # core/src/test/java/org/elasticsearch/search/functionscore/FunctionScoreFieldValueIT.java # core/src/test/java/org/elasticsearch/search/functionscore/FunctionScoreIT.java # core/src/test/java/org/elasticsearch/search/rescore/QueryRescorerIT.java

…sponses

s1monw · 2015-10-15T08:09:56Z

core/src/main/java/org/elasticsearch/action/count/CountRequest.java

    private String[] types = Strings.EMPTY_ARRAY;

-    private int terminateAfter = DEFAULT_TERMINATE_AFTER;
+    private SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();


any chance this can be final?

s1monw · 2015-10-15T08:20:24Z

@colings86 this LGTM but I wonder what happened to all the ParseElements like HighlighterParseElement since we used to have all the parsing in there I wonder if we still need id and separately why we don't reuse the code from them to parse the actual sources at some point. I might just miss something but I though we are parsing all that stuff now already or is that still opaque (which is ok!) I just wonder if we miss something here....

javanna · 2015-10-15T08:37:01Z

core/src/main/java/org/elasticsearch/action/search/SearchRequest.java

@@ -505,18 +361,15 @@ public void writeTo(StreamOutput out) throws IOException {
            out.writeBoolean(true);
            scroll.writeTo(out);
        }
-        out.writeBytesReference(source);
-        out.writeBytesReference(extraSource);
+        if (source == null) {


can the source ever be null? I think it shouldn't be optional in readFrom and writeTo

I got confused with the count request, I see that this can be null, but I still wonder if it should? When does it make sense for it to be null? If it wasn't we could drop a lot of null checks around that.

# Conflicts: # core/src/main/java/org/elasticsearch/search/SearchService.java

javanna · 2015-10-15T12:11:58Z

core/src/main/java/org/elasticsearch/action/search/SearchRequest.java

@@ -71,12 +59,8 @@
    @Nullable
    private String preference;

-    private BytesReference templateSource;
-    private Template template;
+    private SearchSourceBuilder source;


seems like we can make it final now?

oh no we can't, sorry!

javanna · 2015-10-15T12:36:42Z

core/src/main/java/org/elasticsearch/index/search/stats/SearchSlowLog.java

-                } catch (IOException e) {
-                    sb.append("source[_failed_to_convert_], ");
-                }
+            if (context.request().source() != null) {


the null check can be dropped maybe?

javanna · 2015-10-15T12:50:25Z

we decided with @colings86 to make the source builder not nullable in the SearchRequest, but to do it after we merge this PR to master. I just did a final round and this LGTM

colings86 and others added 30 commits September 23, 2015 10:44

new SearchSourceBuilder to use as the search request

a96f14e

implemented hashCode, equals and fromXContent in NewSearchSourceBuidler

5623dd2

cleanup of commented out code

e73052b

various fixes after a review

eed6ab7

Completed serialisation to/from son and transport protocol and added …

3c8c6c0

…a unit test

Renamed SearchSourceBuilder to OldSearchSourceBuilder and NewSearchSo…

9116c1c

…urceBuilder to SearchSourceBuilder

cleanup codebase to work with SearchSourceBuilder

e1759b1

clean up for methods in SearchRequestBuilder so it works with SearchS…

3cb3514

…ourceBuilder

fixes to tests

b4dd6a5

small parsing fix for fields

aa3d29d

muted failing test

41e6350

SearchRequest.source() now returns a SearchSourceBuilder.

ab01ec5

Lots of NOCOMMITS are in the code as lots of compile errors have been temporarily commented out

updates to fix some of the NOCOMMITs

b98f7cf

fix compile errors

ac8c172

remove index from QueryParseContext Constructor

083f28a

Parsing of rescuer and sort ByteReferences.

a53cf89

Not the neatest implementation in the world. Maybe we should consider changing the builders so it is a single builder for sort and a single builder for rescore instead of a list of builders for each?

Search service can now fully populate SearchContext from SearchSource…

d7429a6

…Builder

removed sysout

2e3cc50

unit test fixes

585ed07

fix to sort parsing in SearchService

52d9664

removed sysouts

394e551

test fixes

63da68e

more test fixes

85f4c39

fixes and debugging

2de450c

Add a late-parsing SearchSoruce wrapper for index warmers

95a7b6a

Impl SearchSource#hashCode|equals|toString

04864dc

more test fixes

7a209cc

update rest tests to expect default boost to be returned in warmer re…

4996b07

…sponses

s1monw reviewed Oct 15, 2015
View reviewed changes

javanna reviewed Oct 15, 2015
View reviewed changes

colings86 added 2 commits October 15, 2015 11:02

Merge branch 'master' into feature/search-request-refactoring

c618f75

# Conflicts: # core/src/main/java/org/elasticsearch/search/SearchService.java

review comment fixes

63c51b7

javanna reviewed Oct 15, 2015
View reviewed changes

javanna added 2 commits October 15, 2015 14:28

Revert changes made to IdsQueryBuilder, fixed upstream meanwhile

8a87448

Revert changes made to IndexQueryParserService, fixed upstream meanwhile

0cab4a2

javanna reviewed Oct 15, 2015
View reviewed changes

reverted non-null searchSourceBuilder in searchRequest for now

ef5fc72

Merge branch 'master' into feature/search-request-refactoring

cf3e92a

colings86 merged commit cf3e92a into master Oct 15, 2015

This was referenced Oct 15, 2015

Inconsistent usage pattern of SearchSourceBuilder #6371

Closed

Java api: revise SearchRequest and SearchRequestBuilder object structure #14190

Closed

danielmitterdorfer deleted the feature/search-request-refactoring branch January 18, 2016 07:50

clintongormley added >breaking-java and removed >breaking labels Jul 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor SearchRequest to be parsed on the coordinating node #13859

Refactor SearchRequest to be parsed on the coordinating node #13859

Uh oh!

colings86 commented Sep 29, 2015

Uh oh!

s1monw Oct 15, 2015

Uh oh!

s1monw commented Oct 15, 2015

Uh oh!

javanna Oct 15, 2015

Uh oh!

javanna Oct 15, 2015

Uh oh!

javanna Oct 15, 2015

Uh oh!

javanna Oct 15, 2015

Uh oh!

javanna Oct 15, 2015

Uh oh!

javanna commented Oct 15, 2015

Uh oh!

Uh oh!

Refactor SearchRequest to be parsed on the coordinating node #13859

Refactor SearchRequest to be parsed on the coordinating node #13859

Uh oh!

Conversation

colings86 commented Sep 29, 2015

Uh oh!

s1monw Oct 15, 2015

Choose a reason for hiding this comment

Uh oh!

s1monw commented Oct 15, 2015

Uh oh!

javanna Oct 15, 2015

Choose a reason for hiding this comment

Uh oh!

javanna Oct 15, 2015

Choose a reason for hiding this comment

Uh oh!

javanna Oct 15, 2015

Choose a reason for hiding this comment

Uh oh!

javanna Oct 15, 2015

Choose a reason for hiding this comment

Uh oh!

javanna Oct 15, 2015

Choose a reason for hiding this comment

Uh oh!

javanna commented Oct 15, 2015

Uh oh!

Uh oh!