Phrase Suggester: Add option to filter out phrase suggestions #6773

areek · 2014-07-07T19:25:02Z

The newly added filter option will let the user provide a template query which will be executed for every
phrase suggestions generated to ensure that the suggestion matches at least one document for the query.
The filter query is only executed on the local node for now. When the new filter option is used, the
size of the suggestion is restricted to 20.

Closes #3482

…ching any documents for a given query The newly added filter option will let the user provide a template query which will be executed for every phrase suggestions generated to ensure that the suggestion matches at least one document for the query. The filter query is only executed on the local node for now. When the new filter option is used, the size of the suggestion is restricted to 20. Closes elastic#3482

s1monw · 2014-07-08T13:44:05Z

src/main/java/org/elasticsearch/search/suggest/phrase/PhraseSuggestParser.java

+                            }
+                            CompiledScript compiledScript = suggester.scriptService().compile("mustache", templateNameOrTemplateContent);
+                            suggestion.setFilterQueryScript(compiledScript);
+                            if (suggestion.getSize() >= MAX_RESULT_SIZE_FILTER) {


I am not sure if we should do this.... we don't introduce limits elsewhere though let them figure that out themself :)

Removed limits check

s1monw · 2014-07-08T13:57:52Z

I like this PR a lot. left a bunch of comments

areek · 2014-07-08T23:35:16Z

Updated PR; incorporated feedback and Thanks for the review!

s1monw · 2014-07-09T07:26:09Z

src/main/java/org/elasticsearch/cluster/routing/operation/plain/PreferenceType.java

+/**
+ * Routing Preference Type
+ */
+public final class PreferenceType {


I'd name it just Preference...maybe we can make this an enum and have a parse method on it like:

public Preference parse(String string) { switch (valueBeforeColon(string)) { case "_shards": return SHARDS; case "_prefer_node": case "_preferNode": return PREFER_NODE; //... } } /** * this splits off the values ie in _prefer_node / _shards*/ public abstract String values();

I renamed it to Preference and changed it to an enum, but it seems the way I have it currently, there is more boiler plate added rather then being a clean solution. Examples:

Preference.ONLY_LOCAL.type()

is needed for the string representation of '_local_only'.

The values() business feels wrong on the enum, some options have to have values others don't and its not obvious wants happening.

I got rid of all the changes to the unrelated files for Preference, but still left the implementation for the feedback and how to move forward with it.

s1monw · 2014-07-09T07:36:09Z

I left some more comments. This looks great though. I'd like to get @clintongormley to give some feedback to make sure the REST interface is good

… API; Added support for accepting filters and template params

areek · 2014-07-10T21:57:15Z

I have incorporated the feedback (would like to get some comments on the Preference (see comment above).

After discussion on the REST interface, this is what it looks like now:

curl -XPOST 'localhost:9200/_search' -d {
    "suggest": {
        "text": "Xor the Got-Jewel",
        "simple_phrase": {
            "phrase": {
                "field": "body",
                "size": 5,
                "shard_size": 10,
                "confidence": 2,
                "collate": {
                    "query": {
                        "body": "{{suggestion}}"
                    },
                    "preference": "_primary"
                }
            }
        }
    }
}

collate also accepts a filter instead of the query and there is an optional params which the user can use to inject additional params to the query/filter template

s1monw · 2014-07-11T06:16:44Z

src/main/java/org/elasticsearch/cluster/routing/operation/plain/Preference.java

+            case "_onlyLocal":
+                return ONLY_LOCAL;
+            case "_only_node":
+                return ONLY_NODE.setValue(value);


I think the default case should throw an exception instead of return null?

s1monw · 2014-07-11T06:21:11Z

I left some cosmetic comments. I think this looks very good @clintongormley WDYT... In a second PR we could add the ability to just mark the results if the query / filter matched or not instead of filtering that might help folks to see what is going on.

clintongormley · 2014-07-11T12:19:40Z

docs/reference/search/suggesters/phrase-suggest.asciidoc

@@ -161,6 +161,54 @@ can contain misspellings (See parameter descriptions below).
    in a row are changed the entire phrase of changed tokens 
    is wrapped rather than each token.

+`collate`::


Here's a suggestion to make the docs slightly easier to read:

`collate`:: Checks each suggestion against the specified `query` or `filter` to prune suggestions for which not matching docs exist in the index. Either a `query` or a `filter` must be specified, and it is run as a <<query- dsl-template-query,`template` query>>. The current suggestion is automatically made available as the `{{suggestion}}` variable, which should be used in your query/filter. You can still specify your own template `params` -- the `suggestion` value will be added to the variables you specify. You can also specify a `preference` to control on which shards the query is executed (see <<search-request-preference). The default value is `_only_local`. + [source,js] -------------------------------------------------- curl -XPOST 'localhost:9200/_search' -d { "suggest" : { "text" : "Xor the Got-Jewel", "simple_phrase" : { "phrase" : { "field" : "bigram", "size" : 1, "direct_generator" : [ { "field" : "body", "suggest_mode" : "always", "min_word_length" : 1 } ], "collate": { "query": { <1> "match": { "{{field_name}}" : "{{suggestion}}" <2> } }, "params": {"field_name" : "title"}, <3> "preference": "_primary", <4> } } } } } -------------------------------------------------- <1> This query will be run once for every suggestion. <2> The `{{suggestion}}` variable will be replaced by the text of each suggestion. <3> An additional `field_name` variable has been specified in `params` and is used by the `match` query. <4> The default `preference` has been changed to `_primary`.

I also removed most of the options from the suggest request which are not relevant to collate

Thanks @clintongormley, I have updated the docs

clintongormley · 2014-07-11T12:38:18Z

Suggested some doc changes, but otherwise LGTM

areek · 2014-07-11T16:47:19Z

I have updated the PR with the feedback. I think the enhancement is ready but still not happy with the state of Preference (see comment in Preference).

s1monw · 2014-07-14T08:12:19Z

...but still not happy with the state of Preference (see comment in Preference)

I didn't see a comment, what are you not happy with?

this LGTM actually and I think we should move forward with it unless the issue is major which I didn't see yet?

areek · 2014-07-14T14:06:59Z

It seems the way I have Preference currently, there is more boiler plate added rather then being a clean solution. Examples:

Preference.ONLY_LOCAL.type()

is needed for '_local_only'.

The values() business feels wrong on the enum, some options have to have values others don't and its not obvious wants happening.

That was my concern, I think if we plan to have it as is, I will atleast want to use the new Preference type in more places than in PhraseSuggester (at least use it in the PlainOperationRouting)

areek · 2014-07-14T17:54:30Z

Updated PR:

added static helper methods to parse Preference type & value
use new Preference enum in PlainOperationRouting

s1monw · 2014-07-14T19:29:33Z

src/main/java/org/elasticsearch/cluster/routing/operation/plain/PlainOperationRouting.java

            }
-            if ("_local".equals(preference)) {


I think now that we have an enum we should just use a switch / case statement here?

s1monw · 2014-07-14T19:57:25Z

LGTM please squash and push

areek added review labels Jul 7, 2014

s1monw reviewed Jul 8, 2014
View reviewed changes

s1monw removed the review label Jul 8, 2014

areek added 3 commits July 8, 2014 19:20

Incorporated Feedback

f3d38df

Use PreferenceType instead of raw strings

ceac6ec

removed unused method

2693624

s1monw reviewed Jul 9, 2014
View reviewed changes

s1monw assigned areek Jul 9, 2014

incorporated feedback; added more tests; changed filter to collate in…

6181d83

… API; Added support for accepting filters and template params

areek added the review label Jul 10, 2014

areek added 3 commits July 10, 2014 18:00

minor change in comments; remove redundant code

902763b

remove unused imports

be328f8

remove unused imports; restore to default

b4533aa

s1monw reviewed Jul 11, 2014
View reviewed changes

clintongormley reviewed Jul 11, 2014
View reviewed changes

incorporated feedback; added tests; minor changes in exception msgs

0d7cafe

minor change to fix asciidoc

359691d

s1monw added v1.4.0 and removed review labels Jul 14, 2014

fix Preference enum

f23712e

areek added the review label Jul 14, 2014

Removed Preference.parseValue

d6e1f07

s1monw reviewed Jul 14, 2014
View reviewed changes

areek added 3 commits July 14, 2014 15:40

minor fix

5df71bb

minor fix

b4a905d

minor fix

beea2ab

areek removed review labels Jul 14, 2014

areek closed this Jul 14, 2014

clintongormley added >feature v2.0.0-beta1 v1.3.0 :Search/Suggesters "Did you mean" and suggestions as you type labels Jun 6, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phrase Suggester: Add option to filter out phrase suggestions #6773

Phrase Suggester: Add option to filter out phrase suggestions #6773

areek commented Jul 7, 2014

s1monw Jul 8, 2014

areek Jul 8, 2014

s1monw commented Jul 8, 2014

areek commented Jul 8, 2014

s1monw Jul 9, 2014

areek Jul 10, 2014

s1monw commented Jul 9, 2014

areek commented Jul 10, 2014

s1monw Jul 11, 2014

areek Jul 11, 2014

s1monw commented Jul 11, 2014

clintongormley Jul 11, 2014

areek Jul 11, 2014

clintongormley commented Jul 11, 2014

areek commented Jul 11, 2014

s1monw commented Jul 14, 2014

areek commented Jul 14, 2014

areek commented Jul 14, 2014

s1monw Jul 14, 2014

s1monw commented Jul 14, 2014

Phrase Suggester: Add option to filter out phrase suggestions #6773

Phrase Suggester: Add option to filter out phrase suggestions #6773

Conversation

areek commented Jul 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1monw commented Jul 8, 2014

areek commented Jul 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1monw commented Jul 9, 2014

areek commented Jul 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1monw commented Jul 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintongormley commented Jul 11, 2014

areek commented Jul 11, 2014

s1monw commented Jul 14, 2014

areek commented Jul 14, 2014

areek commented Jul 14, 2014

Choose a reason for hiding this comment

s1monw commented Jul 14, 2014