Separating QueryParseContext and QueryShardContext#12527
Conversation
There was a problem hiding this comment.
I think the exception should still be QueryParsingException here
|
I did another round, I think this is getting very close. The only issue I have is the fact that you can retrieve the QueryParseContext from the QueryShardContext. This confuses me, I find it conceptually wrong as you are in parse phase and might need to move on, hence create a QueryShardContext given a QueryParseContext. The other way around just because the latter is a lighter context compared to the former isn't a goo engough reason to do things this way to be honest, unless I am missing other reasons why we moved to this approach. Let me know what you think. I would still have all the parse methods accept a QueryParseContext and remove getParseContext from QueryShardContext. |
|
@javanna addressed most of your last comment, I have trouble removing catching those exceptions you mentioned, in one case test fails, in other case I still don't see why this should happen. Also need to have a final sync on the direction in which the two contexts refer to each other for now. |
4f3b72c to
dd90dec
Compare
There was a problem hiding this comment.
can you add a //norelease here too? context should really go away after all queries are refactored
|
I did a final round, left a few minor comments, but this LGTM once those are addressed |
dd90dec to
9b218b3
Compare
|
@javanna did another rebase, will still need to do a final squash and work on the commit message if you think this should go into the branch now |
9b218b3 to
07226a4
Compare
We are currently splitting the parse() method in the query parsers into a query parsing part (fromXContent(QueryParseContext)) and a new method that generates the lucene query (toQuery(QueryParseContext)). At some point we would like to be able to excute these two steps in different phases, one on the coordinating node and one on the shards. This PR tries to anticipate this by renaming the existing QueryParseContext to QueryShardContext to make it clearer that this context object will provide the information needed to generate the lucene queries on the shard. A new QueryParseContext is introduced and all the functionality needed for parsing the XContent in the request is moved there (parser, helper methods for parsing inner queries). While the refactoring is not finished, the new QueryShardContext will expose the QueryParseContext via the parseContext() method. Also, for the time beeing the QueryParseContext will contain a link back to the QueryShardContext it wraps, which eventually will be deleted once the refactoring is done. Closes elastic#12527 Relates to elastic#10217
07226a4 to
904cbf5
Compare
…eryCreationContext Separating QueryParseContext and QueryShardContext
The parse() method we are currently splitting into a query parsing part (fromXContent()) and a lucene query generating part (toQuery()) in the feature refactoring branch (#10217) takes QueryParseContext as an argument. At some point we need to excute these two steps in different phases on the coordinating node and the shards. This protoype PR tries to anticipate this by introducing a new QueryCreationContext object as the argument of the query-generating methods (toQuery() and all dependent methods).
As a first step the existing QueryParseContext is renamed to QueryCreationContext, which itself acts as a wrapper around a new lightweight QueryParseContext that should only contain the functionality used during query parsing. In subsequent steps we can then try to separate the different behaviour needed in the two context objects.
This is WIP, the separation of the two context is not completely clean and local variables are not renames. Also the naming of the new context object is completely open. Would like to open this PR to get feedback and discuss.
PR goes against query refactoring branch.