Skip to content

Commit 2ba9e36

Browse files
authored
Add helper classes to determine if aggs have a value (#36020)
This adds a set of helper classes to determine if an agg "has a value". This is needed because InternalAggs represent "empty" in different manners according to convention. Some use `NaN`, `+/- Inf`, `0.0`, etc. A user can pass the Internal agg type to one of these helper methods and it will report if the agg contains a value or not, which allows the user to differentiate "empty" from a real `NaN`. These helpers are best-effort in some cases. For example, several pipeline aggs share a single return class but use different conventions to mark "empty", so the helper uses the loosest definition that applies to all the aggs that use the class. Sums in particular are unreliable. The InternalSum simply returns 0.0 if the agg is empty (which is correct, no values == sum of zero). But this also means the helper cannot differentiate from "empty" and `+1 + -1`.
1 parent 715719e commit 2ba9e36

File tree

49 files changed

+777
-98
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

49 files changed

+777
-98
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.elasticsearch.search.aggregations.matrix.stats;
20+
21+
/**
22+
* Counterpart to {@link org.elasticsearch.search.aggregations.support.AggregationInspectionHelper}, providing
23+
* helpers for some aggs in the MatrixStats package
24+
*/
25+
public class MatrixAggregationInspectionHelper {
26+
public static boolean hasValue(InternalMatrixStats agg) {
27+
return agg.getResults() != null;
28+
}
29+
}

modules/aggs-matrix-stats/src/test/java/org/elasticsearch/search/aggregations/matrix/stats/MatrixStatsAggregatorTests.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,12 @@ public void testNoData() throws Exception {
5353
.fields(Collections.singletonList("field"));
5454
InternalMatrixStats stats = search(searcher, new MatchAllDocsQuery(), aggBuilder, ft);
5555
assertNull(stats.getStats());
56+
assertFalse(MatrixAggregationInspectionHelper.hasValue(stats));
5657
}
5758
}
5859
}
5960

61+
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/37587")
6062
public void testTwoFields() throws Exception {
6163
String fieldA = "a";
6264
MappedFieldType ftA = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.DOUBLE);
@@ -87,8 +89,9 @@ public void testTwoFields() throws Exception {
8789
IndexSearcher searcher = new IndexSearcher(reader);
8890
MatrixStatsAggregationBuilder aggBuilder = new MatrixStatsAggregationBuilder("my_agg")
8991
.fields(Arrays.asList(fieldA, fieldB));
90-
InternalMatrixStats stats = search(searcher, new MatchAllDocsQuery(), aggBuilder, ftA, ftB);
92+
InternalMatrixStats stats = searchAndReduce(searcher, new MatchAllDocsQuery(), aggBuilder, ftA, ftB);
9193
multiPassStats.assertNearlyEqual(new MatrixStatsResults(stats.getStats()));
94+
assertTrue(MatrixAggregationInspectionHelper.hasValue(stats));
9295
}
9396
}
9497
}
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.elasticsearch.join.aggregations;
20+
21+
/**
22+
* Counterpart to {@link org.elasticsearch.search.aggregations.support.AggregationInspectionHelper}, providing
23+
* helpers for some aggs in the Join package
24+
*/
25+
public class JoinAggregationInspectionHelper {
26+
27+
public static boolean hasValue(InternalParent agg) {
28+
return agg.getDocCount() > 0;
29+
}
30+
31+
public static boolean hasValue(InternalChildren agg) {
32+
return agg.getDocCount() > 0;
33+
}
34+
}

modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenToParentAggregatorTests.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ public void testNoDocs() throws IOException {
9090
assertEquals(0, childrenToParent.getDocCount());
9191
assertNotNull("Aggregations: " + childrenToParent.getAggregations().asMap(), parentAggregation);
9292
assertEquals(Double.POSITIVE_INFINITY, ((InternalMin) parentAggregation).getValue(), Double.MIN_VALUE);
93+
assertFalse(JoinAggregationInspectionHelper.hasValue(childrenToParent));
9394
});
9495
indexReader.close();
9596
directory.close();
@@ -119,6 +120,7 @@ public void testParentChild() throws IOException {
119120
parent.getAggregations().asMap(),
120121
expectedTotalParents, parent.getDocCount());
121122
assertEquals(expectedMinValue, ((InternalMin) parent.getAggregations().get("in_parent")).getValue(), Double.MIN_VALUE);
123+
assertTrue(JoinAggregationInspectionHelper.hasValue(parent));
122124
});
123125

124126
// verify for each children
@@ -170,6 +172,7 @@ public void testParentChildTerms() throws IOException {
170172
// verify a terms-aggregation inside the parent-aggregation
171173
testCaseTerms(new MatchAllDocsQuery(), indexSearcher, parent -> {
172174
assertNotNull(parent);
175+
assertTrue(JoinAggregationInspectionHelper.hasValue(parent));
173176
LongTerms valueTerms = parent.getAggregations().get("value_terms");
174177
assertNotNull(valueTerms);
175178

modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ public void testParentChild() throws IOException {
105105
expectedMinValue = Math.min(expectedMinValue, expectedValues.v2());
106106
}
107107
assertEquals(expectedTotalChildren, child.getDocCount());
108+
assertTrue(JoinAggregationInspectionHelper.hasValue(child));
108109
assertEquals(expectedMinValue, ((InternalMin) child.getAggregations().get("in_child")).getValue(), Double.MIN_VALUE);
109110
});
110111

server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/InternalComposite.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ InternalBucket next() {
226226
}
227227
}
228228

229-
static class InternalBucket extends InternalMultiBucketAggregation.InternalBucket
229+
public static class InternalBucket extends InternalMultiBucketAggregation.InternalBucket
230230
implements CompositeAggregation.Bucket, KeyComparable<InternalBucket> {
231231

232232
private final CompositeKey key;

server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/InternalFilter.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,4 +50,4 @@ public String getWriteableName() {
5050
protected InternalSingleBucketAggregation newAggregation(String name, long docCount, InternalAggregations subAggregations) {
5151
return new InternalFilter(name, docCount, subAggregations, pipelineAggregators(), getMetaData());
5252
}
53-
}
53+
}

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridBucket.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@
3434
import java.util.List;
3535
import java.util.Objects;
3636

37-
class GeoGridBucket extends InternalMultiBucketAggregation.InternalBucket implements GeoHashGrid.Bucket, Comparable<GeoGridBucket> {
37+
public class GeoGridBucket extends InternalMultiBucketAggregation.InternalBucket implements GeoHashGrid.Bucket, Comparable<GeoGridBucket> {
3838

3939
protected long geohashAsLong;
4040
protected long docCount;

server/src/main/java/org/elasticsearch/search/aggregations/bucket/sampler/InternalSampler.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,4 +59,4 @@ protected InternalSingleBucketAggregation newAggregation(String name, long docCo
5959
InternalAggregations subAggregations) {
6060
return new InternalSampler(name, docCount, subAggregations, pipelineAggregators(), metaData);
6161
}
62-
}
62+
}

server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractInternalHDRPercentiles.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,10 @@ public long getEstimatedMemoryFootprint() {
9898
return state.getEstimatedFootprintInBytes();
9999
}
100100

101+
DoubleHistogram getState() {
102+
return state;
103+
}
104+
101105
@Override
102106
public AbstractInternalHDRPercentiles doReduce(List<InternalAggregation> aggregations, ReduceContext reduceContext) {
103107
DoubleHistogram merged = null;

0 commit comments

Comments
 (0)