Make path syntax for lower and upper standard deviation bounds in the extended_stats aggregation more obvious #20818
Conversation
… extended_stats aggregation more obvious Fix for elastic#19040
|
Can someone review this? Maybe @colings86? |
|
Jenkins test this |
There was a problem hiding this comment.
@alexshadow007 Thanks for opening the PR, sorry its taken so long to get a review. I left a few comments.
| @@ -32,4 +38,15 @@ protected ExtendedStatsAggregationBuilder doCreateTestAggregatorFactory() { | |||
| return factory; | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Our build does not allow the use of the @Test annotation and instead we have the convention naming test methods starting with test. Could you change this test method to have a name beginning with test? Also it might be a good idea to run gradle precommit after making the change to find out if there are any other issues.
| return getStdDeviationBound(Bounds.UPPER); | ||
| } | ||
| } | ||
| return super.getProperty(path); |
There was a problem hiding this comment.
The disadvantage of doing the change here is that we will have two ways of getting the upper and lower bounds. In buckets path syntax for pipeline aggregations we will have std_deviation_bounds.upper and std_deviation_bounds.lower and for the AggregationPath syntax used by the terms aggregation order parameter we will still use std_upper and std_lower. I now wonder if we should instead change the name in the value(String name) method so that it is standard_deviation_bounds.upper and standard_deviation_bounds.lower? Then it would be consistent all the places it is used. We could make the change in 6.0 and have a change in 5.x which accepts both versions but logs to the deprecation logger if the std_upper or std_lower version is used.
@clintongormley @jpountz what do you think?
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
@alexshadow007 are you still interested in working on this? |
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
Closes #19040