Query refactoring: SpanContainingQueryBuilder and Parser#12057
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
There was a problem hiding this comment.
I think this assert may trip cause the result of toQuery might be null in case big is EmptyQueryBuilder?
There was a problem hiding this comment.
we might have the same bug somewhere else I'm afraid
There was a problem hiding this comment.
EmptyQueryBuilder can not be inner SpanQueryBuilder because of instanceof checks in parser, also can't be used here in constructor, so I think were good here? The whole nested SpanQueryBuilder hierarchy shouldn't suffer from this problem, in theory it would be possible to nest an empty { } in them but that already would not be accepted by the current DSL I think.
There was a problem hiding this comment.
alrighty that makes sense now. Being paranoid, can't one set pass EmptyQueryBuilder.PROTOTYPE to the constructor?
There was a problem hiding this comment.
Not sure if I understand what you mean.
There was a problem hiding this comment.
you said that EmptyQueryBuilder can't be used here in constructor. Why not?
There was a problem hiding this comment.
I see, because it only accepts SpanQueryBuilder arguments and EmptyQueryBuilder isn't one.
|
left a few minor comments |
|
Changed the test query setup to use switch. |
|
LGTM |
…ancontaining Query refactoring: SpanContainingQueryBuilder and Parser
…ring-spancontaining Query refactoring: SpanContainingQueryBuilder and Parser
|
This change is breaking for the java api as it removes setters for mandatory big/little inner span queries. Both arguments now have to be supplied at construction time instead and have to be non-null. |
Moving the query building functionality from the parser to the builders
new toQuery() method analogous to other recent query refactorings.
PR is against query refactoring branch.
Relates to #10217