Add moving percentiles pipeline aggregation #55441
Conversation
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
There was a problem hiding this comment.
Left some comments!
@nik9000, wanna give this some eyeballs too since you've been elbow-deep in pipelines lately?
| if (bucketsPaths.length != 1) { | ||
| context.addBucketPathValidationError("must contain a single entry for aggregation [" + name + "]"); | ||
| } | ||
| context.validateParentAggSequentiallyOrdered(NAME, name); |
There was a problem hiding this comment.
I wonder if @nik9000 has ideas how we could validate that the pipeline targets a percentiles agg up front in the builder, instead of down below during runtime? Might not be possible yet...
There was a problem hiding this comment.
I think it may be worth adding something near bucketCardinality that returns the type of buckets. That'd let us do this ordered assertion without instanceofs. I'd be happy to take a stab at it in a follow up because I have ideas. No need to delay this PR for it though.
...org/elasticsearch/xpack/analytics/movingPercentiles/MovingPercentilesPipelineAggregator.java
Show resolved
Hide resolved
...org/elasticsearch/xpack/analytics/movingPercentiles/MovingPercentilesPipelineAggregator.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private void reduceHDR(List<? extends InternalMultiBucketAggregation.InternalBucket> buckets, |
There was a problem hiding this comment.
Shame there isn't a way to share this code with tdigest since it's mainly the same. Not sure it's worth trying to figure out though, unless someone has a simple/clever solution :)
There was a problem hiding this comment.
I must say I started that way, trying to wrap the different sketches in a generic object but I gave up as it was actually much harder and at some point you need to know which method you are using.
There was a problem hiding this comment.
Gotcha 👍 Yeah the more I looked at it, I came to a similar conclusion. Oh well :)
...org/elasticsearch/xpack/analytics/movingPercentiles/MovingPercentilesPipelineAggregator.java
Outdated
Show resolved
Hide resolved
| currentAggName = aggPathsList.get(0); | ||
| } | ||
|
|
||
| throw new AggregationExecutionException(AbstractPipelineAggregationBuilder.BUCKETS_PATH_FIELD.getPreferredName() |
There was a problem hiding this comment.
Let's change this to an IllegalArgumentException, since technically it's the client's fault for pointing us at the wrong thing :)
...org/elasticsearch/xpack/analytics/movingPercentiles/MovingPercentilesPipelineAggregator.java
Show resolved
Hide resolved
x-pack/plugin/src/test/resources/rest-api-spec/test/analytics/moving_percentile.yml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I left some comments about the pipeline stuff - I think this should spawn two follow up PRs, one for plumbing the parent builder to the ctor and one for better validation. I can't really speak to the percentiles stuff without doing a bunch of background reading.
| @@ -96,10 +96,18 @@ public long getEstimatedMemoryFootprint() { | |||
| return state.getEstimatedFootprintInBytes(); | |||
| } | |||
|
|
|||
| DoubleHistogram getState() { | |||
There was a problem hiding this comment.
Would you kindly add javadoc to these while you are here?
| if (bucketsPaths.length != 1) { | ||
| context.addBucketPathValidationError("must contain a single entry for aggregation [" + name + "]"); | ||
| } | ||
| context.validateParentAggSequentiallyOrdered(NAME, name); |
There was a problem hiding this comment.
I think it may be worth adding something near bucketCardinality that returns the type of buckets. That'd let us do this ordered assertion without instanceofs. I'd be happy to take a stab at it in a follow up because I have ideas. No need to delay this PR for it though.
| return index; | ||
| } | ||
|
|
||
| // TODO: replace this with the PercentilesConfig that's used by the percentiles builder. |
There was a problem hiding this comment.
I think it'd be a fairly small change to plumb the parent builder into the ctor of the pipeline aggregator. A fine thing to do in a follow up though because it'd touch a bunch of file mechanically.
rjernst
added
the
Team:Analytics
Relates: elastic/elasticsearch#55441 Co-authored-by: Russ Cam <[email protected]>
Relates: elastic/elasticsearch#55441 Co-authored-by: Russ Cam <[email protected]>
Similar to what the moving function aggregation does, except merging windows of percentiles sketches together instead of cumulatively merging final metrics.
closes #49452