Move bucket_selector agg to the agg module#91355
Move bucket_selector agg to the agg module#91355elasticsearchmachine merged 8 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
This moves the bucket selector aggregation to the aggregations module and ports all of it's ESIntegTestCase tests to REST tests which gets us mixed cluster testing. Relates to elastic#26220 Relates to elastic#90283
martijnvg
left a comment
There was a problem hiding this comment.
👍 assuming successful build
The change looks good. The PivotTests should be easy to fix. The namedXContentRegistry should also load bucket_selector xcontent classes. I don't yet understand RestSqlIT#testFetchAllPagesCompositeAggCursorWithFilterOnAggregate* failures. Maybe flaky test?
I'll have a look. SQL uses bucket selector so it could be a real thing! |
| for (var agg : aggregation.getPipelineAggregations()) { | ||
| if (agg instanceof BucketSelectorPipelineAggregationBuilder) { | ||
| // Use type.equals because there are two copies of BucketSelectorPipelineAggregationBuilder on the classpath | ||
| if (agg.getType().equals(BucketSelectorPipelineAggregationBuilder.NAME)) { |
There was a problem hiding this comment.
@martijnvg, I think this is similar to the kinds of problems you'd had before. I'm not a fan of this solution, but it got the tests passing and we can talk about it here.
There was a problem hiding this comment.
// Use type.equals because there are two copies of BucketSelectorPipelineAggregationBuilder on the classpath
With java modularisation, it shouldn't be possible to have two classes with the same package/name to be exported from different modules?
There was a problem hiding this comment.
Plugins load them on different classloaders. I'm certainly doing something wrong here though.
There was a problem hiding this comment.
I'm ok with this approach here, since for now there isn't an alternative.
This moves the bucket selector aggregation to the aggregations module and ports all of it's ESIntegTestCase tests to REST tests which gets us mixed cluster testing.
Relates to #26220
Relates to #90283