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

Make path syntax for lower and upper standard deviation bounds in the extended_stats aggregation more obvious #20818

Closed
wants to merge 1 commit into from
Closed

Make path syntax for lower and upper standard deviation bounds in the extended_stats aggregation more obvious #20818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...main/java/org/elasticsearch/search/aggregations/metrics/stats/extended/ExtendedStats.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public interface ExtendedStats extends Stats {
String getVarianceAsString();


public enum Bounds {
enum Bounds {
UPPER, LOWER
}

Expand Down
14 changes: 14 additions & 0 deletions ...a/org/elasticsearch/search/aggregations/metrics/stats/extended/InternalExtendedStats.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,20 @@ public double value(String name) {
return super.value(name);
}

@Override
public Object getProperty(List<String> path) {
if (path.size() == 2 && "std_deviation_bounds".equals(path.get(0))) {
String bound = path.get(1);
if ("lower".equals(bound)) {
return getStdDeviationBound(Bounds.LOWER);
}
if ("upper".equals(bound)) {
return getStdDeviationBound(Bounds.UPPER);
}
}
return super.getProperty(path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable

}

@Override
public double getSumOfSquares() {
return sumOfSqrs;
Expand Down
17 changes: 17 additions & 0 deletions core/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,13 @@

package org.elasticsearch.search.aggregations.metrics;

import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.search.aggregations.metrics.stats.extended.ExtendedStats;
import org.elasticsearch.search.aggregations.metrics.stats.extended.ExtendedStatsAggregationBuilder;
import org.elasticsearch.search.aggregations.metrics.stats.extended.InternalExtendedStats;
import org.junit.Test;

import java.util.Collections;

public class ExtendedStatsTests extends AbstractNumericMetricTestCase<ExtendedStatsAggregationBuilder> {

Expand All @@ -32,4 +38,15 @@ protected ExtendedStatsAggregationBuilder doCreateTestAggregatorFactory() {
return factory;
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public void getStdDeviationBoundsByPath() throws Exception {
double min = 1.0;
double max = 4.0;

ExtendedStats stats = new InternalExtendedStats("test", 2, min + max, min, max, Math.pow(min, 2) + Math.pow(max, 2),
1.0, DocValueFormat.RAW, Collections.emptyList(), Collections.emptyMap());

assertEquals(stats.getStdDeviationBound(ExtendedStats.Bounds.LOWER), stats.getProperty("std_deviation_bounds.lower"));
assertEquals(stats.getStdDeviationBound(ExtendedStats.Bounds.UPPER), stats.getProperty("std_deviation_bounds.upper"));
}
}