Skip to content

Commit 10e2cc3

Browse files
original-brownbearoriginal-brownbear
authored
Dry up Bucket types in o.e.search.aggregations.bucket.histogram (elastic#110303)
It's in the title, lots of duplication here that deserves cleanup in isolation. Also, bucket instances are a perpetual source of memory consuption in aggs. There are lots of possible improvements we can make to reduce their footprint, drying up this code enables cleaner PRs for these improvements.
1 parent 99280b4 commit 10e2cc3

File tree

16 files changed

+172
-227
lines changed

16 files changed

+172
-227
lines changed

‎modules/aggregations/src/internalClusterTest/java/org/elasticsearch/aggregations/pipeline/DateDerivativeIT.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,9 @@
1515
import org.elasticsearch.plugins.Plugin;
1616
import org.elasticsearch.search.aggregations.InternalAggregation;
1717
import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation;
18+
import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation.Bucket;
1819
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval;
1920
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
20-
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram.Bucket;
2121
import org.elasticsearch.search.aggregations.metrics.Sum;
2222
import org.elasticsearch.search.aggregations.pipeline.SimpleValue;
2323
import org.elasticsearch.search.aggregations.support.AggregationPath;
@@ -127,7 +127,7 @@ public void testSingleValuedField() throws Exception {
127127
assertThat(buckets.size(), equalTo(3));
128128

129129
ZonedDateTime key = ZonedDateTime.of(2012, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
130-
Histogram.Bucket bucket = buckets.get(0);
130+
Bucket bucket = buckets.get(0);
131131
assertThat(bucket, notNullValue());
132132
assertThat((ZonedDateTime) bucket.getKey(), equalTo(key));
133133
assertThat(bucket.getDocCount(), equalTo(1L));
@@ -171,7 +171,7 @@ public void testSingleValuedFieldNormalised() throws Exception {
171171
assertThat(buckets.size(), equalTo(3));
172172

173173
ZonedDateTime key = ZonedDateTime.of(2012, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
174-
Histogram.Bucket bucket = buckets.get(0);
174+
Bucket bucket = buckets.get(0);
175175
assertThat(bucket, notNullValue());
176176
assertThat(bucket.getKey(), equalTo(key));
177177
assertThat(bucket.getDocCount(), equalTo(1L));
@@ -383,7 +383,7 @@ private static void addNTimes(int amount, String index, ZonedDateTime dateTime,
383383
}
384384

385385
private static void assertBucket(
386-
Histogram.Bucket bucket,
386+
Bucket bucket,
387387
ZonedDateTime expectedKey,
388388
long expectedDocCount,
389389
Matcher<Object> derivativeMatcher,
@@ -421,7 +421,7 @@ public void testSingleValuedFieldWithSubAggregation() throws Exception {
421421
Object[] propertiesCounts = (Object[]) ((InternalAggregation) histo).getProperty("sum.value");
422422

423423
ZonedDateTime key = ZonedDateTime.of(2012, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
424-
Histogram.Bucket bucket = buckets.get(0);
424+
Bucket bucket = buckets.get(0);
425425
assertThat(bucket, notNullValue());
426426
assertThat((ZonedDateTime) bucket.getKey(), equalTo(key));
427427
assertThat(bucket.getDocCount(), equalTo(1L));
@@ -500,7 +500,7 @@ public void testMultiValuedField() throws Exception {
500500
assertThat(buckets.size(), equalTo(4));
501501

502502
ZonedDateTime key = ZonedDateTime.of(2012, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
503-
Histogram.Bucket bucket = buckets.get(0);
503+
Bucket bucket = buckets.get(0);
504504
assertThat(bucket, notNullValue());
505505
assertThat((ZonedDateTime) bucket.getKey(), equalTo(key));
506506
assertThat(bucket.getDocCount(), equalTo(1L));
@@ -574,7 +574,7 @@ public void testPartiallyUnmapped() throws Exception {
574574
assertThat(buckets.size(), equalTo(3));
575575

576576
ZonedDateTime key = ZonedDateTime.of(2012, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
577-
Histogram.Bucket bucket = buckets.get(0);
577+
Bucket bucket = buckets.get(0);
578578
assertThat(bucket, notNullValue());
579579
assertThat((ZonedDateTime) bucket.getKey(), equalTo(key));
580580
assertThat(bucket.getDocCount(), equalTo(1L));

‎modules/aggregations/src/internalClusterTest/java/org/elasticsearch/aggregations/pipeline/SerialDiffIT.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@
1212
import org.elasticsearch.aggregations.AggregationIntegTestCase;
1313
import org.elasticsearch.common.collect.EvictingQueue;
1414
import org.elasticsearch.common.util.Maps;
15+
import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation.Bucket;
1516
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
16-
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram.Bucket;
1717
import org.elasticsearch.search.aggregations.pipeline.BucketHelpers;
1818
import org.elasticsearch.search.aggregations.pipeline.SimpleValue;
1919
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
@@ -91,7 +91,7 @@ private void assertValidIterators(Iterator<?> expectedBucketIter, Iterator<?> ex
9191
}
9292
}
9393

94-
private void assertBucketContents(Histogram.Bucket actual, Double expectedCount, Double expectedValue) {
94+
private void assertBucketContents(Bucket actual, Double expectedCount, Double expectedValue) {
9595
// This is a gap bucket
9696
SimpleValue countDiff = actual.getAggregations().get("diff_counts");
9797
if (expectedCount == null) {
@@ -239,15 +239,15 @@ public void testBasicDiff() {
239239
List<Double> expectedCounts = testValues.get(MetricTarget.COUNT.toString());
240240
List<Double> expectedValues = testValues.get(MetricTarget.VALUE.toString());
241241

242-
Iterator<? extends Histogram.Bucket> actualIter = buckets.iterator();
242+
Iterator<? extends Bucket> actualIter = buckets.iterator();
243243
Iterator<PipelineAggregationHelperTests.MockBucket> expectedBucketIter = mockHisto.iterator();
244244
Iterator<Double> expectedCountsIter = expectedCounts.iterator();
245245
Iterator<Double> expectedValuesIter = expectedValues.iterator();
246246

247247
while (actualIter.hasNext()) {
248248
assertValidIterators(expectedBucketIter, expectedCountsIter, expectedValuesIter);
249249

250-
Histogram.Bucket actual = actualIter.next();
250+
Bucket actual = actualIter.next();
251251
PipelineAggregationHelperTests.MockBucket expected = expectedBucketIter.next();
252252
Double expectedCount = expectedCountsIter.next();
253253
Double expectedValue = expectedValuesIter.next();

‎modules/aggregations/src/main/java/org/elasticsearch/aggregations/bucket/histogram/InternalAutoDateHistogram.java‎

Lines changed: 11 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import org.elasticsearch.search.aggregations.bucket.BucketReducer;
2424
import org.elasticsearch.search.aggregations.bucket.IteratorAndCurrent;
2525
import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation;
26+
import org.elasticsearch.search.aggregations.bucket.histogram.AbstractHistogramBucket;
2627
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval;
2728
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
2829
import org.elasticsearch.search.aggregations.bucket.histogram.HistogramFactory;
@@ -48,28 +49,20 @@ public final class InternalAutoDateHistogram extends InternalMultiBucketAggregat
4849
InternalAutoDateHistogram,
4950
InternalAutoDateHistogram.Bucket> implements Histogram, HistogramFactory {
5051

51-
public static class Bucket extends InternalMultiBucketAggregation.InternalBucket implements Histogram.Bucket, KeyComparable<Bucket> {
52+
public static class Bucket extends AbstractHistogramBucket implements KeyComparable<Bucket> {
5253

5354
final long key;
54-
final long docCount;
55-
final InternalAggregations aggregations;
56-
protected final transient DocValueFormat format;
5755

5856
public Bucket(long key, long docCount, DocValueFormat format, InternalAggregations aggregations) {
59-
this.format = format;
57+
super(docCount, aggregations, format);
6058
this.key = key;
61-
this.docCount = docCount;
62-
this.aggregations = aggregations;
6359
}
6460

6561
/**
6662
* Read from a stream.
6763
*/
68-
public Bucket(StreamInput in, DocValueFormat format) throws IOException {
69-
this.format = format;
70-
key = in.readLong();
71-
docCount = in.readVLong();
72-
aggregations = InternalAggregations.readFrom(in);
64+
public static Bucket readFrom(StreamInput in, DocValueFormat format) throws IOException {
65+
return new Bucket(in.readLong(), in.readVLong(), format, InternalAggregations.readFrom(in));
7366
}
7467

7568
@Override
@@ -105,16 +98,6 @@ public Object getKey() {
10598
return Instant.ofEpochMilli(key).atZone(ZoneOffset.UTC);
10699
}
107100

108-
@Override
109-
public long getDocCount() {
110-
return docCount;
111-
}
112-
113-
@Override
114-
public InternalAggregations getAggregations() {
115-
return aggregations;
116-
}
117-
118101
@Override
119102
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
120103
String keyAsString = format.format(key).toString();
@@ -222,7 +205,7 @@ public InternalAutoDateHistogram(StreamInput in) throws IOException {
222205
super(in);
223206
bucketInfo = new BucketInfo(in);
224207
format = in.readNamedWriteable(DocValueFormat.class);
225-
buckets = in.readCollectionAsList(stream -> new Bucket(stream, format));
208+
buckets = in.readCollectionAsList(stream -> Bucket.readFrom(stream, format));
226209
this.targetBuckets = in.readVInt();
227210
if (in.getTransportVersion().onOrAfter(TransportVersions.V_8_3_0)) {
228211
bucketInnerInterval = in.readVLong();
@@ -286,7 +269,7 @@ public InternalAutoDateHistogram create(List<Bucket> buckets) {
286269

287270
@Override
288271
public Bucket createBucket(InternalAggregations aggregations, Bucket prototype) {
289-
return new Bucket(prototype.key, prototype.docCount, prototype.format, aggregations);
272+
return new Bucket(prototype.key, prototype.getDocCount(), prototype.getFormatter(), aggregations);
290273
}
291274

292275
/**
@@ -376,14 +359,14 @@ private List<Bucket> mergeBuckets(
376359
long roundedBucketKey = reduceRounding.round(bucket.key);
377360
if (Double.isNaN(key)) {
378361
key = roundedBucketKey;
379-
sameKeyedBuckets.add(createBucket(key, bucket.docCount, bucket.aggregations));
362+
sameKeyedBuckets.add(createBucket(key, bucket.getDocCount(), bucket.getAggregations()));
380363
} else if (roundedBucketKey == key) {
381-
sameKeyedBuckets.add(createBucket(key, bucket.docCount, bucket.aggregations));
364+
sameKeyedBuckets.add(createBucket(key, bucket.getDocCount(), bucket.getAggregations()));
382365
} else {
383366
mergedBuckets.add(reduceBucket(sameKeyedBuckets, reduceContext));
384367
sameKeyedBuckets.clear();
385368
key = roundedBucketKey;
386-
sameKeyedBuckets.add(createBucket(key, bucket.docCount, bucket.aggregations));
369+
sameKeyedBuckets.add(createBucket(key, bucket.getDocCount(), bucket.getAggregations()));
387370
}
388371
}
389372
if (sameKeyedBuckets.isEmpty() == false) {
@@ -594,7 +577,7 @@ private BucketReduceResult mergeConsecutiveBuckets(
594577
sameKeyedBuckets.clear();
595578
key = current.preparedRounding.round(bucket.key);
596579
}
597-
sameKeyedBuckets.add(new Bucket(Math.round(key), bucket.docCount, format, bucket.aggregations));
580+
sameKeyedBuckets.add(new Bucket(Math.round(key), bucket.getDocCount(), format, bucket.getAggregations()));
598581
}
599582
if (sameKeyedBuckets.isEmpty() == false) {
600583
mergedBuckets.add(reduceBucket(sameKeyedBuckets, reduceContext));

‎modules/aggregations/src/test/java/org/elasticsearch/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -501,7 +501,7 @@ public void testReadFromPre830() throws IOException {
501501
assertEquals(1, deserialized.getBuckets().size());
502502
InternalAutoDateHistogram.Bucket bucket = deserialized.getBuckets().iterator().next();
503503
assertEquals(10, bucket.key);
504-
assertEquals(100, bucket.docCount);
504+
assertEquals(100, bucket.getDocCount());
505505
}
506506
}
507507
}

‎server/src/internalClusterTest/java/org/elasticsearch/indices/IndicesRequestCacheIT.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,10 @@
1616
import org.elasticsearch.common.time.DateFormatter;
1717
import org.elasticsearch.index.cache.request.RequestCacheStats;
1818
import org.elasticsearch.index.query.QueryBuilders;
19+
import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation.Bucket;
1920
import org.elasticsearch.search.aggregations.bucket.global.GlobalAggregationBuilder;
2021
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval;
2122
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
22-
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram.Bucket;
2323
import org.elasticsearch.test.ESIntegTestCase;
2424
import org.elasticsearch.test.hamcrest.ElasticsearchAssertions;
2525

0 commit comments

Comments
 (0)