Query refactoring: BoostingQueryBuilder and Parser#11621
Conversation
b73dc26 to
55998ac
Compare
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 |
55998ac to
34bdac7
Compare
|
Just posted an update reverting the changes in the constructor and adressing other comments. |
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
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. |
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.