Query refactoring: ConstantScoreQueryBuilder and Parser #11629
Conversation
|
|
||
| public static final String NAME = "constant_score"; | ||
|
|
||
| private final QueryBuilder filterBuilder; | ||
| private final QueryBuilder queryBuilder; |
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. |
|
|
||
| @Override | ||
| public Query toQuery(QueryParseContext parseContext) throws QueryParsingException, IOException { | ||
| Query innerFilter = queryBuilder.toQuery(parseContext); |
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.
|
|
||
| @Override | ||
| public Query toQuery(QueryParseContext parseContext) throws QueryParsingException, IOException { | ||
| Query innerFilter = filterBuilder.toQuery(parseContext); |
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 |
| * Test empty "filter" element. | ||
| * Current DSL allows inner filter element to be empty, returning a `null` inner filter builder | ||
| */ | ||
| public void testEmptyFilterElement() throws IOException { |
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
cbuescher
force-pushed
the
feature/query-refactoring-constantscore
branch
from
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
clintongormley
added
:Search/Search
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.