Fix sorting agg buckets by doc_count (backport of #53617) (#53627)

I broke sorting aggregations by `doc_count` in #51271 by mixing up true
and false. This flips that comparison and adds a few tests to double
check that we don't so this again.

rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/50_filter.yml

@@ -14,10 +14,10 @@ setup:
                   type: keyword
 
   - do:
-        index:
+      index:
-          index: test
+        index: test
-          id: foo|bar|baz0
+        id: foo|bar|baz0
-          body: { "notifications" : ["abc"] }
+        body: { "notifications" : ["abc"] }
 
   - do:
       index:
@@ -75,3 +75,82 @@ setup:
   - match: { _all.total.request_cache.hit_count: 0 }
   - match: { _all.total.request_cache.miss_count: 1 }
   - is_true: indices.test
+
+---
+"As a child of terms":
+  - skip:
+      version: " - 6.99.99"
+      reason: the test is written for hits.total.value
+
+  - do:
+      bulk:
+        refresh: true
+        index: test
+        body: |
+          {"index":{}}
+          {"category": "bar", "val": 8}
+          {"index":{}}
+          {"category": "bar", "val": 0}
+  - do:
+      search:
+        size: 0
+        body:
+          aggs:
+            category:
+              terms:
+                field: category.keyword
+              aggs:
+                high:
+                  filter:
+                    range:
+                      val:
+                        gte: 7
+
+  - match: { hits.total.value: 4 }
+  - match: { aggregations.category.buckets.0.key: bar }
+  - match: { aggregations.category.buckets.0.doc_count: 2 }
+  - match: { aggregations.category.buckets.0.high.doc_count: 1 }
+
+---
+"Sorting terms":
+  - skip:
+      version: " - 7.6.99"
+      reason: fixed in 7.7.0
+
+  - do:
+      bulk:
+        refresh: true
+        index: test
+        body: |
+          {"index":{}}
+          {"category": "foo", "val": 7}
+          {"index":{}}
+          {"category": "bar", "val": 8}
+          {"index":{}}
+          {"category": "bar", "val": 9}
+          {"index":{}}
+          {"category": "bar", "val": 0}
+  - do:
+      search:
+        size: 0
+        body:
+          aggs:
+            category:
+              terms:
+                field: category.keyword
+                order:
+                  high.doc_count: desc
+              aggs:
+                high:
+                  filter:
+                    range:
+                      val:
+                        gte: 7
+
+  - match: { hits.total.value: 6 }
+  - match: { aggregations.category.buckets.0.key: bar }
+  - match: { aggregations.category.buckets.0.doc_count: 3 }
+  - match: { aggregations.category.buckets.0.high.doc_count: 2 }
+  - match: { aggregations.category.buckets.1.key: foo }
+  - match: { aggregations.category.buckets.1.doc_count: 1 }
+  - match: { aggregations.category.buckets.1.high.doc_count: 1 }

server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java

@@ -176,7 +176,7 @@ public abstract class BucketsAggregator extends AggregatorBase {
 
     @Override
     public BucketComparator bucketComparator(String key, SortOrder order) {
-        if (key == null || false == "doc_count".equals(key)) {
+        if (key == null || "doc_count".equals(key)) {
             return (lhs, rhs) -> order.reverseMul() * Integer.compare(bucketDocCount(lhs), bucketDocCount(rhs));
         }
         throw new IllegalArgumentException("Ordering on a single-bucket aggregation can only be done on its doc_count. " +

server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java

@@ -29,12 +29,23 @@ import org.apache.lucene.search.MatchAllDocsQuery;
 import org.apache.lucene.store.Directory;
 import org.elasticsearch.index.mapper.KeywordFieldMapper;
 import org.elasticsearch.index.mapper.MappedFieldType;
+import org.elasticsearch.index.query.MatchAllQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.index.query.QueryBuilders;
+import org.elasticsearch.search.aggregations.Aggregator.BucketComparator;
 import org.elasticsearch.search.aggregations.AggregatorTestCase;
+import org.elasticsearch.search.aggregations.LeafBucketCollector;
 import org.elasticsearch.search.aggregations.support.AggregationInspectionHelper;
+import org.elasticsearch.search.sort.SortOrder;
 import org.junit.Before;
 
+import java.io.IOException;
+
+import static java.util.Collections.singleton;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.lessThan;
+
 public class FilterAggregatorTests extends AggregatorTestCase {
     private MappedFieldType fieldType;
 
@@ -110,6 +121,33 @@ public class FilterAggregatorTests extends AggregatorTestCase {
             indexReader.close();
             directory.close();
         }
+    }
 
+    public void testBucketComparator() throws IOException {
+        try (Directory directory = newDirectory()) {
+            try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
+                indexWriter.addDocument(singleton(new Field("field", "1", fieldType)));
+            }
+            try (IndexReader indexReader = DirectoryReader.open(directory)) {
+                IndexSearcher indexSearcher = newSearcher(indexReader, true, true);
+                FilterAggregationBuilder builder = new FilterAggregationBuilder("test", new MatchAllQueryBuilder());
+                FilterAggregator agg = createAggregator(builder, indexSearcher, fieldType);
+                agg.preCollection();
+                LeafBucketCollector collector = agg.getLeafCollector(indexReader.leaves().get(0));
+                collector.collect(0, 0);
+                collector.collect(0, 0);
+                collector.collect(0, 1);
+                BucketComparator c = agg.bucketComparator(null, SortOrder.ASC);
+                assertThat(c.compare(0, 1), greaterThan(0));
+                assertThat(c.compare(1, 0), lessThan(0));
+                c = agg.bucketComparator("doc_count", SortOrder.ASC);
+                assertThat(c.compare(0, 1), greaterThan(0));
+                assertThat(c.compare(1, 0), lessThan(0));
+                Exception e = expectThrows(IllegalArgumentException.class, () ->
+                    agg.bucketComparator("garbage", randomFrom(SortOrder.values())));
+                assertThat(e.getMessage(), equalTo("Ordering on a single-bucket aggregation can only be done on its doc_count. "
+                        + "Either drop the key (a la \"test\") or change it to \"doc_count\" (a la \"test.doc_count\") or \"key\"."));
+            }
+        }
     }
 }