-
Notifications
You must be signed in to change notification settings - Fork 25k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Query refactoring: SpanContainingQueryBuilder and Parser #12057
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
printBoostAndQueryName(builder); | ||
builder.endObject(); | ||
} | ||
|
||
@Override | ||
protected Query doToQuery(QueryParseContext parseContext) throws IOException { | ||
Query innerBig = big.toQuery(parseContext); | ||
assert innerBig instanceof SpanQuery; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might have the same bug somewhere else I'm afraid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alrighty that makes sense now. Being paranoid, can't one set pass EmptyQueryBuilder.PROTOTYPE to the constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understand what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you said that EmptyQueryBuilder can't be used here in constructor. Why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, because it only accepts SpanQueryBuilder arguments and EmptyQueryBuilder isn't one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, sorry! gotcha
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