-
Notifications
You must be signed in to change notification settings - Fork 730
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.
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
Deprecate wrong buckets_path initialization on bucket aggregations #2038
Deprecate wrong buckets_path initialization on bucket aggregations #2038
Conversation
a9fe831
to
a9e7cb6
Compare
773deaf
to
f48cacb
Compare
2e21635
to
5e02bed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change LGTM.
The comment I left can easily be a follow up PR or ignored.
@@ -19,6 +19,10 @@ public function __construct(string $name, ?string $bucketsPath = null, ?string $ | |||
|
|||
if (null !== $bucketsPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the top part of the constructor is always shared. Made me wonder if we should extract it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's shared by a lot of pipeline aggregation, but there're some exceptions like bucket_script
or bucket_selector
which accepts an array of buckets paths.
A topic I have in mind for 8.0 version is to review the aggregations classes to separate them in three categories (as described in https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations.html) pipeline
, metric
and bucket
as they have some difference (for example a pipeline aggregation can't have sub aggregations, ...)
No description provided.