Move bucket_selector agg to the agg module #91355
Conversation
nik9000
added
>test
elasticsearchmachine
added
the
Team:Analytics
|
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
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.
nik9000
added
the
auto-merge-without-approval
3 participants
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