Query refactoring: ConstantScoreQueryBuilder and Parser#11629
Conversation
There was a problem hiding this comment.
I am not sure we want to rename this to queryBuilder. I do see that it is a query and not a filter anymore, but the query is used as a filter, which might be why we left it this way when we removed filters? @jpountz thoughts?
|
left a couple of comments, looks good though! |
|
Adressed the last two comments, not sure about the queryBuilder/filterBuilder naming thing. Happy to change that back since it's a filter as you said. |
There was a problem hiding this comment.
now you rely on validation right given that queryBuilder can potentially be null but not after we called validate?
There was a problem hiding this comment.
yes, I think the null-check for queryBuilder can be dropped because we should catch it eventually with the validation.
There was a problem hiding this comment.
I have second thoughts about removing the null checks here.... sorry! let's say a java api user creates a new query without a filter, then calls manually toQuery which makes no sense but it is a public method... what do we do? this case won't be covered by our validation framework. Shall we let it throw NPE or handle it gracefully?
There was a problem hiding this comment.
Yes, I re-added this. Also, since things like `{"constant_score" : { "filer" : {} }}" are currently valid in the DSL, I removed the vaidation check for filterBuilder == null.
|
Revisited null-checks for inner query builder again. It seems that current DSL allows inner empty queries like |
There was a problem hiding this comment.
missing @test just for consistency with existing tests?
|
LGTM |
Moving the query building functionality from the parser to the builders new toQuery() method analogous to other recent query refactorings. Relates to elastic#10217 Closes elastic#11629
ddbabdf to
1c0b7b3
Compare
|
Final squash and rebase, will merge this then as soon as I ran the tests again another time. |
…nstantscore Query refactoring: ConstantScoreQueryBuilder and Parser
Moving the query building functionality from the parser to the builders new toQuery() method analogous to other recent query refactorings. Relates to elastic#10217 Closes elastic#11629
…ring-constantscore Query refactoring: ConstantScoreQueryBuilder and Parser
Moving the query building functionality from the parser to the builders
new toQuery() method analogous to other recent query refactorings.
PR goes agains query-refactoring feature branch.