-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Splits phrase
and phrase_prefix
in match query into MatchPhraseQueryBuilder
and MatchPhrasePrefixQueryBuilder
#17508
Conversation
* | ||
* @deprecated for phrase queries use {@link MatchPhraseQueryBuilder} | ||
*/ | ||
@Deprecated |
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.
Shall we also deprecate the Type enum in MatchQuery so we don't forget to remove it at some point?
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 still need MatchQuery.Type
as we are still using MatchQuery
underneath and need to tell it which mode to use.
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.
oh right....unless we split the lucene query too. gotcha
left a few comments, looks good though. I also wonder if it makes sense to split the MatchQuery at the lucene level, seems that we have a few switches and instanceof checks that depend on the selected type. That can wait for another PR though. |
Yeah it might be a good idea to split the MatchQuery too, haven\t looked at how easy/hard that would be yet though |
// move to the next token | ||
token = parser.nextToken(); | ||
if (token != XContentParser.Token.END_OBJECT) { | ||
throw new ParsingException(parser.getTokenLocation(), "[match] query parsed in simplified form, with direct field name, " |
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.
shall we replace [match] in the error message with the name of this specific query?
left a few more comments, it's close though |
import java.io.IOException; | ||
import java.util.Objects; | ||
|
||
public class MatchPhraseQueryBuilder extends AbstractQueryBuilder<MatchPhraseQueryBuilder> { |
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.
can we have some javadocs on these new classes?
LGTM besides those last minor comments I left. |
…eryBuilder` and `MatchPhrasePrefixQueryBuilder` The `phrase` and `phrase_prefix` options in the `MatchQueryBuilder` have been deprecated in favour of using the new `MatchPhraseQueryBuilder` and `MatchPhrasePrefixQueryBuilder`. This is not a breaking change since `MatchQueryBuilder` still supports `phrase` and `phrase_prefix` but this option will be removed from the `MatchQueryBuilder` in the future (probably in 6.0) Relates to #17458 (comment)
phrase
and phrase_prefix in match query into
MatchPhraseQueryBuilder and
MatchPhrasePrefixQueryBuilder`phrase
and phrase_prefix
in match query into MatchPhraseQueryBuilder
and MatchPhrasePrefixQueryBuilder
The
phrase
andphrase_prefix
options in theMatchQueryBuilder
have been deprecated in favour of using the newMatchPhraseQueryBuilder
andMatchPhrasePrefixQueryBuilder
. This means that there is only one name for each query and also means that we can better validate the options which are compatible (for example you could previously define amatch_phrase
query and add fuzziness options which actually had no effect on the query)This is not a breaking change since
MatchQueryBuilder
still supportsphrase
andphrase_prefix
but this option will be removed from theMatchQueryBuilder
in the future (probably in 6.0)Relates to #17458 (comment)