Aggregations: the children agg didn't take deleted document into ac…

…count. The live docs that is passed down was ignored by the filter impl. Now the children filter gets wrapped with ApplyAcceptedDocsFilter, so live docs are actually applied. Closes elastic#8180
mute · Oct 22, 2014 · f522172 · f522172
1 parent bd26fd9
commit f522172
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 2 deletions.
diff --git a/...ava/org/elasticsearch/search/aggregations/bucket/children/ParentToChildrenAggregator.java b/...ava/org/elasticsearch/search/aggregations/bucket/children/ParentToChildrenAggregator.java
@@ -29,6 +29,7 @@
 import org.elasticsearch.common.lease.Releasables;
 import org.elasticsearch.common.lucene.ReaderContextAware;
 import org.elasticsearch.common.lucene.docset.DocIdSets;
+import org.elasticsearch.common.lucene.search.ApplyAcceptedDocsFilter;
 import org.elasticsearch.common.util.LongArray;
 import org.elasticsearch.common.util.LongObjectPagedHashMap;
 import org.elasticsearch.index.cache.fixedbitset.FixedBitSetFilter;
@@ -52,7 +53,7 @@
 public class ParentToChildrenAggregator extends SingleBucketAggregator implements ReaderContextAware {
 
     private final String parentType;
-    private final FixedBitSetFilter childFilter;
+    private final Filter childFilter;
     private final FixedBitSetFilter parentFilter;
     private final ValuesSource.Bytes.WithOrdinals.ParentChild valuesSource;
 
@@ -75,7 +76,10 @@ public ParentToChildrenAggregator(String name, AggregatorFactories factories, Ag
                                       ValuesSource.Bytes.WithOrdinals.ParentChild valuesSource, long maxOrd) {
         super(name, factories, aggregationContext, parent);
         this.parentType = parentType;
-        this.childFilter = aggregationContext.searchContext().fixedBitSetFilterCache().getFixedBitSetFilter(childFilter);
+        // The child filter doesn't rely on random access it just used to iterate over all docs with a specific type,
+        // so use the filter cache instead. When the filter cache is smarter with what filter impl to pick we can benefit
+        // from it here
+        this.childFilter = new ApplyAcceptedDocsFilter(aggregationContext.searchContext().filterCache().cache(childFilter));
         this.parentFilter = aggregationContext.searchContext().fixedBitSetFilterCache().getFixedBitSetFilter(parentFilter);
         this.parentOrdToBuckets = aggregationContext.bigArrays().newLongArray(maxOrd, false);
         this.parentOrdToBuckets.fill(0, maxOrd, -1);

diff --git a/src/test/java/org/elasticsearch/search/aggregations/bucket/ChildrenTests.java b/src/test/java/org/elasticsearch/search/aggregations/bucket/ChildrenTests.java
@@ -20,9 +20,11 @@
 
 import org.elasticsearch.action.index.IndexRequestBuilder;
 import org.elasticsearch.action.search.SearchResponse;
+import org.elasticsearch.action.update.UpdateResponse;
 import org.elasticsearch.search.SearchHit;
 import org.elasticsearch.search.aggregations.bucket.children.Children;
 import org.elasticsearch.search.aggregations.bucket.terms.Terms;
+import org.elasticsearch.search.aggregations.metrics.sum.Sum;
 import org.elasticsearch.search.aggregations.metrics.tophits.TopHits;
 import org.elasticsearch.search.sort.SortOrder;
 import org.elasticsearch.test.ElasticsearchIntegrationTest;
@@ -33,8 +35,10 @@
 import static org.elasticsearch.index.query.QueryBuilders.matchQuery;
 import static org.elasticsearch.search.aggregations.AggregationBuilders.*;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.is;
 
 /**
@@ -215,6 +219,45 @@ public void testParentWithMultipleBuckets() throws Exception {
         assertThat(topHits.getHits().getAt(0).getType(), equalTo("comment"));
     }
 
+    @Test
+    public void testWithDeletes() throws Exception {
+        String indexName = "xyz";
+        assertAcked(
+                prepareCreate(indexName)
+                        .addMapping("parent")
+                        .addMapping("child", "_parent", "type=parent", "count", "type=long")
+        );
+
+        List<IndexRequestBuilder> requests = new ArrayList<>();
+        requests.add(client().prepareIndex(indexName, "parent", "1").setSource("{}"));
+        requests.add(client().prepareIndex(indexName, "child", "0").setParent("1").setSource("count", 1));
+        requests.add(client().prepareIndex(indexName, "child", "1").setParent("1").setSource("count", 1));
+        requests.add(client().prepareIndex(indexName, "child", "2").setParent("1").setSource("count", 1));
+        requests.add(client().prepareIndex(indexName, "child", "3").setParent("1").setSource("count", 1));
+        indexRandom(true, requests);
+
+        for (int i = 0; i < 10; i++) {
+            SearchResponse searchResponse = client().prepareSearch(indexName)
+                    .addAggregation(children("children").childType("child").subAggregation(sum("counts").field("count")))
+                    .get();
+
+            assertNoFailures(searchResponse);
+            Children children = searchResponse.getAggregations().get("children");
+            assertThat(children.getDocCount(), equalTo(4l));
+
+            Sum count = children.getAggregations().get("counts");
+            assertThat(count.getValue(), equalTo(4.));
+
+            String idToUpdate = Integer.toString(randomInt(3));
+            UpdateResponse updateResponse = client().prepareUpdate(indexName, "child", idToUpdate)
+                    .setParent("1")
+                    .setDoc("count", 1)
+                    .get();
+            assertThat(updateResponse.getVersion(), greaterThan(1l));
+            refresh();
+        }
+    }
+
     private static final class Control {
 
         final String category;