Add size parameter to time_series aggregation #93496
Conversation
Adds an optional size parameter which caps the number of buckets in responses.
elasticsearchmachine
added
v8.7.0
needs:triage
tmgordeeva
added
Team:Analytics
elasticsearchmachine
added
needs:triage
tmgordeeva
added
Team:Analytics
elasticsearchmachine
added
needs:triage
| @@ -247,6 +251,9 @@ protected boolean lessThan(IteratorAndCurrent<InternalBucket> a, IteratorAndCurr | |||
| BytesRef tsid = reducedBucket.key; | |||
| assert prevTsid == null || tsid.compareTo(prevTsid) > 0; | |||
| reduced.buckets.add(reducedBucket); | |||
| if (size != null && ++count >= size) { | |||
There was a problem hiding this comment.
I think reduced.size() can be used instead of using a count variable here?
| public static final InstantiatingObjectParser<TimeSeriesAggregationBuilder, String> PARSER; | ||
|
|
||
| private boolean keyed; | ||
| private int size; | ||
|
|
||
| private static final int DEFAULT_SIZE = 10000; |
There was a problem hiding this comment.
This should be: private static final int DEFAULT_SIZE = MultiBucketConsumerService.DEFAULT_MAX_BUCKETS
| @@ -14,7 +14,7 @@ public class TimeSeriesAggregationBuilderTests extends AggregationBuilderTestCas | |||
|
|
|||
| @Override | |||
| protected TimeSeriesAggregationBuilder createTestAggregatorBuilder() { | |||
| return new TimeSeriesAggregationBuilder(randomAlphaOfLength(10), randomBoolean()); | |||
| return new TimeSeriesAggregationBuilder(randomAlphaOfLength(10), randomBoolean(), 10000); | |||
There was a problem hiding this comment.
Maybe randomise size here? randomIntBetween(0, 100_000)
modules/aggregations/src/yamlRestTest/resources/rest-api-spec/test/aggregations/time_series.yml
Show resolved
Hide resolved
| @@ -217,6 +217,10 @@ protected boolean lessThan(IteratorAndCurrent<InternalBucket> a, IteratorAndCurr | |||
| } | |||
|
|
|||
| InternalTimeSeries reduced = new InternalTimeSeries(name, new ArrayList<>(initialCapacity), keyed, getMetadata()); | |||
| Integer size = reduceContext.builder() instanceof TimeSeriesAggregationBuilder | |||
There was a problem hiding this comment.
+1 then we don't have to serialise size as part of InternalTimeSeries class.
Is reduceContext.builder() instanceof TimeSeriesAggregationBuilder also ways true?
If so we should throw an IllagalStateException if the builder isn't instance of TimeSeriesAggregationBuilder.
If this isn't the case then we should default to TimeSeriesAggregationBuilder#DEFAULT_SIZE.
I think size variable can just be an int?
There was a problem hiding this comment.
tests may use a fake builder
I was unaware of this. So I think using Integer as type here is ok.
| @@ -66,6 +70,9 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I | |||
| ); | |||
| bucket.bucketOrd = ordsEnum.ord(); | |||
| buckets.add(bucket); | |||
| if (++count >= size) { | |||
There was a problem hiding this comment.
I think buckets.size() also works here instead of count variable?
tmgordeeva
added
the
Team:Analytics
elasticsearchmachine
removed
the
Team:Analytics
tmgordeeva
added
:Analytics/Geo
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
Hi @tmgordeeva, I've created a changelog YAML for you. |
tmgordeeva
added
:StorageEngine/TSDB
|
Hi @tmgordeeva, I've updated the changelog YAML for you. |
martijnvg
changed the title
Adds an optional size parameter which caps the number of buckets in responses.
After aggregation, caps the number of buckets we send back in responses and in reduction.