Query refactoring: Introduce toQuery() and fromXContent() methods in QueryBuilders and QueryParsers#10580
Conversation
…yBuilders and QueryParser
There was a problem hiding this comment.
I think a regular UnsupportedOperationException would work better instead of the Apache one
|
Left a minor comment but this looks pretty good to me, @javanna what do you think? |
…ilder, one for checking caching. Delegate those two from old method for bwc.
|
Working on BoolQueryBuilder based in this I realized that I also needed to split QueryParseContext#parseInnerQuery() into two parts, will be used to build the inner QueryBuilder in the parse-step and the second to later do some cache checking that is in place there at the moment. This part will only be called once the lucene queries are created. |
There was a problem hiding this comment.
the above two methods don't need to be repeated here as long as this abstract class implements QueryParser
|
I really like this change, it allows us to migrate queries gradually without changing everything at the same time. The only concern I have is whether we should merge this into master or start a new feature branch for the query refactoring. I do know that we are afraid of merge conflicts here, but I am also afraid that the solution is not merging temporary code. I am not doubting the quality of this PR, it's just that part of it will go away at the end of different steps of the refactoring and we don't know yet how long it will take to get it done. That said I have the feeling we want to work on this on a feature branch and pay the price of merge conflicts. Should get easier already though with this PR as we don't merge parser and builder anymore. |
…QueryParser class to move refactored queries to
|
Changes according to your comments. Main thing I did is pulling the "toQuery()" logic to the BaseQueryBuilder and letting the different builders provide the name of their corresponding parsers by a final method. Open to suggestions on this one though. |
There was a problem hiding this comment.
Not sure if I understand that. We will have to override that one in the QueryBuilders that we refactor, or am I wrong?
There was a problem hiding this comment.
Right I got confused with the parser, sorry! Shall we introduce these new methods through a new intermediate base class (BaseQueryBuilderTemp?) instead and have a new one for the migrated builders too? maybe overkill... what do you think?
There was a problem hiding this comment.
I think in this case it's not so nice since the existing BaseQueryBuilder already contains some implementations (toString, buildAsBytes etc...) that otherwise we just have to move over. I think to check how many extending classes don't have their own refactored "toQuery()" method it is sufficient to temporarily remove it from the base class and see what turns red. At least that works for me.
|
I like it, left a few more comments. |
…ate methods final
The currently planed refactoring of search queries layed out in #9901 and currently tracked in #10217 requires the current "parse()" methods into two methods, first a "fromXContent(...)" method that allows parsing to an intermediate query representation and second a "Query toQuery(...)" method these parsed query objects that create the actual lucene queries.
This PR is a first step in that direction as it introduces the interface changes necessary for the further refactoring. It introduces the new interface methods while for now keeping the old Builder/Parser API still in place by delegating the new "toQuery()" implementations to the existing "parse()" methods, and by introducing a "catch-all" "fromXContent()" implementation in a BaseQueryParser that returns a temporary QueryBuilder wrapper implementation. This allows us to refactor the existing QueryBuilders step by step while already beeing able to start refactoring queries with nested inner queries.