Skip to content

Improving parsing of sigma param for Extended Stats Bucket Aggregation #17562

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

Merged
merged 1 commit into from
May 10, 2016
Merged

Improving parsing of sigma param for Extended Stats Bucket Aggregation #17562

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
45 changes: 23 additions & 22 deletions ...ava/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;

import java.io.IOException;
import java.text.ParseException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
Expand All @@ -53,7 +52,7 @@ public final BucketMetricsPipelineAggregatorBuilder<?> parse(String pipelineAggr
String[] bucketsPaths = null;
String format = null;
GapPolicy gapPolicy = null;
Map<String, Object> leftover = new HashMap<>(5);
Map<String, Object> params = new HashMap<>(5);

while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
Expand All @@ -66,7 +65,7 @@ public final BucketMetricsPipelineAggregatorBuilder<?> parse(String pipelineAggr
} else if (context.getParseFieldMatcher().match(currentFieldName, GAP_POLICY)) {
gapPolicy = GapPolicy.parse(context, parser.text(), parser.getTokenLocation());
} else {
leftover.put(currentFieldName, parser.text());
parseToken(pipelineAggregatorName, parser, context, currentFieldName, token, params);
}
} else if (token == XContentParser.Token.START_ARRAY) {
if (context.getParseFieldMatcher().match(currentFieldName, BUCKETS_PATH)) {
Expand All @@ -77,10 +76,10 @@ public final BucketMetricsPipelineAggregatorBuilder<?> parse(String pipelineAggr
}
bucketsPaths = paths.toArray(new String[paths.size()]);
} else {
leftover.put(currentFieldName, parser.list());
parseToken(pipelineAggregatorName, parser, context, currentFieldName, token, params);
}
} else {
leftover.put(currentFieldName, parser.objectText());
parseToken(pipelineAggregatorName, parser, context, currentFieldName, token, params);
Copy link
Contributor

Choose a reason for hiding this comment

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

This reason for using the leftover map here is so we can provide all the incorrect fields in one error message rather than reporting one error at a time and making the user potentially have to iterate multiple times to fix all their errors. Would you be able to revert these changes here so we again have these errors all reported in one go?

In the future I would like to extend this to the entire request and all parsing errors so we report all problems with the request at once.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually looking more at the changes you have made I see that you have made the code more like the other aggregation parsers so I think I would like to keep these changes and implement the above in another way in the future.

}
}

Expand All @@ -89,30 +88,32 @@ public final BucketMetricsPipelineAggregatorBuilder<?> parse(String pipelineAggr
"Missing required field [" + BUCKETS_PATH.getPreferredName() + "] for aggregation [" + pipelineAggregatorName + "]");
}

BucketMetricsPipelineAggregatorBuilder<?> factory = null;
try {
factory = buildFactory(pipelineAggregatorName, bucketsPaths[0], leftover);
if (format != null) {
factory.format(format);
}
if (gapPolicy != null) {
factory.gapPolicy(gapPolicy);
}
} catch (ParseException exception) {
throw new ParsingException(parser.getTokenLocation(),
"Could not parse settings for aggregation [" + pipelineAggregatorName + "].", exception);
BucketMetricsPipelineAggregatorBuilder<?> factory = buildFactory(pipelineAggregatorName, bucketsPaths[0], params);
if (format != null) {
factory.format(format);
}

if (leftover.size() > 0) {
throw new ParsingException(parser.getTokenLocation(),
"Unexpected tokens " + leftover.keySet() + " in [" + pipelineAggregatorName + "].");
if (gapPolicy != null) {
factory.gapPolicy(gapPolicy);
}

assert(factory != null);

return factory;
}

protected abstract BucketMetricsPipelineAggregatorBuilder<?> buildFactory(String pipelineAggregatorName, String bucketsPaths,
Map<String, Object> unparsedParams) throws ParseException;
Map<String, Object> params);

protected boolean token(XContentParser parser, QueryParseContext context, String field,
XContentParser.Token token, Map<String, Object> params) throws IOException {
return false;
}

