Refactors WildcardQueryBuilder and Parser#12110
Conversation
There was a problem hiding this comment.
we set the rewrite method twice it seems? probably a bug in the original parser
|
left a few comments, looks good though |
8024840 to
18842d3
Compare
|
@javanna Thanks for the review. I rebased and addressed all comments. |
There was a problem hiding this comment.
Would it make sense to check for empty string here as well?
There was a problem hiding this comment.
It is not something we systematically do. The parser did not do this check, hence the reason why it is not in validate.
There was a problem hiding this comment.
I think it would be interesting then to test what happens when you provide an empty string? is it something that we should accept or should we add validation for it?
There was a problem hiding this comment.
did we test this? what happens with empty string? is it something that we should allow or maybe change and disallow?
There was a problem hiding this comment.
The parser did allow for it. It does return a valid query.
There was a problem hiding this comment.
the question is if that query makes sense, does it return documents? which ones?
There was a problem hiding this comment.
it returns an empty result set.
There was a problem hiding this comment.
ok lets leave it, it doesn't seem like a problem for now
|
Left one question, looks good otherwise. |
|
@MaineC Thanks for the quick review. Should I just go ahead and push this? |
There was a problem hiding this comment.
this was added with the prefix query already right? we need to rebase maybe?
|
LGTM besides the two minor comments I left |
Relates to elastic#10217 Closes elastic#12110 This PR is against the query-refactoring branch.
64cdeaa to
94d13c7
Compare
Relates to elastic#10217 Closes elastic#12110 This PR is against the query-refactoring branch.
Relates to #10217
This PR is against the query-refactoring branch.