Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Collapse aggregation packages #22868

Closed
jpountz opened this issue Jan 30, 2017 · 6 comments
Closed

Collapse aggregation packages #22868

jpountz opened this issue Jan 30, 2017 · 6 comments
Labels
:Analytics/Aggregations Aggregations >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) team-discuss

Comments

@jpountz
Copy link
Contributor

jpountz commented Jan 30, 2017

The fact that every aggregation has its own package requires to make too many classes public. We should look into collapsing packages and making as many classes package-private as possible. Ideally we'd do this close to the 6.0 release to keep backporting easy.
@colings86 colings86 self-assigned this Jul 6, 2017
@colings86 colings86 mentioned this issue Jul 6, 2017
Merged
colings86 added a commit that referenced this issue Jul 10, 2017
@colings86
Collapses package structure for some bucket aggs (#25579)
3a5a54e 
This change collapses some of the packages for the bucket aggregations into their parent packages. This was done for the following aggregations:
* The variants of the range aggregation (geo_distance, date and ip) were moved into the `o.e.s.a.bucket.range` package
* The `o.e.s.a.bucket.terms.support` package was removed and the classes were moved to `o.e.s.a.bucket.terms`
* The filter aggregation was moved to `o.e.s.a.bucket.filter`

Since this PR is already relatively large with only the above changes subsequent PRs will do similar operations on relevant metric and pipeline aggregations

Relates to #22868
@sophiec20
Copy link
Contributor

@jpountz Should this be an open blocker?

@jpountz
Copy link
Contributor Author

jpountz commented Oct 31, 2017 •
edited
Loading

Bumping the version, it's now too late to do this change. Thanks for the ping.

@jpountz jpountz added v7.0.0 and removed v6.0.0 labels Oct 31, 2017
@colings86 colings86 removed their assignment Nov 3, 2017
@polyfractal
Copy link
Contributor

@elastic/es-search-aggs

jimczi added a commit to jimczi/elasticsearch that referenced this issue Sep 6, 2018
@jimczi
Collapse package structure for metrics aggs
23c72e5 
This change collapses all metrics aggregations classes into a single package `org.elasticsearch.aggregations.metrics`.
It also restricts the visibility of some classes (aggregators and factories) that should not be used outside of the package.

Relates elastic#22868
@jimczi jimczi mentioned this issue Sep 6, 2018
Merged
jimczi added a commit that referenced this issue Sep 7, 2018
@jimczi
Collapse package structure for metrics aggs (#33463)
79cd638 
This change collapses all metrics aggregations classes into a single package `org.elasticsearch.aggregations.metrics`.
It also restricts the visibility of some classes (aggregators and factories) that should not be used outside of the package.

Relates #22868
@polyfractal polyfractal mentioned this issue Oct 19, 2018
Merged
@jpountz
Copy link
Contributor Author

jpountz commented Feb 1, 2019

Removing the blocker label since we won't treat it as a blocker.

@jpountz jpountz removed the blocker label Feb 1, 2019
@polyfractal polyfractal added v7.2.0 and removed v7.0.0 labels Apr 9, 2019
@jakelandis jakelandis added v7.3.0 and removed v7.2.0 labels Jun 17, 2019
@jpountz jpountz removed the v7.3.0 label Jul 5, 2019
@rjernst rjernst added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 4, 2020
@csoulios
Copy link
Contributor

csoulios commented Sep 4, 2020

Now that metric and pipeline aggregations have been collapsed to one package each, does it make sense to do the same for bucket aggregations? Bucket agg packages may contain from 6 classes (org.elasticsearch.search.aggregations.bucket.global) to 51 classes (org.elasticsearch.search.aggregations.bucket.terms).

Also, lately we have developed min/max/sum/avg aggs for histogram and aggregate_double_metric field types as x-pack modules. This required changing some classes back to public.

So, I wonder if this issue is still valid.

@csoulios
Copy link
Contributor

csoulios commented Sep 9, 2020

We discussed this issue in the team meeting and agreed that collapsing all bucket aggregations to a single package would result in more confusing package structure. Therefore, we decided not to collapse bucket aggregations and close this ticket.

@csoulios csoulios closed this as completed Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
:Analytics/Aggregations Aggregations >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) team-discuss
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@clintongormley @colings86 @rjernst @jpountz @jakelandis @polyfractal @csoulios @sophiec20