private void parseToken(String aggregationName, XContentParser parser, QueryParseContext context, String currentFieldName,
XContentParser.Token currentToken, Map<String, Object> params) throws IOException {
if (token(parser, context, currentFieldName, currentToken, params) == false) {
throw new ParsingException(parser.getTokenLocation(),
"Unexpected token " + currentToken + " [" + currentFieldName + "] in [" + aggregationName + "]");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think BuildFactory should be allowed to throw a ParseException since subclasses should have the ability to throw it if there is a problem with creating the builder at this point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is ParseException right choice in this case?
If you want to parse something, you must override method token.
If there something illegal in params you should throw IllegalArgumentException.

}
}
}
4 changes: 2 additions & 2 deletions ...ch/search/aggregations/pipeline/bucketmetrics/avg/AvgBucketPipelineAggregatorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params)
public static final PipelineAggregator.Parser PARSER = new BucketMetricsParser() {
@Override
protected AvgBucketPipelineAggregatorBuilder buildFactory(String pipelineAggregatorName,
String bucketsPath, Map<String, Object> unparsedParams) {
String bucketsPath, Map<String, Object> params) {
return new AvgBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath);
}
};
Expand All @@ -94,4 +94,4 @@ protected boolean innerEquals(BucketMetricsPipelineAggregatorBuilder<AvgBucketPi
public String getWriteableName() {
return NAME;
}
}
}
4 changes: 2 additions & 2 deletions ...ch/search/aggregations/pipeline/bucketmetrics/max/MaxBucketPipelineAggregatorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params)
public static final PipelineAggregator.Parser PARSER = new BucketMetricsParser() {
@Override
protected MaxBucketPipelineAggregatorBuilder buildFactory(String pipelineAggregatorName,
String bucketsPath, Map<String, Object> unparsedParams) {
String bucketsPath, Map<String, Object> params) {
return new MaxBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath);
}
};
Expand All @@ -94,4 +94,4 @@ protected boolean innerEquals(BucketMetricsPipelineAggregatorBuilder<MaxBucketPi
public String getWriteableName() {
return NAME;
}
}
}
4 changes: 2 additions & 2 deletions ...ch/search/aggregations/pipeline/bucketmetrics/min/MinBucketPipelineAggregatorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params)
public static final PipelineAggregator.Parser PARSER = new BucketMetricsParser() {
@Override
protected MinBucketPipelineAggregatorBuilder buildFactory(String pipelineAggregatorName,
String bucketsPath, Map<String, Object> unparsedParams) {
String bucketsPath, Map<String, Object> params) {
return new MinBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath);
}
};
Expand All @@ -94,4 +94,4 @@ protected boolean innerEquals(BucketMetricsPipelineAggregatorBuilder<MinBucketPi
public String getWriteableName() {
return NAME;
}
}
}
52 changes: 25 additions & 27 deletions ...gations/pipeline/bucketmetrics/percentile/PercentilesBucketPipelineAggregatorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,13 @@

package org.elasticsearch.search.aggregations.pipeline.bucketmetrics.percentile;

import com.carrotsearch.hppc.DoubleArrayList;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.query.QueryParseContext;
import org.elasticsearch.search.aggregations.AggregatorFactory;
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregatorBuilder;
Expand Down Expand Up @@ -117,41 +120,36 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params)
}

public static final PipelineAggregator.Parser PARSER = new BucketMetricsParser() {

@Override
protected PercentilesBucketPipelineAggregatorBuilder buildFactory(String pipelineAggregatorName,
String bucketsPath, Map<String, Object> unparsedParams) throws ParseException {
double[] percents = null;
int counter = 0;
Object percentParam = unparsedParams.get(PERCENTS_FIELD.getPreferredName());

if (percentParam != null) {
if (percentParam instanceof List) {
percents = new double[((List<?>) percentParam).size()];
for (Object p : (List<?>) percentParam) {
if (p instanceof Double) {
percents[counter] = (Double) p;
counter += 1;
} else {
throw new ParseException(
"Parameter [" + PERCENTS_FIELD.getPreferredName() + "] must be an array of doubles, type `"
+ percentParam.getClass().getSimpleName() + "` provided instead",
0);
}
}
unparsedParams.remove(PERCENTS_FIELD.getPreferredName());
} else {
throw new ParseException("Parameter [" + PERCENTS_FIELD.getPreferredName() + "] must be an array of doubles, type `"
+ percentParam.getClass().getSimpleName() + "` provided instead", 0);
}
}
String bucketsPath, Map<String, Object> params) {

PercentilesBucketPipelineAggregatorBuilder factory = new
PercentilesBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath);
PercentilesBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath);

