From 9845dbb7d6d426dba717fb1310d18d024b303682 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 16 Mar 2020 17:35:43 -0400 Subject: [PATCH] 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. --- .../test/search.aggregation/50_filter.yml | 87 ++++++++++++++++++- .../bucket/BucketsAggregator.java | 2 +- .../bucket/filter/FilterAggregatorTests.java | 38 ++++++++ 3 files changed, 122 insertions(+), 5 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/50_filter.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/50_filter.yml index c32cae9ff82..bc4105af85e 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/50_filter.yml +++ b/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: test - id: foo|bar|baz0 - body: { "notifications" : ["abc"] } + index: + index: test + id: foo|bar|baz0 + 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 } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java index f26cd0478a0..acf4d6c7bcb 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java +++ b/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. " + diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java index 56153d2d35f..9e57545a47b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java +++ b/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\".")); + } + } } }