From 39dba215231810bef15cf08cd62e1c773227afef Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 27 Oct 2020 15:45:24 -0400 Subject: [PATCH] Make sure non-collecting aggs include sub-aggs (backport of #64214) 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 65378876c3ee8..08e2ceed3ac7b 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 503d8e6763040..ce590c263fb84 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 fa87f584c80c3..4c55dcdc006d0 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 @@ -387,3 +387,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 515b5ad07b65a..d3d8157f24279 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 6ec5d8671a92d..31c304528ba99 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 1f573f70480dc..ae784179dcde2 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 60c1a21a90aa3..ad22c05c7bea1 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 c2839441bccef..ca189819a4860 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 @@ -75,7 +75,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 60a2cd19cd5cd..d61ed62f1e0b0 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 fd19af079b27d..ed73f17e2d63f 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;