From 3aa0c803f2326fc0535643c474f1a750934bad7c Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 27 Oct 2020 15:45:24 -0400 Subject: [PATCH 1/3] 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 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 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 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 d5d9e7df98cbb..16f6ecba9c0bc 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 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 4652c75f8118b..a0cac5a388fb4 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; From a7fc0d3ce462edd85ed2b6376bf20043014b82e4 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 27 Oct 2020 16:37:51 -0400 Subject: [PATCH 2/3] hush bwc tests Backport is coming. Please wait. --- .../rest-api-spec/test/search.aggregation/40_range.yml | 4 ++++ 1 file changed, 4 insertions(+) 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 4c55dcdc006d0..6b4b16e62807f 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 @@ -391,6 +391,10 @@ setup: --- "Date range unmapped with children": + - skip: + version: " - 7.10.99" + reason: Fixed in 7.11.0 to be backported to 7.10.0 + - do: indices.create: index: test_a_unmapped From 4b1a03133f99214664f11a91e8f20582482132e1 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 28 Oct 2020 08:40:57 -0400 Subject: [PATCH 3/3] Update fix after backport --- .../rest-api-spec/test/search.aggregation/40_range.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 6b4b16e62807f..a050b4c5bbf11 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 @@ -392,8 +392,8 @@ setup: --- "Date range unmapped with children": - skip: - version: " - 7.10.99" - reason: Fixed in 7.11.0 to be backported to 7.10.0 + version: " - 7.9.99" + reason: Fixed in 7.10.0 - do: indices.create: