Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support to match_phrase query for zero_terms_query. #29598

Merged
merged 3 commits into from
Apr 19, 2018
Merged

Add support to match_phrase query for zero_terms_query. #29598

merged 3 commits into from
Apr 19, 2018

Conversation

jtibshirani
Copy link
Contributor

Addresses #29344.

jtibshirani
jtibshirani commented Apr 19, 2018
View reviewed changes
server/src/main/java/org/elasticsearch/index/query/MatchPhraseQueryBuilder.java
@@ -39,6 +40,7 @@
public class MatchPhraseQueryBuilder extends AbstractQueryBuilder<MatchPhraseQueryBuilder> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additions in this class are a bit messy, but I favored consistency with MatchQueryBuilder and MultiMatchQueryBuilder.

@jtibshirani jtibshirani force-pushed the feature/match-phrase-zero-terms-query branch 2 times, most recently from 978a36f to e447eb6 Compare April 19, 2018 01:13
@jtibshirani jtibshirani requested a review from talevy April 19, 2018 03:16
@jtibshirani jtibshirani added the :Search/Search Search-related issues that do not fall into other categories label Apr 19, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jtibshirani jtibshirani changed the title Add support to the match_phrase query for zero_terms_query. Add support to match_phrase query for zero_terms_query. Apr 19, 2018
jimczi
jimczi requested changes Apr 19, 2018
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jtibshirani
It looks great, I left one comment regarding backward compatibility that needs to handle in the (de)serialization of the query.

