Introduce a new aggregation module by martijnvg · Pull Request #90294 · elastic/elasticsearch

martijnvg · 2022-09-23T09:39:31Z

Also this PR moves the adjacency_matrix to this new module.
This serves as an example how to move the other aggregations from the server module into this new module for aggregations.

The new module name is aggregations.
The new module will use the org.elasticsearch.aggregations.bucket package for all bucket aggregations. (org.elasticsearch.search.aggregations.bucket is already taken by server module).
We can change the naming as part of reviewing this PR.

Relates to #90283

and moves the `adjacency_matrix` to this new module. The new module name is `aggregations`. The new module will use the `org.elasticsearch.aggregations.bucket` package for all bucket aggregations. (`org.elasticsearch.searcg.aggregations.bucket` is already taken by server module). Relates to elastic#90283

martijnvg · 2022-09-23T09:43:21Z

Opening this PR as draft in order for this change to pass PR CI. Probably some issues will occur, that I didn't foresee locally.

…s the jarhell.

martijnvg · 2022-09-23T12:00:59Z

server/src/test/java/org/elasticsearch/search/aggregations/AggregationsTests.java

@@ -138,7 +137,6 @@ public class AggregationsTests extends ESTestCase {
        new InternalDateRangeTests(),


I think we want this test also in the new module? As we move over aggregations we remove entries here and add it to the equivalent test case in aggregations module?

How do we think this should look in the end? Would there be an AggregationsTests in test:framework that has no aggs listed? That feels maybe not great. But neither does having test:framework depend on the module. Maybe it'd make sense to move this class to a aggregations:test module that other aggregation extensions depend on. That'd pick up all these tests.

But in the short term removing this from the list is fine.

Maybe it'd make sense to move this class to a aggregations:test module that other aggregation extensions depend on. That'd pick up all these tests.

Yes, I think when enough tests (50%?) have moved we should move this test to the aggregation module.

…dule

elasticsearchmachine · 2022-09-26T07:04:59Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

nik9000 · 2022-09-26T16:00:05Z

modules/aggregations/src/main/java/module-info.java

+    requires org.elasticsearch.base;
+    requires org.elasticsearch.server;
+    requires org.elasticsearch.xcontent;
+    requires org.apache.logging.log4j;


We might should just use ES's logging directly while we're moving.

nik9000 · 2022-09-26T16:33:48Z

...src/main/java/org/elasticsearch/search/aggregations/support/AggregationInspectionHelper.java

    }

-    public static boolean hasValue(InternalAdjacencyMatrix agg) {
+    public static boolean hasValue(MultiBucketsAggregation agg) {


I've never really understood the point of this method. It seems like we could assert this in the tests if we wanted.

Right, this is used in tests (GeoGridAggregatorTestCase) and sql (MetricAggExtractor). I think this should be moved to sql (looks like the biggest user of this class?) and the one usage in GeoGridAggregatorTestCase can be replaced with adding a helper method that does agg.getBuckets().stream().anyMatch(bucket -> bucket.getDocCount() > 0).

Let's do this in a followup change?

nik9000 · 2022-09-26T16:37:07Z

server/src/test/java/org/elasticsearch/search/aggregations/AggregationsTests.java

@@ -138,7 +137,6 @@ public class AggregationsTests extends ESTestCase {
        new InternalDateRangeTests(),


How do we think this should look in the end? Would there be an AggregationsTests in test:framework that has no aggs listed? That feels maybe not great. But neither does having test:framework depend on the module. Maybe it'd make sense to move this class to a aggregations:test module that other aggregation extensions depend on. That'd pick up all these tests.

nik9000 · 2022-09-26T16:37:18Z

server/src/test/java/org/elasticsearch/search/aggregations/AggregationsTests.java

@@ -138,7 +137,6 @@ public class AggregationsTests extends ESTestCase {
        new InternalDateRangeTests(),


But in the short term removing this from the list is fine.

…dule

nik9000 · 2022-09-29T18:27:28Z

rest-api-spec/build.gradle


 testClusters.configureEach {
  module ':modules:mapper-extras'
+  module ':modules:aggregations'


I think this should be "temporary" - as in, when we've moved the rest of the aggs it should be removed.

I added comment to remind us: 71a3530

…dule

elasticsearchmachine added the v8.6.0 label Sep 23, 2022

martijnvg added 7 commits September 23, 2022 11:44

spotless

05825ce

spotless

855ec3e

add modules-info

589486a

remove dependency to new module from test framework plugin. This fixe…

59086fe

…s the jarhell.

fixed tests

3a02b42

fixed module yaml tests

3adf0bf

fix test compile error

c34fedc

martijnvg commented Sep 23, 2022

View reviewed changes

ensure that aggregation module is loaded for bwc yaml compat tests

1828b70

martijnvg changed the title ~~Introduces a new aggregation module~~ Introduce a new aggregation module Sep 23, 2022

martijnvg mentioned this pull request Sep 23, 2022

Centralize aggregations into a module #90283

Open

60 tasks

Merge remote-tracking branch 'es/main' into introduce_aggregations_mo…

013abde

…dule

martijnvg added >non-issue :Analytics/Aggregations Aggregations labels Sep 26, 2022

martijnvg marked this pull request as ready for review September 26, 2022 07:04

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 26, 2022

nik9000 reviewed Sep 26, 2022

View reviewed changes

martijnvg added 3 commits September 27, 2022 08:50

Merge remote-tracking branch 'es/main' into introduce_aggregations_mo…

617f60b

…dule

update module-info.java

a767ae7

added package level java docs

bdac183

martijnvg requested a review from nik9000 September 27, 2022 07:28

fixed jdocs

4363496

martijnvg mentioned this pull request Sep 28, 2022

Tidyup aggs-matrix-stats module-info.java #90384

Merged

nik9000 approved these changes Sep 29, 2022

View reviewed changes

martijnvg added 2 commits September 30, 2022 09:22

added a comment

71a3530

Merge remote-tracking branch 'es/main' into introduce_aggregations_mo…

ee8c73c

…dule

martijnvg merged commit 14cf04d into elastic:main Sep 30, 2022

martijnvg mentioned this pull request Oct 10, 2022

Move auto date histogram to aggregations module #90746

Merged

		@@ -138,7 +137,6 @@ public class AggregationsTests extends ESTestCase {
		new InternalDateRangeTests(),

Conversation

martijnvg commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnvg commented Sep 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Sep 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

martijnvg commented Sep 23, 2022 •

edited

Loading