Remove the distinction between query and filter context in QueryBuilders

[jimczi]

When building a query Lucene distinguishes two cases, queries that require to produce a score and queries that only need to match. We cloned this mechanism in the QueryBuilders in order to be able to produce different queries based on whether they need to produce a score or not. However the only case in es that require this distinction is the BoolQueryBuilder that sets a different minimum_should_match when a bool query is built in a filter context. This behavior doesn't seem right because it makes the matching of should clauses different when the score is not required.

Closes #35293

[elasticmachine]

Pinging @elastic/es-search-aggs

[jimczi]

This is the part that confuses me. Why do we need to set minimum_should_match in this case ? I cannot think of one example where this fixes something ? @martijnvg can you confirm ?

[javanna]

heya @jimczi I was wondering, where is this change breaking?

[jimczi]

I forgot to add the breaking change ;). Today we set minimum_should_match to 1 on bool queries with should clauses if the query is built on the filter context. I don't have the history on why we did that but this makes the following query behaves differently if it is put under a filter:

{
  "query": {
    "bool": {
	"must":   {"match": {"tags": "a"}},
	"should": {"match": {"tags": "c"}},
         "should": {"match": {"tags": "d"}}
    }
  }
}

When put in a filter context the bool query above automatically changes its minimum_should_match to 1. The change in this pr removes this behavior so the should clause would be ignored/removed if they are in a filter context. I find it difficult to reason about should clauses with the current behavior. We have documentation for the current behavior in es but I mistakenly thought that it describes the behavior when there are only should clauses in the bool query.
I have two motivations for this change, the first one is to simplify the understanding of should clauses in general. The second one is that this behavior seems to be the only reason we need to separate filter and query context in es. We had other places where this distinction was required but they are all removed now so I thought that 7 would be a good chance to remove the toQuery, toFilter distinction in QueryBuilder.

[mayya-sharipova]

I wonder if also need to modify or completely remove query_filter_context.asciidoc https://github.com/elastic/elasticsearch/blob/master/docs/reference/query-dsl/query_filter_context.asciidoc

Because as I understand with your PR, we will not have query and filter contexts any more. And also what this document says about caching doesn't seem to be right (as I understood we don't cache based on query and filter context, but based on the query type)

[mayya-sharipova]

should we also add "optionally" here, like "should optionally appear".

[jimczi]

I wonder if also need to modify or completely remove query_filter_context.asciidoc https://github.com/elastic/elasticsearch/blob/master/docs/reference/query-dsl/query_filter_context.asciidoc

It's still relevant. The explanation implies that the query is sorted by relevancy so overall score of the query is taken into account. In this case the differenciation between a filter and a should clause is important. Same with the constant_score query.

I think the main question here is whether we want to make this breaking change in 7 or not.
In such case we'd also need to add deprecation warnings in 6 for queries with a mixed of should and required clauses in a filter context. If we feel that it's too late for 7 we could only deprecate now with the goal to remove it in 8.

[jpountz]

+1 this distinction was mostly useful for the terms query initially, which was parsed as a BooleanQuery when scoring was enabled and TermsQuery in a filter context. We should remove it now, I agree that the current behavior on bool queries is quite confusing.

[jimczi]

Thanks @jpountz , we discussed this some time ago and I forgot to update the pr. We've agreed that this change should be done in ES 7 and we'll deprecate the behavior in 6x. I'll update the pr with the missing breaking change and prepare the deprecation.

[matthuhiggins]

With this change, is this the shortest way to perform an unscored "OR"?

{
  "query": {
    "bool": {
      "filter": {
        "bool": {
          "should": [
            { "term": {"city": "Portland"}},
            { "term": {"state": "Washington"}}
          ],
         "minimum_should_match": 1
        }
      }
    }
  }
}

[telendt]

@matthuhiggins Yes, this is a proper way of performing "unscored disjunction (OR)". It's not the shortest way though as minimum_should_match defaults to 1 if there are only should clauses (this is a default Lucene behavior). It's still ok to specify minimum_should_match explicitly (it's also how I prefer to do it).

[matthuhiggins]

Got it, thanks. The release notes and deprecation warning leads one to think "minimum_should_match" always defaults to 0, even in the case when there are only "should" clauses.