From 4bee8fecb68b7c2acade8a765e5aae96949bc1b0 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 27 Oct 2020 08:59:50 -0400 Subject: [PATCH] Make sure non-collecting aggs include sub-aggs Now that we're consistently using `cat_match` to filter which shards we run on we can get this confusing case: 1. You have a search with, say, a range and a sub-agg. 2. That search has a query that `can_match` can recognize will match no docs. On *any* shard. 3. So we dutifully run it on a single shard so it can produce the "empty" aggs. 4. The shard we pick happens to not have the target of the range mapped. 5. This kicks in the special range aggregator that doesn't collect any documents. 6. Before this commit, that range aggregator *also* never produced any sub-aggs. So, without this change, it was quite possible for a search that happened to match no documents to "throw away" the sub-aggs of a range and a few other aggs. We've had this problem for a long, long time but it is more confusing now because `can_match` is really kicking in and causing us to see cases where it looks like you are targeting a lot of shards but you really are only targeting a couple. It used to be that to get the "no sub-aggs" behavior you had to explicitly target only shards that didn't map the target field of the `range` agg. And, like, in that case it isn't too bad because you targeted a sort of degenerate shard. But now that `can_match` is doing its thing you can end up with the confusing steps above. It took me several hours to track down what what happening I know how the individual pieces of all of this works. It took four hours to figure out how they fit together in this case.... Anyway! This replaces all the aggregator implementations that throw out the sub-aggregators with ones that keep them. I think this'll be less confusing in the future. Closes #64142 --- .../ChildrenAggregatorFactory.java | 2 +- .../aggregations/ParentAggregatorFactory.java | 2 +- .../test/search.aggregation/40_range.yml | 36 +++++++++++++++++++ .../aggregations/NonCollectingAggregator.java | 8 ----- .../geogrid/GeoHashGridAggregatorFactory.java | 2 +- .../geogrid/GeoTileGridAggregatorFactory.java | 2 +- .../nested/NestedAggregatorFactory.java | 5 +-- .../ReverseNestedAggregatorFactory.java | 5 +-- .../range/AbstractRangeAggregatorFactory.java | 2 +- .../GeoDistanceRangeAggregatorFactory.java | 2 +- .../bucket/range/RangeAggregator.java | 18 +++++++--- .../SignificantTermsAggregatorFactory.java | 2 +- 12 files changed, 62 insertions(+), 24 deletions(-) diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenAggregatorFactory.java b/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenAggregatorFactory.java index 1e9f96f91c3f9..587e31564f5a7 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenAggregatorFactory.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenAggregatorFactory.java @@ -60,7 +60,7 @@ public ChildrenAggregatorFactory(String name, @Override protected Aggregator createUnmapped(SearchContext searchContext, Aggregator parent, Map metadata) throws IOException { - return new NonCollectingAggregator(name, searchContext, parent, metadata) { + return new NonCollectingAggregator(name, searchContext, parent, factories, metadata) { @Override public InternalAggregation buildEmptyAggregation() { return new InternalChildren(name, 0, buildEmptySubAggregations(), metadata()); diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentAggregatorFactory.java b/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentAggregatorFactory.java index 55fa2d8086883..a8be5c2b132b7 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentAggregatorFactory.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentAggregatorFactory.java @@ -60,7 +60,7 @@ public ParentAggregatorFactory(String name, @Override protected Aggregator createUnmapped(SearchContext searchContext, Aggregator parent, Map metadata) throws IOException { - return new NonCollectingAggregator(name, searchContext, parent, metadata) { + return new NonCollectingAggregator(name, searchContext, parent, factories, metadata) { @Override public InternalAggregation buildEmptyAggregation() { return new InternalParent(name, 0, buildEmptySubAggregations(), metadata()); diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml index aaedba3679fd2..de1053860bed8 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml @@ -384,3 +384,39 @@ setup: - match: { aggregations.age_groups.buckets.2.key: "Generation Y" } - match: { aggregations.age_groups.buckets.2.doc_count: 2 } + + +--- +"Date range unmapped with children": + - do: + indices.create: + index: test_a_unmapped + body: + settings: + number_of_shards: 1 + number_of_replicas: 0 + - do: + search: + index: test_a_unmapped + body: + size: 0 + query: + terms: + animal: [] + aggs: + date_range: + date_range: + field: date + ranges: + - from: 2020-01-01T00:00:00Z + aggs: + sounds: + cardinality: + field: sound.keyword + + - match: { hits.total.value: 0 } + - length: { aggregations.date_range.buckets: 1 } + - match: { aggregations.date_range.buckets.0.doc_count: 0 } + - match: { aggregations.date_range.buckets.0.key: "2020-01-01T00:00:00.000Z-*" } + - is_false: aggregations.date_range.buckets.0.to + - match: { aggregations.date_range.buckets.0.sounds.value: 0 } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/NonCollectingAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/NonCollectingAggregator.java index 39cd4259226bf..710767795bb14 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/NonCollectingAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/NonCollectingAggregator.java @@ -38,14 +38,6 @@ protected NonCollectingAggregator(String name, SearchContext context, Aggregator super(name, subFactories, context, parent, CardinalityUpperBound.NONE, metadata); } - /** - * Build a {@linkplain NonCollectingAggregator} for an aggregator without sub-aggregators. - */ - protected NonCollectingAggregator(String name, SearchContext context, Aggregator parent, - Map metadata) throws IOException { - this(name, context, parent, AggregatorFactories.EMPTY, metadata); - } - @Override public final LeafBucketCollector getLeafCollector(LeafReaderContext reader, LeafBucketCollector sub) { // the framework will automatically eliminate it diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java index a46be62fd6fc7..b81854b613963 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java @@ -63,7 +63,7 @@ protected Aggregator createUnmapped(SearchContext searchContext, Aggregator parent, Map metadata) throws IOException { final InternalAggregation aggregation = new InternalGeoHashGrid(name, requiredSize, emptyList(), metadata); - return new NonCollectingAggregator(name, searchContext, parent, metadata) { + return new NonCollectingAggregator(name, searchContext, parent, factories, metadata) { @Override public InternalAggregation buildEmptyAggregation() { return aggregation; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorFactory.java index ae82edda1eaa2..f62a33df59548 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorFactory.java @@ -61,7 +61,7 @@ protected Aggregator createUnmapped(SearchContext searchContext, Aggregator parent, Map metadata) throws IOException { final InternalAggregation aggregation = new InternalGeoTileGrid(name, requiredSize, Collections.emptyList(), metadata); - return new NonCollectingAggregator(name, searchContext, parent, metadata) { + return new NonCollectingAggregator(name, searchContext, parent, factories, metadata) { @Override public InternalAggregation buildEmptyAggregation() { return aggregation; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/nested/NestedAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/nested/NestedAggregatorFactory.java index a1709abee1fc4..b898a5e42b7bb 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/nested/NestedAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/nested/NestedAggregatorFactory.java @@ -51,7 +51,7 @@ public Aggregator createInternal(SearchContext searchContext, CardinalityUpperBound cardinality, Map metadata) throws IOException { if (childObjectMapper == null) { - return new Unmapped(name, searchContext, parent, metadata); + return new Unmapped(name, searchContext, parent, factories, metadata); } return new NestedAggregator(name, factories, parentObjectMapper, childObjectMapper, searchContext, parent, cardinality, metadata); @@ -62,8 +62,9 @@ private static final class Unmapped extends NonCollectingAggregator { Unmapped(String name, SearchContext context, Aggregator parent, + AggregatorFactories factories, Map metadata) throws IOException { - super(name, context, parent, metadata); + super(name, context, parent, factories, metadata); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/nested/ReverseNestedAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/nested/ReverseNestedAggregatorFactory.java index 5cf1c0db7aa1c..57a3c86a434e5 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/nested/ReverseNestedAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/nested/ReverseNestedAggregatorFactory.java @@ -52,7 +52,7 @@ public Aggregator createInternal(SearchContext searchContext, CardinalityUpperBound cardinality, Map metadata) throws IOException { if (unmapped) { - return new Unmapped(name, searchContext, parent, metadata); + return new Unmapped(name, searchContext, parent, factories, metadata); } else { return new ReverseNestedAggregator(name, factories, parentObjectMapper, searchContext, parent, cardinality, metadata); @@ -64,8 +64,9 @@ private static final class Unmapped extends NonCollectingAggregator { Unmapped(String name, SearchContext context, Aggregator parent, + AggregatorFactories factories, Map metadata) throws IOException { - super(name, context, parent, metadata); + super(name, context, parent, factories, metadata); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java index 0ed96c207ec69..6790cb5ee9147 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java @@ -76,7 +76,7 @@ public AbstractRangeAggregatorFactory(String name, protected Aggregator createUnmapped(SearchContext searchContext, Aggregator parent, Map metadata) throws IOException { - return new Unmapped<>(name, ranges, keyed, config.format(), searchContext, parent, rangeFactory, metadata); + return new Unmapped<>(name, factories, ranges, keyed, config.format(), searchContext, parent, rangeFactory, metadata); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/GeoDistanceRangeAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/GeoDistanceRangeAggregatorFactory.java index 83aa8cc1407c7..790ae59c66b31 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/GeoDistanceRangeAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/GeoDistanceRangeAggregatorFactory.java @@ -107,7 +107,7 @@ public GeoDistanceRangeAggregatorFactory(String name, ValuesSourceConfig config, protected Aggregator createUnmapped(SearchContext searchContext, Aggregator parent, Map metadata) throws IOException { - return new RangeAggregator.Unmapped<>(name, ranges, keyed, config.format(), searchContext, parent, + return new RangeAggregator.Unmapped<>(name, factories, ranges, keyed, config.format(), searchContext, parent, rangeFactory, metadata); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java index 23319116d1bda..82005aaaf40d4 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java @@ -353,11 +353,19 @@ public static class Unmapped extends NonCollect private final InternalRange.Factory factory; private final DocValueFormat format; - public Unmapped(String name, R[] ranges, boolean keyed, DocValueFormat format, SearchContext context, Aggregator parent, - InternalRange.Factory factory, Map metadata) - throws IOException { - - super(name, context, parent, metadata); + public Unmapped( + String name, + AggregatorFactories factories, + R[] ranges, + boolean keyed, + DocValueFormat format, + SearchContext context, + Aggregator parent, + InternalRange.Factory factory, + Map metadata + ) throws IOException { + + super(name, context, parent, factories, metadata); this.ranges = ranges; this.keyed = keyed; this.format = format; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregatorFactory.java index a2270bf3ed21b..abb6c1c9bd116 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregatorFactory.java @@ -186,7 +186,7 @@ protected Aggregator createUnmapped(SearchContext searchContext, Map metadata) throws IOException { final InternalAggregation aggregation = new UnmappedSignificantTerms(name, bucketCountThresholds.getRequiredSize(), bucketCountThresholds.getMinDocCount(), metadata); - return new NonCollectingAggregator(name, searchContext, parent, metadata) { + return new NonCollectingAggregator(name, searchContext, parent, factories, metadata) { @Override public InternalAggregation buildEmptyAggregation() { return aggregation;