server/src/main/java/org/elasticsearch/index/query/MatchPhraseQueryBuilder.java Outdated
@@ -67,6 +71,7 @@ public MatchPhraseQueryBuilder(StreamInput in) throws IOException {
fieldName = in.readString();
value = in.readGenericValue();
slop = in.readVInt();
zeroTermsQuery = ZeroTermsQuery.readFromStream(in);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to handle backward compatibility here. You can check

elasticsearch/server/src/main/java/org/elasticsearch/index/query/MatchQueryBuilder.java

Line 132 in 5263b8c

if (in.getVersion().onOrAfter(Version.V_6_1_0)) {
for an example (the version should be v7_0...).

server/src/main/java/org/elasticsearch/index/query/MatchPhraseQueryBuilder.java Outdated
@@ -75,6 +80,7 @@ protected void doWriteTo(StreamOutput out) throws IOException {
out.writeString(fieldName);
out.writeGenericValue(value);
out.writeVInt(slop);
zeroTermsQuery.writeTo(out);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, write only the out.getVersion is before 7.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing you mean 'after 7' here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, sorry

@jtibshirani jtibshirani force-pushed the feature/match-phrase-zero-terms-query branch from e447eb6 to 527d424 Compare April 19, 2018 16:46
@jtibshirani
Copy link
Contributor Author

jtibshirani commented Apr 19, 2018 •
edited
Loading

Fixed, thanks for the review!

jimczi
jimczi approved these changes Apr 19, 2018
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
You can also backport to 6x. Don t need to open a new pr though, you can just change the version for bwc when you merge in the 6x branch. Then after you commmited in 6x you can change the version in master to reflect the version compatibility.

@jtibshirani jtibshirani merged commit b9e1a00 into elastic:master Apr 19, 2018
@jtibshirani jtibshirani deleted the feature/match-phrase-zero-terms-query branch April 19, 2018 18:25
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 19, 2018
@jasontedor
Merge branch 'master' into remove-bulk-fallback
4c28942 
* master:
  Remove extra spaces from changelog
  Add support to match_phrase query for zero_terms_query. (elastic#29598)
  Fix incorrect references to 'zero_terms_docs' in query parsing error messages. (elastic#29599)
  Build: Move java home checks to pre-execution phase (elastic#29548)
  Avoid side-effect in VersionMap when assertion enabled (elastic#29585)
  [Tests] Remove accidental logger usage
  Add tests for ranking evaluation with aliases (elastic#29452)
  Deprecate use of `htmlStrip` as name for HtmlStripCharFilter (elastic#27429)
  Update plan for the removal of mapping types. (elastic#29586)
  [Docs] Add rankEval method for Jva HL client
  Make ranking evaluation details accessible for client
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Apr 19, 2018
@jtibshirani
Add support to match_phrase query for zero_terms_query. (elastic#29598)
d0e31d4 
(cherry picked from commit b9e1a00)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 19, 2018
@jasontedor
Merge branch 'master' into common-stats-flags-ordinal
cfef0e3 
* master: (21 commits)
  Remove bulk fallback for write thread pool (elastic#29609)
  Fix an incorrect reference to 'zero_terms_docs' in match_phrase queries.
  Update the version compatibility for zero_terms_query in match_phrase.
  Account translog location to ram usage in version map
  Remove extra spaces from changelog
  Add support to match_phrase query for zero_terms_query. (elastic#29598)
  Fix incorrect references to 'zero_terms_docs' in query parsing error messages. (elastic#29599)
  Build: Move java home checks to pre-execution phase (elastic#29548)
  Avoid side-effect in VersionMap when assertion enabled (elastic#29585)
  [Tests] Remove accidental logger usage
  Add tests for ranking evaluation with aliases (elastic#29452)
  Deprecate use of `htmlStrip` as name for HtmlStripCharFilter (elastic#27429)
  Update plan for the removal of mapping types. (elastic#29586)
  [Docs] Add rankEval method for Jva HL client
  Make ranking evaluation details accessible for client
  Rename the bulk thread pool to write thread pool (elastic#29593)
  [Test] Minor changes to rank_eval tests (elastic#29577)
  Fix missing node id prefix in startup logs (elastic#29534)
  Added painless execute api. (elastic#29164)
  test: also assert deprecation warning after clusters have been closed.
  ...
@jtibshirani jtibshirani mentioned this pull request Apr 19, 2018
Closed
martijnvg added a commit that referenced this pull request Apr 20, 2018
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
56b6157 
* es/master: (32 commits)
  TEST: Unmute testPrimaryRelocationWhileIndexing
  Remove remaining tribe node references (#29574)
  Never leave stale delete tombstones in version map (#29619)
  Do not serialize common stats flags using ordinal (#29600)
  Remove stale comment from JVM stats (#29625)
  TEST: Mute testPrimaryRelocationWhileIndexing
  Remove bulk fallback for write thread pool (#29609)
  Fix an incorrect reference to 'zero_terms_docs' in match_phrase queries.
  Update the version compatibility for zero_terms_query in match_phrase.
  Account translog location to ram usage in version map
  Remove extra spaces from changelog
  Add support to match_phrase query for zero_terms_query. (#29598)
  Fix incorrect references to 'zero_terms_docs' in query parsing error messages. (#29599)
  Build: Move java home checks to pre-execution phase (#29548)
  Avoid side-effect in VersionMap when assertion enabled (#29585)
  [Tests] Remove accidental logger usage
  Add tests for ranking evaluation with aliases (#29452)
  Deprecate use of `htmlStrip` as name for HtmlStripCharFilter (#27429)
  Update plan for the removal of mapping types. (#29586)
  [Docs] Add rankEval method for Jva HL client
  ...
martijnvg added a commit that referenced this pull request Apr 20, 2018
@martijnvg
Merge remote-tracking branch 'es/6.x' into ccr-6.x
1f8ea58 
* es/6.x: (28 commits)
  TEST: Unmute testPrimaryRelocationWhileIndexing
  Never leave stale delete tombstones in version map (#29619)
  Do not serialize common stats flags using ordinal (#29600)
  Remove stale comment from JVM stats (#29625)
  TEST: Mute testPrimaryRelocationWhileIndexing
  Remove 7.0.0 from 6.x changelog (#29621)
  Add support to match_phrase query for zero_terms_query. (#29598)
  Account translog location to ram usage in version map
  Avoid side-effect in VersionMap when assertion enabled (#29585)
  Build: Move java home checks to pre-execution phase (#29548)
  Add tests for ranking evaluation with aliases (#29452)
  [Test] Fix assertion in SearchDocumentationIT
  Deprecate use of `htmlStrip` as name for HtmlStripCharFilter (#27429)
  test: Assert deprecated http.enebled setting warning
  Update plan for the removal of mapping types. (#29586)
  [Docs] Add rankEval method for Jva HL client
  Make ranking evaluation details accessible for client
  Rename the bulk thread pool to write thread pool (#29593)
  [Test] Minor changes to rank_eval tests (#29577)
  test: Assert deprecated http.enebled setting warning
  ...
@russcam russcam mentioned this pull request Jul 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimczi jimczi jimczi approved these changes

@talevy talevy Awaiting requested review from talevy

Assignees
No one assigned
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v6.3.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jtibshirani @elasticmachine @jimczi @colings86