Query refactoring: BoostingQueryBuilder and Parser #11621
Conversation
cbuescher
force-pushed
the
feature/query-refactoring-boostingquery
branch
2 times, most recently
from
b73dc26 to
55998ac
Compare
| if (negativeBoost == -1) { | ||
| throw new IllegalArgumentException("boosting query requires negativeBoost to be set"); | ||
| if (negativeBoost < 0) { | ||
| throw new IllegalArgumentException("boosting query requires negativeBoost to be set to positive value"); |
There was a problem hiding this comment.
not sure, I think this error should only be returned by the validate method. Here we could print out whatever we have. The fact that we print out a query doesn't mean that it's validated? That said we might have relied on validation for previous queries in doXContent, which can be a problem... thoughts?
|
left a couple of minor comments |
cbuescher
force-pushed
the
feature/query-refactoring-boostingquery
branch
from
55998ac to
34bdac7
Compare
|
Just posted an update reverting the changes in the constructor and adressing other comments. |
| @@ -81,25 +126,87 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep | |||
| if (negativeQuery == null) { | |||
There was a problem hiding this comment.
shall we have these same checks in the validate method too? Or only there?
There was a problem hiding this comment.
I had them in validate() before, but the problem like in #11629 is that we need to support inner clauses that parse to null like "positive": { "constant_score": { "filter": {}}}, so if we have these checks in validate() we will throw errors on queries the DSL currently accepts.
I wonder why this behaviour was introduced in the first place, maybe there is a deeper reason for that, otherwise we should try and get rid of this but I think that's out of scope for this refactoring for now. Do you agree?
There was a problem hiding this comment.
agreed, sorry I keep getting confused about this. those queries can be null, we do these checks to prevent NPEs, makes sense.
|
left one small comment, LGTM otherwise |
As part of the refactoring of queries this PR splits the parsers parse() method into a parsing and a query building part, adding serialization, hashCode() and equals() to the builder. Relates to elastic#10217 Closes elastic#11621
cbuescher
force-pushed
the
feature/query-refactoring-boostingquery
branch
from
34bdac7 to
6202118
Compare
…ostingquery Query refactoring: BoostingQueryBuilder and Parser
As part of the refactoring of queries this PR splits the parsers parse() method into a parsing and a query building part, adding serialization, hashCode() and equals() to the builder. Relates to elastic#10217 Closes elastic#11621
…ring-boostingquery Query refactoring: BoostingQueryBuilder and Parser
|
This change is breaking for the java api as it removes setters for mandatory positive/negative query. Both arguments have to be supplied at construction time instead and have to be non-null. |
clintongormley
added
:Search/Search
As part of the refactoring of queries this PR splits the parsers parse() method into a parsing and a query building part, adding serialization, hashCode(), equals() to the builder.
In addition this changes the BoostingQueryBuilder constructor from a no-arg constructor to one requiring the mandatory postive and negative query to be set, also making those fields final internally. For this reason also the setters for these two fields were deleted.
PR goes agains query-refacoring feature branch.