Terms aggregations shows partial results with terms aggregation over invalid field

[greentruff]

Elasticsearch version (bin/elasticsearch --version): 6.8.1
Works as expected in 6.4.3.
Unexpected behavior for versions 6.5.3 to 6.8.1

Plugins installed: Default plugins in docker image provided Elastic

JVM version (java -version): 11.0

OS version (uname -a if on a Unix-like system):

Description of the problem including expected versus actual behavior:
Aggregations with a term referrencing a min aggregation over an invalid field does not return results per bucket. Other aggregations are not affected.

Steps to reproduce:

The following script reproduces the issue on a local default ES instance:

import requests

ES = 'http://localhost:9200'

## Set up
requests.delete(ES + '/parrot')

requests.put(ES + '/parrot')

requests.post(ES + '/parrot/doc', json={ "a": 1, "b": 2 })
requests.post(ES + '/parrot/doc', json={ "a": 1, "b": 2 })
requests.post(ES + '/parrot/doc', json={ "a": 2, "b": 3 })

requests.post(ES + '/parrot/_refresh')

resp = requests.get(ES + '/parrot/_search', json={
    "size":0,
    "timeout":"1m",
    "query":{
        "constant_score":{
            "filter":{
                "bool":{
                    "filter":[{"exists":{"field":"a","boost":1.0}}],
                    "adjust_pure_negative":True,"boost":1.0
                }
            },"boost":1.0
        }
    },
    "aggregations":{
        "v__max:max":{"max":{"field":"invalid"}},
        "a":{"terms":{"field":"a","show_term_doc_count_error":False,"order":[{"v__max:max":"asc"},{"_key":"asc"}]},
        "aggregations":{"v__max:max":{"max":{"field":"invalid"}}}}
    }
})
print(resp.json())

resp = requests.get(ES + '/parrot/_search', json={
    "size":0,
    "timeout":"1m",
    "query":{
        "constant_score":{
            "filter":{
                "bool":{
                    "filter":[{"exists":{"field":"a","boost":1.0}}],
                    "adjust_pure_negative":True,"boost":1.0
                }
            },"boost":1.0
        }
    },
    "aggregations":{
        "v__min:min":{"min":{"field":"invalid"}},
        "a":{"terms":{"field":"a","show_term_doc_count_error":False,"order":[{"v__min:min":"asc"},{"_key":"asc"}]},
        "aggregations":{"v__min:min":{"min":{"field":"invalid"}}}}
    }
})
print(resp.json())

Actual behavior:

{
  'took':369,
  'timed_out':False,
  '_shards':{
    'total':5,
    'successful':5,
    'skipped':0,
    'failed':0
  },
  'hits':{
    'total':3,
    'max_score':0.0,
    'hits':[

    ]
  },
  'aggregations':{
    'a':{
      'doc_count_error_upper_bound':0,
      'sum_other_doc_count':0,
      'buckets':[
        {
          'key':1,
          'doc_count':2,
          'v__max:max':{
            'value':None
          }
        },
        {
          'key':2,
          'doc_count':1,
          'v__max:max':{
            'value':None
          }
        }
      ]
    },
    'v__max:max':{
      'value':None
    }
  }
}

{
  'took':22,
  'timed_out':False,
  '_shards':{
    'total':5,
    'successful':5,
    'skipped':0,
    'failed':0
  },
  'hits':{
    'total':3,
    'max_score':0.0,
    'hits':[

    ]
  },
  'aggregations':{
    'a':{
      'doc_count_error_upper_bound':0,
      'sum_other_doc_count':0,
      'buckets':[

      ]
    },
    'v__min:min':{
      'value':None
    }
  }
}

The query with min returns no buckets.

Expected behavior:

{'took': 256, 'timed_out': False, '_shards': {'total': 5, 'successful': 5, 'skipped': 0, 'failed': 0}, 'hits': {'total': 3, 'max_score': 0.0, 'hits': []}, 'aggregations': {'a': {'doc_count_error_upper_bound': 0, 'sum_other_doc_count': 0, 'buckets': [{'key': 1, 'doc_count': 2, 'v__max:max': {'value': None}}, {'key': 2, 'doc_count': 1, 'v__max:max': {'value': None}}]}, 'v__max:max': {'value': None}}}

{'took': 156, 'timed_out': False, '_shards': {'total': 5, 'successful': 5, 'skipped': 0, 'failed': 0}, 'hits': {'total': 3, 'max_score': 0.0, 'hits': []}, 'aggregations': {'a': {'doc_count_error_upper_bound': 0, 'sum_other_doc_count': 0, 'buckets': [{'key': 1, 'doc_count': 2, 'v__min:min': {'value': None}}, {'key': 2, 'doc_count': 1, 'v__min:min': {'value': None}}]}, 'v__min:min': {'value': None}}}

[elasticmachine]

Pinging @elastic/es-analytics-geo

[Hohol]

I think I've found the cause of this bug.

elasticsearch/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java

Lines 91 to 96 in a5df840

	if (parent != null) {
	return LeafBucketCollector.NO_OP_COLLECTOR;
	} else {
	// we have no parent and the values source is empty so we can skip collecting hits.
	throw new CollectionTerminatedException();
	}

elasticsearch/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java

Lines 96 to 101 in a5df840

	if (parent == null) {
	return LeafBucketCollector.NO_OP_COLLECTOR;
	} else {
	// we have no parent and the values source is empty so we can skip collecting hits.
	throw new CollectionTerminatedException();
	}

Looks like the second snippet is incorrect.

MinAggregator and MaxAggregators classes are weird. I believe they should be almost identical, but actually, there are many differences.
I think they should be refactored to get rid of code duplication and any unwanted differences.

I can work on this if someone from Elastic team approves.

[jimczi]

Thanks for reporting @greentruff . I opened #44963 to fix the bug, @Hohol these snippets are correct, what's missing is the handling of the CollectionTerminatedException in the deferring collectors (see #44963).

[polyfractal]

@Hohol Regarding the code differences, the reason they look so different is due to some early termination optimization. If possible, both aggregators attempt to use the BKD to lookup the min or max because that is a lot faster than iterating over all the documents to collect the values.

The BKD tree sorts the values ascending, so the min is at the left-most leaf in the tree. The min aggregator walks the tree leaves until it finds the first non-deleted document, then exits the BKD intersection and returns the value.

In contrast, the max aggregator has a comparably more difficult job. BKD intersections proceed from least-to-greatest, and we don't want to walk the whole tree. So the Max agg only inspects leaves that contain the maximum value for the segment (e.g. the last leaf), and then scans through those values to see which is the largest and also not deleted. So the max agg is more heuristic in nature, if all the docs are deleted in the last leaf it will fall back to iterating over all the values to find the max.

Thus, the differences in how the code looks :) On the surface they look similar, but due to how the tree is structured it introduces some subtle differences.

Pretty sure these details are at least mostly right, Jim can correct me if I got anything grossly wrong :)

[Hohol]

Thanks for the explanation!