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

Additional Graph Support in Match Query #22503

Merged
merged 1 commit into from Jan 10, 2017

Conversation

Projects
None yet
5 participants
@mattweber
Copy link
Contributor

commented Jan 9, 2017

Make match queries that use phrase prefix or cutoff frequency options
graph aware.

Closes #22490

@mattweber

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2017

Also, this can be cherry picked into 5x without conflict. I can open PR for that if needed.

@mikemccand
Copy link
Contributor

left a comment

LGTM

I love the test cases :)

core/src/test/java/org/elasticsearch/index/search/MatchQueryIT.java Outdated
*/
@Override
protected int numberOfShards() {
return 1;

This comment has been minimized.

Copy link
@pickypg

pickypg Jan 9, 2017

Member

I worry that this may end up hiding some cross-shard issues for other queries. Perhaps this should be controlled via the tests that need it (possibly using DFS if that works?)?

Since I'm not familiar with what Lucene's Graph query is even doing, I just wanted to put this out there.

Additional Graph Support in Match Query
Make match queries that use phrase prefix or cutoff frequency options
graph aware.

@mattweber mattweber force-pushed the mattweber:issue22490 branch to 86a2a8b Jan 9, 2017

@mattweber

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2017

@pickypg I switched it so that test using routing to force everything into the same shard. How does it look now?

BTW, GraphQuery is really just a simple wrapper around a boolean query at this point with a few helper methods we can use to know if it hold other boolean or phrase queries which might need additional processing. At some point, it might be extended to rewrite other graph aware queries such as TermAutomatonQuery.

@jimczi

jimczi approved these changes Jan 10, 2017

Copy link
Member

left a comment

Good catch, thanks for doing this.
LGTM

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

Thanks @mattweber, I'll merge this later today.

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

Also, this can be cherry picked into 5x without conflict. I can open PR for that if needed.

No need for a separate PR; I'll cherry-pick to 5.x!

@mikemccand mikemccand merged commit 28273e0 into elastic:master Jan 10, 2017

1 check passed

CLA Commit author has signed the CLA
Details

mikemccand added a commit that referenced this pull request Jan 10, 2017

Additional Graph Support in Match Query (#22503)
Make match queries that use phrase prefix or cutoff frequency options
graph aware.

Closes #22490
@pickypg

This comment has been minimized.

Copy link
Member

commented Jan 10, 2017

@mattweber Nice change! Thanks!

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.