Inner_hits in should clauses override each other

[imotov]

This issues is somewhat similar to #16218, but it manifests itself if two should clauses contain has_child with inner_hits and different queries and only one of the query in the last clause doesn't matches the document returned by the first clause.

This is a repro for 6.x

DELETE test
PUT test
{
  "mappings": {
    "_doc": {
      "properties": {
        "my_join_field": {
          "type": "join",
          "relations": {
            "question": "answer"
          }
        },
        "text": {
          "type": "text"
        }
      }
    }
  }
}

POST test/_doc/_bulk
{"index":{"_id": "1"}}
{"text": "This is a question","my_join_field": {"name": "question"}}
{"index":{"_id": "2"}}
{"text": "This is another question","my_join_field": {"name": "question"}}
{"index":{"_id": "3", "_routing": "1"}}
{"text": "This is an answer", "my_join_field": {"name": "answer","parent":"1"}}
{"index":{"_id": "4", "_routing": "1"}}
{"text": "This is another answer", "my_join_field": {"name": "answer","parent":"1"}}

POST test/_search

# Doesn't return inner hits
POST test/_search
{
  "query": {
    "bool": {
      "should": [
        {
          "has_child": {
            "type": "answer",
            "query": {
              "match": {
                "text": "answer"
              }
            },
            "inner_hits": {}
          }
        },
        {
          "has_child": {
            "type": "answer",
            "query": {
              "match": {
                "text": "noanswer"
              }
            },
            "inner_hits": {}
          }
        }
      ]
    }
  }
}

# Returns inner hits:
POST test/_search
{
  "query": {
    "bool": {
      "should": [
        {
          "has_child": {
            "type": "answer",
            "query": {
              "match": {
                "text": "noanswer"
              }
            },
            "inner_hits": {}
          }
        },
        {
          "has_child": {
            "type": "answer",
            "query": {
              "match": {
                "text": "answer"
              }
            },
            "inner_hits": {}
          }
        }
      ]
    }
  }
}

Repro for 7.x can be found here

Originally reported in https://discuss.elastic.co/t/inner-hits-has-child/164627/10

[elasticmachine]

Pinging @elastic/es-search

[jimczi]

inner_hits takes the name of the nested path by default. If you have two queries that uses the same path you need to use a different name to disambiguate. It's not explained well in the docs and we should throw an error if we find two inner_hits with the same name so I am not closing this issue and will mark it as a bug. However the workaround is simple, just set a different name on each inner_hits and they will be returned in different sections in the search response.

[kwojcicki]

Hey @jimczi, @imotov looking to take a crack at this :) would looking at https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java and https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java be a good start?

[jimczi]

looking to take a crack at this

This is the function that populates the inner_hits map at each level.
We should check that the name is not already used when entries are added in the map. This can happen in three places:

• NestedQueryBuilder
• HasChildQueryBuilder
• HasParentQueryBuilder

All these functions should throw an illegal argument exception if the map already contains an entry with the same name.

[kwojcicki]

Perfect thanks @jimczi :)