double[] percents = (double[]) params.get(PERCENTS_FIELD.getPreferredName());
if (percents != null) {
factory.percents(percents);
}

return factory;
}

@Override
protected boolean token(XContentParser parser, QueryParseContext context, String field,
XContentParser.Token token, Map<String, Object> params) throws IOException {
if (context.getParseFieldMatcher().match(field, PERCENTS_FIELD) && token == XContentParser.Token.START_ARRAY) {
DoubleArrayList percents = new DoubleArrayList(10);
while (parser.nextToken() != XContentParser.Token.END_ARRAY) {
percents.add(parser.doubleValue());
}
params.put(PERCENTS_FIELD.getPreferredName(), percents.toArray());
return true;
}
return false;
}

};

@Override
Expand All @@ -169,4 +167,4 @@ protected boolean innerEquals(BucketMetricsPipelineAggregatorBuilder<Percentiles
public String getWriteableName() {
return NAME;
}
}
}
4 changes: 2 additions & 2 deletions ...earch/aggregations/pipeline/bucketmetrics/stats/StatsBucketPipelineAggregatorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params)
public static final PipelineAggregator.Parser PARSER = new BucketMetricsParser() {
@Override
protected StatsBucketPipelineAggregatorBuilder buildFactory(String pipelineAggregatorName,
String bucketsPath, Map<String, Object> unparsedParams) {
String bucketsPath, Map<String, Object> params) {
return new StatsBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath);
}
};
Expand All @@ -96,4 +96,4 @@ protected boolean innerEquals(BucketMetricsPipelineAggregatorBuilder<StatsBucket
public String getWriteableName() {
return NAME;
}
}
}
33 changes: 17 additions & 16 deletions .../search/aggregations/pipeline/bucketmetrics/stats/extended/ExtendedStatsBucketParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,35 +20,36 @@
package org.elasticsearch.search.aggregations.pipeline.bucketmetrics.stats.extended;

import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.query.QueryParseContext;
import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.BucketMetricsParser;

import java.text.ParseException;
import java.io.IOException;
import java.util.Map;

public class ExtendedStatsBucketParser extends BucketMetricsParser {
static final ParseField SIGMA = new ParseField("sigma");

@Override
protected ExtendedStatsBucketPipelineAggregatorBuilder buildFactory(String pipelineAggregatorName,
String bucketsPath, Map<String, Object> unparsedParams) throws ParseException {

Double sigma = null;
Object param = unparsedParams.get(SIGMA.getPreferredName());

if (param != null) {
if (param instanceof Double) {
sigma = (Double) param;
unparsedParams.remove(SIGMA.getPreferredName());
} else {
throw new ParseException("Parameter [" + SIGMA.getPreferredName() + "] must be a Double, type `"
+ param.getClass().getSimpleName() + "` provided instead", 0);
}
}
String bucketsPath, Map<String, Object> params) {
ExtendedStatsBucketPipelineAggregatorBuilder factory =
new ExtendedStatsBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath);
new ExtendedStatsBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath);
Double sigma = (Double) params.get(SIGMA.getPreferredName());
if (sigma != null) {
factory.sigma(sigma);
}

return factory;
}

