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

Add a limit for graph phrase query expansion #34061

Merged
merged 6 commits into from
Oct 19, 2018

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Sep 25, 2018

Backport of #34031 for 5.6.

@jimczi jimczi added >bug :Search/Search Search-related issues that do not fall into other categories v5.6.12 labels Sep 25, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jimczi jimczi added v5.6.13 and removed v5.6.12 labels Oct 18, 2018
Copy link
Contributor Author

@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.

@colings86 , as discussed with Jason yesterday I added a JVM option named es.query.apply_graph_phrase_limit to activate the limit in this version. By default it is deactivated and the only valid value for the JVM option is true.

Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

@jimczi I left two comment on user feddback but otherwise I think this change is good

Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

Left a super minor comment but this LGTM, thanks @jimczi

@@ -889,11 +888,12 @@ protected SpanQuery analyzeGraphPhrase(TokenStream source, String field, int phr
* The JVM option can only be set to <code>true</code> (false is the default value), any other value
* will throw an {@link IllegalArgumentException}.
*/
// public for tests
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this isn't just public for tests now that SearchModule calls it?

@jimczi jimczi merged commit 42b2268 into elastic:5.6 Oct 19, 2018
@jimczi jimczi deleted the max_graph_phrase_expansion_5_6 branch October 19, 2018 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories v5.6.13
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants