Update bucket metric pipeline agg paths to allow intermediate single bucket and bucket qualified multi-bucket aggs #85729
Conversation
…bucket and bucket qualified multi-bucket aggs
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
|
Hi @benwtrent, I've created a changelog YAML for you. |
…buckets-path-resolver
…enwtrent/elasticsearch into feature/refactor-buckets-path-resolver
| (aggBuilder.get().bucketCardinality() == AggregationBuilder.BucketCardinality.MANY | ||
| && AggregationPath.pathElementContainsBucketKey(currentAgg)) | ||
| // Is this a single bucket agg with sub-aggs? | ||
| || (aggBuilder.get().bucketCardinality() != AggregationBuilder.BucketCardinality.MANY |
There was a problem hiding this comment.
Shouldn't this branch check if it's exactly BucketCardinality.ONE? We can't path further into an agg with BucketCardinality.NONE, unless I'm missing something?
| // Dig through the aggregation tree to find the first aggregation specified by the path. | ||
| // The path may have many single bucket aggs (with sub-aggs) or many multi-bucket aggs specified by bucket keys | ||
| while (aggBuilder.isPresent() && pathPos < path.size() && ( | ||
| // is this a multi-bucket agg with a bucket key specified? |
There was a problem hiding this comment.
The formatting here is pretty harsh. I've been told that having the comments inside the conditional like this may be confusing the formatter, can we try putting them before the while loop and see if this is any better? I'm open to other ideas too, but this is a mess to read right now.
| Aggregation currentAgg = aggregation; | ||
| while (currElement < parsedPath.size() - 1) { | ||
| if (currentAgg == null) { | ||
| throw new AggregationExecutionException( |
There was a problem hiding this comment.
These should be IllegalArgumentExceptions, which render as 400s. Or we can change how AggregationExecutionException reports its status code, but that seems out of scope. I've been meaning to overhaul how we communicate errors...
server/src/main/java/org/elasticsearch/search/aggregations/support/AggregationPath.java
Show resolved
Hide resolved
|
@elasticmachine update branch |
|
@elasticmachine update branch |
|
@elasticmachine update branch |
…bucket and bucket qualified multi-bucket aggs (elastic#85729) Bucket metric pipeline aggregation paths currently always expect the sibling aggregation to be a multi-bucket agg. This honestly doesn't have to be the case for bucket metric pipeline aggs to work. Consider the following path: ``` filter_agg>filters_agg['bucket_foo']>histo>some_metric_agg ``` Since `filter_agg>filters_agg['bucket_foo']` are well defined and are not crossing bucket threasholds, metrics should still be able to be calculated against the bucket values for `histo` This commit allows any combination of single bucket aggs (e.g. filter) and bucket specific multi-bucket aggs before reaching the desired multi-bucket used for the metric calculation.
Bucket metric pipeline aggregation paths currently always expect the sibling aggregation to be a multi-bucket agg.
This honestly doesn't have to be the case for bucket metric pipeline aggs to work.
Consider the following path:
Since
filter_agg>filters_agg['bucket_foo']are well defined and are not crossing bucket threasholds, metrics should still be able to be calculated against the bucket values forhistoThis commit allows any combination of single bucket aggs (e.g. filter) and bucket specific multi-bucket aggs before reaching the desired multi-bucket used for the metric calculation.