Aggregations: moving_avg model parser should accept any numeric, not just doubles
Also changes the models to throw ParseExceptions instead of SearchParseExceptions, so that the validation can be unit-tested. Fixes #11487
This commit is contained in:
Zachary Tong
parent
1ac728d22b
commit
ae742c4a03
|
|
@ -33,6 +33,7 @@ import org.elasticsearch.search.aggregations.support.format.ValueFormatter;
|
|||
import org.elasticsearch.search.internal.SearchContext;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.text.ParseException;
|
||||
import java.util.ArrayList;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
|
|
@ -144,7 +145,14 @@ public class MovAvgParser implements PipelineAggregator.Parser {
|
|||
throw new SearchParseException(context, "Unknown model [" + model + "] specified. Valid options are:"
|
||||
+ movAvgModelParserMapper.getAllNames().toString(), parser.getTokenLocation());
|
||||
}
|
||||
MovAvgModel movAvgModel = modelParser.parse(settings, pipelineAggregatorName, context, window);
|
||||
|
||||
MovAvgModel movAvgModel;
|
||||
try {
|
||||
movAvgModel = modelParser.parse(settings, pipelineAggregatorName, window);
|
||||
} catch (ParseException exception) {
|
||||
throw new SearchParseException(context, "Could not parse settings for model [" + model + "].", null, exception);
|
||||
}
|
||||
|
||||
|
||||
return new MovAvgPipelineAggregator.Factory(pipelineAggregatorName, bucketsPaths, formatter, gapPolicy, window, predict,
|
||||
movAvgModel);
|
||||
|
|
|
|||
|
|
@ -28,6 +28,7 @@ import org.elasticsearch.search.aggregations.pipeline.movavg.MovAvgParser;
|
|||
import org.elasticsearch.search.internal.SearchContext;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.text.ParseException;
|
||||
import java.util.Collection;
|
||||
import java.util.Map;
|
||||
|
||||
|
|
@ -92,9 +93,9 @@ public class EwmaModel extends MovAvgModel {
|
|||
}
|
||||
|
||||
@Override
|
||||
public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, SearchContext context, int windowSize) {
|
||||
public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize) throws ParseException {
|
||||
|
||||
double alpha = parseDoubleParam(context, settings, "alpha", 0.5);
|
||||
double alpha = parseDoubleParam(settings, "alpha", 0.5);
|
||||
|
||||
return new EwmaModel(alpha);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -28,6 +28,7 @@ import org.elasticsearch.search.aggregations.pipeline.movavg.MovAvgParser;
|
|||
import org.elasticsearch.search.internal.SearchContext;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.text.ParseException;
|
||||
import java.util.*;
|
||||
|
||||
/**
|
||||
|
|
@ -151,10 +152,10 @@ public class HoltLinearModel extends MovAvgModel {
|
|||
}
|
||||
|
||||
@Override
|
||||
public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, SearchContext context, int windowSize) {
|
||||
public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize) throws ParseException {
|
||||
|
||||
double alpha = parseDoubleParam(context, settings, "alpha", 0.5);
|
||||
double beta = parseDoubleParam(context, settings, "beta", 0.5);
|
||||
double alpha = parseDoubleParam(settings, "alpha", 0.5);
|
||||
double beta = parseDoubleParam(settings, "beta", 0.5);
|
||||
return new HoltLinearModel(alpha, beta);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -32,6 +32,7 @@ import org.elasticsearch.search.aggregations.pipeline.movavg.MovAvgParser;
|
|||
import org.elasticsearch.search.internal.SearchContext;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.text.ParseException;
|
||||
import java.util.*;
|
||||
|
||||
/**
|
||||
|
|
@ -316,17 +317,17 @@ public class HoltWintersModel extends MovAvgModel {
|
|||
}
|
||||
|
||||
@Override
|
||||
public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, SearchContext context, int windowSize) {
|
||||
public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize) throws ParseException {
|
||||
|
||||
double alpha = parseDoubleParam(context, settings, "alpha", 0.5);
|
||||
double beta = parseDoubleParam(context, settings, "beta", 0.5);
|
||||
double gamma = parseDoubleParam(context, settings, "gamma", 0.5);
|
||||
int period = parseIntegerParam(context, settings, "period", 1);
|
||||
double alpha = parseDoubleParam(settings, "alpha", 0.5);
|
||||
double beta = parseDoubleParam(settings, "beta", 0.5);
|
||||
double gamma = parseDoubleParam(settings, "gamma", 0.5);
|
||||
int period = parseIntegerParam(settings, "period", 1);
|
||||
|
||||
if (windowSize < 2 * period) {
|
||||
throw new SearchParseException(context, "Field [window] must be at least twice as large as the period when " +
|
||||
throw new ParseException("Field [window] must be at least twice as large as the period when " +
|
||||
"using Holt-Winters. Value provided was [" + windowSize + "], which is less than (2*period) == "
|
||||
+ (2 * period), null);
|
||||
+ (2 * period), 0);
|
||||
}
|
||||
|
||||
SeasonalityType seasonalityType = SeasonalityType.ADDITIVE;
|
||||
|
|
@ -337,13 +338,13 @@ public class HoltWintersModel extends MovAvgModel {
|
|||
if (value instanceof String) {
|
||||
seasonalityType = SeasonalityType.parse((String)value);
|
||||
} else {
|
||||
throw new SearchParseException(context, "Parameter [type] must be a String, type `"
|
||||
+ value.getClass().getSimpleName() + "` provided instead", null);
|
||||
throw new ParseException("Parameter [type] must be a String, type `"
|
||||
+ value.getClass().getSimpleName() + "` provided instead", 0);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
boolean pad = parseBoolParam(context, settings, "pad", seasonalityType.equals(SeasonalityType.MULTIPLICATIVE));
|
||||
boolean pad = parseBoolParam(settings, "pad", seasonalityType.equals(SeasonalityType.MULTIPLICATIVE));
|
||||
|
||||
return new HoltWintersModel(alpha, beta, gamma, period, seasonalityType, pad);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -29,6 +29,7 @@ import org.elasticsearch.search.aggregations.pipeline.movavg.MovAvgParser;
|
|||
import org.elasticsearch.search.internal.SearchContext;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.text.ParseException;
|
||||
import java.util.Collection;
|
||||
import java.util.Map;
|
||||
|
||||
|
|
@ -79,7 +80,7 @@ public class LinearModel extends MovAvgModel {
|
|||
}
|
||||
|
||||
@Override
|
||||
public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, SearchContext context, int windowSize) {
|
||||
public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize) throws ParseException {
|
||||
return new LinearModel();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -27,6 +27,7 @@ import org.elasticsearch.search.SearchParseException;
|
|||
import org.elasticsearch.search.internal.SearchContext;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.text.ParseException;
|
||||
import java.util.Arrays;
|
||||
import java.util.Collection;
|
||||
import java.util.Map;
|
||||
|
|
@ -125,26 +126,24 @@ public abstract class MovAvgModel {
|
|||
*
|
||||
* @param settings Map of settings, extracted from the request
|
||||
* @param pipelineName Name of the parent pipeline agg
|
||||
* @param context The parser context that we are in
|
||||
* @param windowSize Size of the window for this moving avg
|
||||
* @return A fully built moving average model
|
||||
*/
|
||||
public abstract MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, SearchContext context, int windowSize);
|
||||
public abstract MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize) throws ParseException;
|
||||
|
||||
|
||||
/**
|
||||
* Extracts a 0-1 inclusive double from the settings map, otherwise throws an exception
|
||||
*
|
||||
* @param context Search query context
|
||||
* @param settings Map of settings provided to this model
|
||||
* @param name Name of parameter we are attempting to extract
|
||||
* @param defaultValue Default value to be used if value does not exist in map
|
||||
*
|
||||
* @throws SearchParseException
|
||||
* @throws ParseException
|
||||
*
|
||||
* @return Double value extracted from settings map
|
||||
*/
|
||||
protected double parseDoubleParam(SearchContext context, @Nullable Map<String, Object> settings, String name, double defaultValue) {
|
||||
protected double parseDoubleParam(@Nullable Map<String, Object> settings, String name, double defaultValue) throws ParseException {
|
||||
if (settings == null) {
|
||||
return defaultValue;
|
||||
}
|
||||
|
|
@ -152,33 +151,32 @@ public abstract class MovAvgModel {
|
|||
Object value = settings.get(name);
|
||||
if (value == null) {
|
||||
return defaultValue;
|
||||
} else if (value instanceof Double) {
|
||||
double v = (Double)value;
|
||||
} else if (value instanceof Number) {
|
||||
double v = ((Number) value).doubleValue();
|
||||
if (v >= 0 && v <= 1) {
|
||||
return v;
|
||||
}
|
||||
|
||||
throw new SearchParseException(context, "Parameter [" + name + "] must be between 0-1 inclusive. Provided"
|
||||
+ "value was [" + v + "]", null);
|
||||
throw new ParseException("Parameter [" + name + "] must be between 0-1 inclusive. Provided"
|
||||
+ "value was [" + v + "]", 0);
|
||||
}
|
||||
|
||||
throw new SearchParseException(context, "Parameter [" + name + "] must be a double, type `"
|
||||
+ value.getClass().getSimpleName() + "` provided instead", null);
|
||||
throw new ParseException("Parameter [" + name + "] must be a double, type `"
|
||||
+ value.getClass().getSimpleName() + "` provided instead", 0);
|
||||
}
|
||||
|
||||
/**
|
||||
* Extracts an integer from the settings map, otherwise throws an exception
|
||||
*
|
||||
* @param context Search query context
|
||||
* @param settings Map of settings provided to this model
|
||||
* @param name Name of parameter we are attempting to extract
|
||||
* @param defaultValue Default value to be used if value does not exist in map
|
||||
*
|
||||
* @throws SearchParseException
|
||||
* @throws ParseException
|
||||
*
|
||||
* @return Integer value extracted from settings map
|
||||
*/
|
||||
protected int parseIntegerParam(SearchContext context, @Nullable Map<String, Object> settings, String name, int defaultValue) {
|
||||
protected int parseIntegerParam(@Nullable Map<String, Object> settings, String name, int defaultValue) throws ParseException {
|
||||
if (settings == null) {
|
||||
return defaultValue;
|
||||
}
|
||||
|
|
@ -186,18 +184,17 @@ public abstract class MovAvgModel {
|
|||
Object value = settings.get(name);
|
||||
if (value == null) {
|
||||
return defaultValue;
|
||||
} else if (value instanceof Integer) {
|
||||
return (Integer)value;
|
||||
} else if (value instanceof Number) {
|
||||
return ((Number) value).intValue();
|
||||
}
|
||||
|
||||
throw new SearchParseException(context, "Parameter [" + name + "] must be an integer, type `"
|
||||
+ value.getClass().getSimpleName() + "` provided instead", null);
|
||||
throw new ParseException("Parameter [" + name + "] must be an integer, type `"
|
||||
+ value.getClass().getSimpleName() + "` provided instead", 0);
|
||||
}
|
||||
|
||||
/**
|
||||
* Extracts a boolean from the settings map, otherwise throws an exception
|
||||
*
|
||||
* @param context Search query context
|
||||
* @param settings Map of settings provided to this model
|
||||
* @param name Name of parameter we are attempting to extract
|
||||
* @param defaultValue Default value to be used if value does not exist in map
|
||||
|
|
@ -206,7 +203,7 @@ public abstract class MovAvgModel {
|
|||
*
|
||||
* @return Boolean value extracted from settings map
|
||||
*/
|
||||
protected boolean parseBoolParam(SearchContext context, @Nullable Map<String, Object> settings, String name, boolean defaultValue) {
|
||||
protected boolean parseBoolParam(@Nullable Map<String, Object> settings, String name, boolean defaultValue) throws ParseException {
|
||||
if (settings == null) {
|
||||
return defaultValue;
|
||||
}
|
||||
|
|
@ -218,8 +215,8 @@ public abstract class MovAvgModel {
|
|||
return (Boolean)value;
|
||||
}
|
||||
|
||||
throw new SearchParseException(context, "Parameter [" + name + "] must be a boolean, type `"
|
||||
+ value.getClass().getSimpleName() + "` provided instead", null);
|
||||
throw new ParseException("Parameter [" + name + "] must be a boolean, type `"
|
||||
+ value.getClass().getSimpleName() + "` provided instead", 0);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -28,6 +28,7 @@ import org.elasticsearch.search.aggregations.pipeline.movavg.MovAvgParser;
|
|||
import org.elasticsearch.search.internal.SearchContext;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.text.ParseException;
|
||||
import java.util.Collection;
|
||||
import java.util.Map;
|
||||
|
||||
|
|
@ -72,7 +73,7 @@ public class SimpleModel extends MovAvgModel {
|
|||
}
|
||||
|
||||
@Override
|
||||
public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, SearchContext context, int windowSize) {
|
||||
public MovAvgModel parse(@Nullable Map<String, Object> settings, String pipelineName, int windowSize) throws ParseException {
|
||||
return new SimpleModel();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -21,6 +21,7 @@ package org.elasticsearch.search.aggregations.pipeline.moving.avg;
|
|||
|
||||
import com.google.common.collect.EvictingQueue;
|
||||
|
||||
import org.elasticsearch.search.SearchParseException;
|
||||
import org.elasticsearch.search.aggregations.pipeline.movavg.models.*;
|
||||
import org.elasticsearch.test.ElasticsearchTestCase;
|
||||
|
||||
|
|
@ -28,7 +29,8 @@ import static org.hamcrest.Matchers.equalTo;
|
|||
|
||||
import org.junit.Test;
|
||||
|
||||
import java.util.Arrays;
|
||||
import java.text.ParseException;
|
||||
import java.util.*;
|
||||
|
||||
public class MovAvgUnitTests extends ElasticsearchTestCase {
|
||||
|
||||
|
|
@ -583,4 +585,50 @@ public class MovAvgUnitTests extends ElasticsearchTestCase {
|
|||
}
|
||||
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testNumericValidation() {
|
||||
|
||||
List<MovAvgModel.AbstractModelParser> parsers = new ArrayList<>(5);
|
||||
|
||||
// Simple and Linear don't have any settings to test
|
||||
parsers.add(new EwmaModel.SingleExpModelParser());
|
||||
parsers.add(new HoltWintersModel.HoltWintersModelParser());
|
||||
parsers.add(new HoltLinearModel.DoubleExpModelParser());
|
||||
|
||||
|
||||
Object[] values = {(byte)1, 1, 1L, (short)1, (double)1};
|
||||
Map<String, Object> settings = new HashMap<>(2);
|
||||
|
||||
for (MovAvgModel.AbstractModelParser parser : parsers) {
|
||||
for (Object v : values) {
|
||||
settings.put("alpha", v);
|
||||
settings.put("beta", v);
|
||||
settings.put("gamma", v);
|
||||
|
||||
try {
|
||||
parser.parse(settings, "pipeline", 10);
|
||||
} catch (ParseException e) {
|
||||
fail(parser.getName() + " parser should not have thrown SearchParseException while parsing [" +
|
||||
v.getClass().getSimpleName() +"]");
|
||||
}
|
||||
|
||||
}
|
||||
}
|
||||
|
||||
for (MovAvgModel.AbstractModelParser parser : parsers) {
|
||||
settings.put("alpha", "abc");
|
||||
settings.put("beta", "abc");
|
||||
settings.put("gamma", "abc");
|
||||
|
||||
try {
|
||||
parser.parse(settings, "pipeline", 10);
|
||||
} catch (ParseException e) {
|
||||
//all good
|
||||
continue;
|
||||
}
|
||||
|
||||
fail(parser.getName() + " parser should have thrown SearchParseException while parsing [String]");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue