-
Notifications
You must be signed in to change notification settings - Fork 25.4k
Refactors WildcardQueryBuilder and Parser #12110
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
Refactors WildcardQueryBuilder and Parser #12110
Conversation
|
||
WildcardQuery query = new WildcardQuery(new Term(indexFieldName, valueBytes)); | ||
QueryParsers.setRewriteMethod(query, rewrite); | ||
query.setRewriteMethod(QueryParsers.parseRewriteMethod(rewrite)); |
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 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. |
if (Strings.isEmpty(this.fieldName)) { | ||
validationException = addValidationError("field name cannot be null or empty.", validationException); | ||
} | ||
if (this.value == null) { |
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.
Would it make sense to check for empty string here as well?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parser did allow for it. It does return a valid query.
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.
the question is if that query makes sense, does it return documents? which ones?
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.
it returns an empty result set.
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.
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? |
/** | ||
* Helper method to return a random rewrite method | ||
*/ | ||
protected static String getRandomRewriteMethod() { |
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.
this was added with the prefix query already right? we need to rebase maybe?
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.
yes of course
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.