Throw parsing exception if terms filter or query has more than one field

[imotov]

Closes #5014

[jpountz]

The TermsFilter constructor that we use indeed only supports filtering on a single field but there is another constructor that takes a list of terms (instead of a list of BytesRefs) and supports filtering based on terms that come from different fields. Maybe we could use it?

[s1monw]

+1 to fixing the terms filter parser to allow different fields!

[kimchy]

Is it really a common use case, that justifies complicating the terms filter? compared to having a terms filter that is then OR'ed using the regular query DSL?

[s1monw]

I don't think it complicates it at all. to me it just does what you would expect.

[kimchy]

maybe we can see an example of how this would look DSL wise, and decide based on that. No problem with adding it if it makes sense.

[s1monw]

to me it woudl just look like this:

{
    "constant_score" : {
        "filter" : {
            "terms" : { 
                   "user" : ["kimchy", "elasticsearch"],
                   "company" : ["elasticsearch"]
           }
        }
    }
}

[kimchy]

That can work, though it will be the first filter that works across fields in ES, and I personally find the lack of boolean logic between fields strange (not very evident if its AND or OR). Would love to hear other people thoughts.

[dadoonet]

Naive answer: writing filter like this makes me think that we want that both Terms filters to match.
So to me, it's a AND here.

[imotov]

We are handling boolean logic to some degree through execution mode. So, perhaps, we can extend it to the case of multiple fields. It might become messy though.

[s1monw]

so to me the confusion is in the name. It's called terms_filter and a term is a (field, value) tuple. I don't see how multiple fields make anything more complex or messy. You have N terms and you wanna fitler on them nothing changes if you have multiple fields they are still N terms? Exec mode is still the same you have a set of terms that's it.

[imotov]

@s1monw it becomes more complicated because before it was one dimensional (multiple values). Now it's two dimensional: (multiple fields and multiple values). Your simplification (which is natural for you since it follows Lucene design) is only one possible way to interpret the query. It also doesn't fit the current query syntax nicely since you don't specify this query as:

{
    "constant_score" : {
        "filter" : {
            "terms" : [{
                "field": "user",
                "value": "kimchy"
            }, {
                "field": "user",
                "value": "elasticsearch"
            }, {
                "field": "company",
                "value": "elasticsearch"
            }]
        }
    }
}

[s1monw]

fair enough - I thought my example was simple and consistent:

{
    "constant_score" : {
        "filter" : {
            "terms" : { 
                   "user" : ["kimchy", "elasticsearch"],
                   "company" : ["elasticsearch"]
           }
        }
    }
}

but apparently it isn't... I don't have strong feelings but it feels to me it's the wrong thing todo to throw an exception....

[lordaugustus]

A wildcard form like this
"tag.*" : ["sushi", "booyah"]
would be useful for people storing inner fields that are not predefined, like
"tag.en", "tag.de".
Maybe it is not a good schema design for multilingual data? I am not sure, but the proposed change would definitely be helpful in this case.

[clintongormley]

I'm torn on this. This is a frequent mistake that users make; they clearly expect something like this to work.

I agree completely with @kimchy that it is not obvious whether the logic is AND or OR. And I agree with @dadoonet that the most likely interpretation is as an AND, so:

{
    "constant_score" : {
        "filter" : {
            "terms" : { 
                   "user" : ["kimchy", "elasticsearch"],
                   "company" : ["elasticsearch"]
           }
        }
    }
}

... would be interpreted as:

user IN ["kimchy","elasticsearch"] AND company = 'elasticsearch'

But then there is no easy way to specify that the AND should be an OR. In the example of wildcarded field names, the user would expect an OR:

{ "terms": { "user.*": ["kimchy","elasticsearch"] }}

In fact, looking at the current terms query & filter, there is already room for parameter/fieldname clashes, eg:

{ "terms": {
    "execution": "bool",
    "field": ["values"..."]
}}

So you can't have a field called execution, _cache, _cache_key, _name or minimum_match. I'm OK with disallowing fields starting with _, but I'm not crazy about having execution and minimum_match at the same level as fieldname. In other clauses we handle this with, eg:

{ "regexp": {
    "field": {
        "value": "xx",
        "flags": "..."
}}

We could do:

{ "terms": {
    "fieldname": {
        "execution": "bool",
        "values": ["foo","bar"]
    },
    "other_fieldname": ["one","two"]
}}

But that still doesn't give us place to include the overall AND/OR logic for multiple fields, unless we add a top-level _operator flag. Of course, elsewhere in the DSL that flag is called operator, which makes this inconsistent.

One way to do it would be to have different filter names, eg

{ "any": { "user.*": ["kimchy", "elasticsearch"]}}
{ "all": {
    "user" : ["kimchy", "elasticsearch"],
    "company" : ["elasticsearch"]
}

(We're missing terms in the name, but any_terms is wrong because it is really terms_in_any_field)

And really, that's not very far from:

{ "and": [
    { "terms": { "user": ["kimchy", "elasticsearch" ]}},
    { "term": { "company": "elasticsearch" }}
]}

...but it doesn't handle wildcards in the field name.

So, lots of rumination without any solutions. What I dislike about all of these suggestions is that they complicate the terms filter and require the user to remember things (what's the default operator? etc)

My inclination would be to leave things as they are, except that we throw an exception if more than one field name is specified.

[clintongormley]

Revisiting this PR: changing the behaviour of the terms filter/query is beyond the scope of the current work, and this PR accurately reflects the situation as it is today. I think we should review and merge.

[GaelTadh]

LGTM but also looks like this is the current behavior. Did this PR already get merged ? If so can it be closed ?

[s1monw]

I agree with @GaelTadh lets get this in @imotov