Query refactoring: OrQueryBuilder and Parser #11817
Conversation
Moving the query building functionality from the parser to the builders new toQuery() method analogous to other recent query refactorings. Relates to elastic#10217
|
The changes include minor updates to AndQueryBuilder as well that I just discoverd. In fact, the two queries are so closely related that I was thinking about factoring out a common parent class, but since they are both deprecated and there are just two cases that could be grouped under that parent class I didn't do that just yet. Open to suggestions on this part though. |
|
|
||
| BooleanQuery query = new BooleanQuery(); | ||
| for (QueryBuilder f : filters) { | ||
| query.add(f.toQuery(parseContext), Occur.SHOULD); |
There was a problem hiding this comment.
what happens when toQuery returns null here?
There was a problem hiding this comment.
Good catch, just copied that part of the code but know we have this extra step. I think if can return null. This is anoying, maybe at some point we can revisit this and see if instead of passing up null up through the whole query-tree we can have some sort of No-Op Query Builder and maybe an equivalent as a lucene query. Just a thought, will just catch this here.
There was a problem hiding this comment.
but what did we do before when parse returned null? should be the same? maybe it is simply allowed to pass in null here if we used to do it already?
There was a problem hiding this comment.
That case is already handled in the parser, I think. Will double check, also in AndQB.
There was a problem hiding this comment.
Can confirm this, if in the parser innerParser returns null, it's not added to the list of QueryBuilders we use. That logic is already in the parser code.
|
LGTM besides the small comment I left. Regarding similarities with and query builder, I think I would keep everything as-is, otherwise we would complicate the class hierarchy even more which is probably not worth it for deprecated classes |
|
LGTM |
Query refactoring: OrQueryBuilder and Parser
…ring-or Query refactoring: OrQueryBuilder and Parser
clintongormley
added
:Search/Search
Moving the query building functionality from the parser to the builders
new toQuery() method analogous to other recent query refactorings.
Relates to #10217
PR goes against the query-refactoring branch