From 23d11ed7292dfc9c714dbb8c3ed56a516d0601e4 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Wed, 14 Dec 2016 12:37:11 +0100 Subject: [PATCH] Fix numeric terms aggregations with includes/excludes and minDocCount=0 (#22141) For minDocCount=0 the numeric terms aggregator should also check the includes/excludes when buckets with empty count are added to the result. This change fixes this bug and adds a test for it. Fixes #22140 --- .../bucket/terms/LongTermsAggregator.java | 5 +- .../aggregations/bucket/LongTermsIT.java | 46 ++++++++++++------- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongTermsAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongTermsAggregator.java index ffb7da13bc0ed..e041ada417f0c 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongTermsAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/LongTermsAggregator.java @@ -113,7 +113,10 @@ public InternalAggregation buildAggregation(long owningBucketOrdinal) throws IOE values.setDocument(docId); final int valueCount = values.count(); for (int i = 0; i < valueCount; ++i) { - bucketOrds.add(values.valueAt(i)); + long value = values.valueAt(i); + if (longFilter == null || longFilter.accept(value)) { + bucketOrds.add(value); + } } } } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/LongTermsIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/LongTermsIT.java index 35905f91a91ca..a314bb2724db0 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/LongTermsIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/LongTermsIT.java @@ -305,41 +305,53 @@ public void testSingleValueFieldWithFiltering() throws Exception { long includes[] = { 1, 2, 3, 98 }; long excludes[] = { -1, 2, 4 }; long empty[] = {}; - testIncludeExcludeResults(includes, empty, new long[] { 1, 2, 3 }); - testIncludeExcludeResults(includes, excludes, new long[] { 1, 3 }); - testIncludeExcludeResults(empty, excludes, new long[] { 0, 1, 3 }); + testIncludeExcludeResults(1, includes, empty, new long[] { 1, 2, 3 }, new long[0]); + testIncludeExcludeResults(1, includes, excludes, new long[] { 1, 3 }, new long[0]); + testIncludeExcludeResults(1, empty, excludes, new long[] { 0, 1, 3 }, new long[0]); + + testIncludeExcludeResults(0, includes, empty, new long[] { 1, 2, 3}, new long[] { 98 }); + testIncludeExcludeResults(0, includes, excludes, new long[] { 1, 3 }, new long[] { 98 }); + testIncludeExcludeResults(0, empty, excludes, new long[] { 0, 1, 3 }, new long[] {5, 6, 7, 8, 9, 10, 11}); } - private void testIncludeExcludeResults(long[] includes, long[] excludes, long[] expecteds) { + private void testIncludeExcludeResults(int minDocCount, long[] includes, long[] excludes, + long[] expectedWithCounts, long[] expectedZeroCounts) { SearchResponse response = client().prepareSearch("idx").setTypes("type") .addAggregation(terms("terms") .field(SINGLE_VALUED_FIELD_NAME) .includeExclude(new IncludeExclude(includes, excludes)) - .collectMode(randomFrom(SubAggCollectionMode.values()))) + .collectMode(randomFrom(SubAggCollectionMode.values())) + .minDocCount(minDocCount)) .execute().actionGet(); assertSearchResponse(response); Terms terms = response.getAggregations().get("terms"); assertThat(terms, notNullValue()); assertThat(terms.getName(), equalTo("terms")); - assertThat(terms.getBuckets().size(), equalTo(expecteds.length)); + assertThat(terms.getBuckets().size(), equalTo(expectedWithCounts.length + expectedZeroCounts.length)); - for (int i = 0; i < expecteds.length; i++) { - Terms.Bucket bucket = terms.getBucketByKey("" + expecteds[i]); + for (int i = 0; i < expectedWithCounts.length; i++) { + Terms.Bucket bucket = terms.getBucketByKey("" + expectedWithCounts[i]); assertThat(bucket, notNullValue()); assertThat(bucket.getDocCount(), equalTo(1L)); } + + for (int i = 0; i < expectedZeroCounts.length; i++) { + Terms.Bucket bucket = terms.getBucketByKey("" + expectedZeroCounts[i]); + assertThat(bucket, notNullValue()); + assertThat(bucket.getDocCount(), equalTo(0L)); + } } - - - + + + public void testSingleValueFieldWithPartitionedFiltering() throws Exception { runTestFieldWithPartitionedFiltering(SINGLE_VALUED_FIELD_NAME); } - + public void testMultiValueFieldWithPartitionedFiltering() throws Exception { runTestFieldWithPartitionedFiltering(MULTI_VALUED_FIELD_NAME); } - + private void runTestFieldWithPartitionedFiltering(String field) throws Exception { // Find total number of unique terms SearchResponse allResponse = client().prepareSearch("idx").setTypes("type") @@ -348,8 +360,8 @@ private void runTestFieldWithPartitionedFiltering(String field) throws Exception Terms terms = allResponse.getAggregations().get("terms"); assertThat(terms, notNullValue()); assertThat(terms.getName(), equalTo("terms")); - int expectedCardinality = terms.getBuckets().size(); - + int expectedCardinality = terms.getBuckets().size(); + // Gather terms using partitioned aggregations final int numPartitions = randomIntBetween(2, 4); Set foundTerms = new HashSet<>(); @@ -368,9 +380,9 @@ private void runTestFieldWithPartitionedFiltering(String field) throws Exception foundTerms.add(bucket.getKeyAsNumber()); } } - assertEquals(expectedCardinality, foundTerms.size()); + assertEquals(expectedCardinality, foundTerms.size()); } - + public void testSingleValueFieldWithMaxSize() throws Exception { SearchResponse response = client().prepareSearch("idx").setTypes("high_card_type")