From 93d1b385a4373eb59284ba4afeffd7f37ffc7416 Mon Sep 17 00:00:00 2001 From: Alexander Kazakov Date: Wed, 6 Apr 2016 17:54:17 +0300 Subject: [PATCH] Improving parsing of sigma param for Extended Stats Bucket Aggregation #17499 --- .../bucketmetrics/BucketMetricsParser.java | 45 ++++++++-------- .../AvgBucketPipelineAggregatorBuilder.java | 4 +- .../MaxBucketPipelineAggregatorBuilder.java | 4 +- .../MinBucketPipelineAggregatorBuilder.java | 4 +- ...ntilesBucketPipelineAggregatorBuilder.java | 52 +++++++++---------- .../StatsBucketPipelineAggregatorBuilder.java | 4 +- .../extended/ExtendedStatsBucketParser.java | 33 ++++++------ .../SumBucketPipelineAggregatorBuilder.java | 4 +- .../ExtendedStatsBucketTests.java | 23 ++++++++ .../bucketmetrics/PercentilesBucketTests.java | 23 ++++++++ 10 files changed, 121 insertions(+), 75 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsParser.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsParser.java index dbd0a5d128cde..8eddd83b7e273 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsParser.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsParser.java @@ -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; @@ -53,7 +52,7 @@ public final BucketMetricsPipelineAggregatorBuilder parse(String pipelineAggr String[] bucketsPaths = null; String format = null; GapPolicy gapPolicy = null; - Map leftover = new HashMap<>(5); + Map params = new HashMap<>(5); while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { @@ -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)) { @@ -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); } } @@ -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 unparsedParams) throws ParseException; + Map params); + protected boolean token(XContentParser parser, QueryParseContext context, String field, + XContentParser.Token token, Map params) throws IOException { + return false; + } + + private void parseToken(String aggregationName, XContentParser parser, QueryParseContext context, String currentFieldName, + XContentParser.Token currentToken, Map params) throws IOException { + if (token(parser, context, currentFieldName, currentToken, params) == false) { + throw new ParsingException(parser.getTokenLocation(), + "Unexpected token " + currentToken + " [" + currentFieldName + "] in [" + aggregationName + "]"); + } + } } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/avg/AvgBucketPipelineAggregatorBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/avg/AvgBucketPipelineAggregatorBuilder.java index 3435c9768b097..248186b24bc3e 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/avg/AvgBucketPipelineAggregatorBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/avg/AvgBucketPipelineAggregatorBuilder.java @@ -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 unparsedParams) { + String bucketsPath, Map params) { return new AvgBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath); } }; @@ -94,4 +94,4 @@ protected boolean innerEquals(BucketMetricsPipelineAggregatorBuilder unparsedParams) { + String bucketsPath, Map params) { return new MaxBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath); } }; @@ -94,4 +94,4 @@ protected boolean innerEquals(BucketMetricsPipelineAggregatorBuilder unparsedParams) { + String bucketsPath, Map params) { return new MinBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath); } }; @@ -94,4 +94,4 @@ protected boolean innerEquals(BucketMetricsPipelineAggregatorBuilder 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 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 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 @@ -169,4 +167,4 @@ protected boolean innerEquals(BucketMetricsPipelineAggregatorBuilder unparsedParams) { + String bucketsPath, Map params) { return new StatsBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath); } }; @@ -96,4 +96,4 @@ protected boolean innerEquals(BucketMetricsPipelineAggregatorBuilder 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 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 params) throws IOException { + if (context.getParseFieldMatcher().match(field, SIGMA) && token == XContentParser.Token.VALUE_NUMBER) { + params.put(SIGMA.getPreferredName(), parser.doubleValue()); + return true; + } + return false; + } } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/sum/SumBucketPipelineAggregatorBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/sum/SumBucketPipelineAggregatorBuilder.java index 0243de209161c..3fb2322dedc0a 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/sum/SumBucketPipelineAggregatorBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/sum/SumBucketPipelineAggregatorBuilder.java @@ -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 unparsedParams) { + String bucketsPath, Map params) { return new SumBucketPipelineAggregatorBuilder(pipelineAggregatorName, bucketsPath); } }; @@ -94,4 +94,4 @@ protected boolean innerEquals(BucketMetricsPipelineAggregatorBuilder { @Override @@ -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)); + } } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/PercentilesBucketTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/PercentilesBucketTests.java index 0ee57c309a113..236f122bdd020 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/PercentilesBucketTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/PercentilesBucketTests.java @@ -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 { @Override @@ -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})); + } }