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.

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

QueryString and SimpleQueryString Graph Support #22541

Merged
merged 1 commit into from Jan 11, 2017

Conversation

Projects
None yet
4 participants
@mattweber
Copy link
Contributor

commented Jan 11, 2017

Add support for graph token streams to "query_string" and
"simple_query_string" queries.

@mattweber

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2017

There is a easily fixable conflict on cherry-picking this into 5.x. I have a branch ready for a PR against that if needed.

@mattweber mattweber force-pushed the mattweber:simple_query_string_graph_support branch Jan 11, 2017

@mattweber

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2017

Removed the note in synonym_graph docs saying that only match queries are supported. I believe all analyzed queries are supported once this PR makes it in.

@mikemccand @jimczi can you please review?

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2017

Thanks @mattweber I will review!

core/src/main/java/org/elasticsearch/common/lucene/search/Queries.java Outdated
List<Query> oldQueries = ((GraphQuery) query).getQueries();
Query[] queries = new Query[oldQueries.size()];
for (int i = 0; i < queries.length; i++) {
Query oldQuery = oldQueries.get(i);

This comment has been minimized.

Copy link
@jimczi

jimczi Jan 11, 2017

Member

Maybe just:

queries[i] = maybeApplyMinimumShouldMatch(oldQueries.get(i));

... this would avoid the duplicate code below ?

@jimczi
Copy link
Member

left a comment

This looks great !
Not related to this PR but I think MultiMatchQuery#parseAndApply does not apply minimumShouldMatch to GraphQuery. Since you've added maybeApplyMinimumShouldMatch in this PR you can maybe add the missing call in MultiMatchQuery ? We can also add it in a follow up.

It's great that query_string and simple_query_string can now produce valid graph query. It's also possible to handle multi words synonyms if split_on_whitespace is set to false.

@mattweber

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2017

Thanks @jimczi Let me look into the multi-match query and get that fix in this PR if needed.

@mattweber mattweber force-pushed the mattweber:simple_query_string_graph_support branch Jan 11, 2017

@mattweber

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2017

@jimczi addressed your comments, can you please have another look? Good catch on the MultiMatchQuery#parseAndApply, I had totally missed that in earlier PR's.

assertHitCount(searchResponse, 3L);
assertSearchHits(searchResponse, "1", "2", "6");
}

This comment has been minimized.

Copy link
@jimczi

jimczi Jan 11, 2017

Member

Can you add a test with splitOnWhitespace(true) as well ? Something that checks that single term synonyms that rewrite to multi terms are handled correctly.

@jimczi

jimczi approved these changes Jan 11, 2017

Copy link
Member

left a comment

I left a small comment regarding tests with split_on_whitespace:true.
Otherwise LGTM, this is a great change that will make a lot of people happy ! Thanks for doing this.
On a side note it is better to just add commits during the review process rather than squashing/rebasing your change after each review. It is easier for the reviewer to follow the latest changes by looking at each commit separately instead of reading the whole change each time.

@mattweber

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2017

Thanks @jimczi I will add that test.

@mattweber

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2017

@jimczi test added. Thanks for reviewing!

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2017

LGTM

Thanks @mattweber and @jimczi!

QueryString and SimpleQueryString Graph Support
Add support for graph token streams to "query_String" and
"simple_query_string" queries.

@mattweber mattweber force-pushed the mattweber:simple_query_string_graph_support branch to 0adeb04 Jan 11, 2017

@mattweber

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2017

Thanks @mikemccand. I have squashed commits so it is ready for merge. I will work on a 5.x PR with the changes from earlier.

@jimczi jimczi merged commit 609d2aa into elastic:master Jan 11, 2017

1 check passed

CLA Commit author has signed the CLA
Details

jimczi added a commit that referenced this pull request Jan 11, 2017

QueryString and SimpleQueryString Graph Support (#22541)
Add support for graph token streams to "query_String" and
"simple_query_string" queries.
@jimczi

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

Thanks @mattweber @mikemccand ! This is now pushed to master and 5.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.