Move auto date histogram to aggregations module #90746
Conversation
| } | ||
| } | ||
|
|
||
| // TODO: maybe add base class that overwrites nodePlugins(...) for all tests that will be added to this module. |
There was a problem hiding this comment.
I will add base test classes in a followup change, so that we will not have to add AggregationsPlugin to each test class to will exist in this module.
| */ | ||
|
|
||
| package org.elasticsearch.search.aggregations.bucket.histogram; | ||
| package org.elasticsearch.search.aggregations.bucket; |
There was a problem hiding this comment.
This test base class can be moved to aggregations module when data histogram has been moved to aggregations module as well. For now test framework is convenient because data histogram lives in server and auto date histogram in aggregations module.
| new InternalGeoCentroidTests(), | ||
| new InternalHistogramTests(), | ||
| new InternalDateHistogramTests(), | ||
| new InternalAutoDateHistogramTests(), |
There was a problem hiding this comment.
See #90294 (comment) for reasoning of removal for now.
| */ | ||
|
|
||
| package org.elasticsearch.search.aggregations.pipeline; | ||
| package org.elasticsearch.aggregations.pipeline; |
There was a problem hiding this comment.
The pipeline tests moved in this PR use PipelineAggregationHelperTests, which randomly use the auto date histogram aggregator. I could have temporarily removed the usage of this aggregator, but it felt better to already move these pipeline aggregator tests to the aggregation module, since this is where the pipeline aggregators that these tests are testing will reside after these have been moved.
| */ | ||
|
|
||
| package org.elasticsearch.search.aggregations.pipeline; | ||
| package org.elasticsearch.aggregations.pipeline; |
There was a problem hiding this comment.
This java integration test is also using: PipelineAggregationHelperTests. So I already moved this integration test to the aggregations module, for the same reason why the other pipeline unit tests were moved.
martijnvg
changed the title
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
| */ | ||
|
|
||
| package org.elasticsearch.aggregations.bucket; | ||
| package org.elasticsearch.aggregations.bucket.adjacency; |
There was a problem hiding this comment.
At one point we were actually trying to reduce the number of packages. I'm not 100% sure why - they don't make a big difference to me either way. I guess I'd prefer they all be in a aggregation.bucket package or something.
There was a problem hiding this comment.
I guess I'd prefer they all be in a aggregation.bucket package or something.
This is what I prefer too, but then I realise it can become a bit messy too. When we move aggregations like term agg that have many helper classes the list of classes the in the bucket package can become overwhelming. Some of these classes due to their name are sorted in a place not close to the actual builder, factory and aggregator classes. Maybe we should change the names of these classes or turn these classes into inner classes? I'm not sure, but then the PRs that move aggregators to the aggregation module also do renames. Maybe we can assess this later and if we decide we want less packages then make these changes in separate PRs?
Note that some packages already share multiple aggregators. Like the histogram or terms package.
At one point we were actually trying to reduce the number of packages.
That is true, but the way I remember this was, avoiding packages to have with just a small number of classes. For example a package with 3 classes or less, is something we should avoid. Most of these aggregator packages have more classes. Enough imo that having its own package is okay.
There was a problem hiding this comment.
Fair enough. Let's keep going then.
…to_aggregations_module
martijnvg
added
the
auto-merge-without-approval
3 participants
This change also moves adjacency_matrix aggregation to its own package.
Note that that this PR also moves test code not related to auto date histogram.
I think this is cleaner then leaving some tests in a non desired state between PRs.
Also the test code that has been moved is slatted for being moved to the aggregations module.
I suspect that future changes, like for example moving
termsagg, require that other aggregationsto be moved as well (e.g.
significant_terms), since a lot of code is reused as well.Relates to #90283