@Override
protected boolean token(XContentParser parser, QueryParseContext context, String field,
XContentParser.Token token, Map<String, Object> params) throws IOException {
if (context.getParseFieldMatcher().match(field, SIGMA) && token == XContentParser.Token.VALUE_NUMBER) {
params.put(SIGMA.getPreferredName(), parser.doubleValue());
return true;
}
return false;
}
}
4 changes: 2 additions & 2 deletions ...ch/search/aggregations/pipeline/bucketmetrics/sum/SumBucketPipelineAggregatorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params)
public static final PipelineAggregator.Parser PARSER = new BucketMetricsParser() {
@Override
protected SumBucketPipelineAggregatorBuilder buildFactory(String pipelineAggregatorName,
String bucketsPath, Map<String, Object> unparsedParams) {
String bucketsPath, Map<String, Object> params) {
return new SumBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath);
}
};
Expand All @@ -94,4 +94,4 @@ protected boolean innerEquals(BucketMetricsPipelineAggregatorBuilder<SumBucketPi
public String getWriteableName() {
return NAME;
}
}
}
23 changes: 23 additions & 0 deletions ...rg/elasticsearch/search/aggregations/pipeline/bucketmetrics/ExtendedStatsBucketTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,14 @@

package org.elasticsearch.search.aggregations.pipeline.bucketmetrics;

import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.query.QueryParseContext;
import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.stats.extended.ExtendedStatsBucketPipelineAggregator;
import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.stats.extended.ExtendedStatsBucketPipelineAggregatorBuilder;

import static org.hamcrest.Matchers.equalTo;

public class ExtendedStatsBucketTests extends AbstractBucketMetricsTestCase<ExtendedStatsBucketPipelineAggregatorBuilder> {

@Override
Expand All @@ -32,5 +38,22 @@ protected ExtendedStatsBucketPipelineAggregatorBuilder doCreateTestAggregatorFac
return factory;
}

public void testSigmaFromInt() throws Exception {
String content = XContentFactory.jsonBuilder()
.startObject()
.field("sigma", 5)
.field("buckets_path", "test")
.endObject()
.string();

XContentParser parser = XContentFactory.xContent(content).createParser(content);
QueryParseContext parseContext = new QueryParseContext(queriesRegistry, parser, parseFieldMatcher);
parser.nextToken(); // skip object start

ExtendedStatsBucketPipelineAggregatorBuilder builder = (ExtendedStatsBucketPipelineAggregatorBuilder) aggParsers
.pipelineParser(ExtendedStatsBucketPipelineAggregator.TYPE.name(), parseFieldMatcher)
.parse("test", parseContext);

assertThat(builder.sigma(), equalTo(5.0));
}
}
23 changes: 23 additions & 0 deletions .../org/elasticsearch/search/aggregations/pipeline/bucketmetrics/PercentilesBucketTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,14 @@

package org.elasticsearch.search.aggregations.pipeline.bucketmetrics;

import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.query.QueryParseContext;
import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.percentile.PercentilesBucketPipelineAggregator;
import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.percentile.PercentilesBucketPipelineAggregatorBuilder;

import static org.hamcrest.Matchers.equalTo;

public class PercentilesBucketTests extends AbstractBucketMetricsTestCase<PercentilesBucketPipelineAggregatorBuilder> {

@Override
Expand All @@ -37,5 +43,22 @@ protected PercentilesBucketPipelineAggregatorBuilder doCreateTestAggregatorFacto
return factory;
}

public void testPercentsFromMixedArray() throws Exception {
String content = XContentFactory.jsonBuilder()
.startObject()
.field("buckets_path", "test")
.array("percents", 0, 20.0, 50, 75.99)
.endObject()
.string();

XContentParser parser = XContentFactory.xContent(content).createParser(content);
QueryParseContext parseContext = new QueryParseContext(queriesRegistry, parser, parseFieldMatcher);
parser.nextToken(); // skip object start

PercentilesBucketPipelineAggregatorBuilder builder = (PercentilesBucketPipelineAggregatorBuilder) aggParsers
.pipelineParser(PercentilesBucketPipelineAggregator.TYPE.name(), parseFieldMatcher)
.parse("test", parseContext);

assertThat(builder.percents(), equalTo(new double[]{0.0, 20.0, 50.0, 75.99}));
}
}