diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/180_percentiles_tdigest_metric.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/180_percentiles_tdigest_metric.yml index faae9c1ccda..9ed414f6b84 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/180_percentiles_tdigest_metric.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/180_percentiles_tdigest_metric.yml @@ -262,7 +262,7 @@ setup: "Invalid params test": - do: - catch: /\[compression\] must be greater than or equal to 0. Found \[-1.0\] in \[percentiles_int\]/ + catch: /\[compression\] must be greater than or equal to 0. Found \[-1.0\]/ search: rest_total_hits_as_int: true body: @@ -274,7 +274,7 @@ setup: compression: -1 - do: - catch: /\[percents\] must not be empty/ + catch: bad_request search: rest_total_hits_as_int: true body: diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/190_percentiles_hdr_metric.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/190_percentiles_hdr_metric.yml index 809e18458f0..32c349c5e46 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/190_percentiles_hdr_metric.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/190_percentiles_hdr_metric.yml @@ -309,7 +309,7 @@ setup: number_of_significant_value_digits: null - do: - catch: /\[percents\] must not be empty/ + catch: bad_request search: rest_total_hits_as_int: true body: diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesAggregationBuilder.java new file mode 100644 index 00000000000..dae008183f2 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesAggregationBuilder.java @@ -0,0 +1,365 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.search.aggregations.metrics; + +import org.elasticsearch.Version; +import org.elasticsearch.common.Nullable; +import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.TriFunction; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.search.aggregations.AggregatorFactories; +import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; +import org.elasticsearch.search.aggregations.support.ValueType; +import org.elasticsearch.search.aggregations.support.ValuesSource; +import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder; +import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper; + +import java.io.IOException; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.function.Supplier; + +/** + * This provides a base class for aggregations that are building percentiles or percentiles-like functionality (e.g. percentile ranks). + * It provides a set of common fields/functionality for setting the available algorithms (TDigest and HDRHistogram), + * as well as algorithm-specific settings via a {@link PercentilesConfig} object + */ +public abstract class AbstractPercentilesAggregationBuilder> + extends ValuesSourceAggregationBuilder.LeafOnly { + + public static final ParseField KEYED_FIELD = new ParseField("keyed"); + protected boolean keyed = true; + protected double[] values; + private PercentilesConfig percentilesConfig; + private ParseField valuesField; + + public static > ConstructingObjectParser createParser(String aggName, + TriFunction ctor, + Supplier defaultConfig, + ParseField valuesField) { + + /** + * This is a non-ideal ConstructingObjectParser, because it is a compromise between Percentiles and Ranks. + * Ranks requires an array of values because there is no sane default, and we want to keep that in the ctor. + * Percentiles has defaults, which means the API allows the user to either use the default or configure + * their own. + * + * The mutability of Percentiles keeps us from having a strict ConstructingObjectParser, while the ctor + * of Ranks keeps us from using a regular ObjectParser. + * + * This is a compromise, in that it is a ConstructingOP which accepts all optional arguments, and then we sort + * out the behavior from there + * + * `args` are provided from the ConstructingObjectParser in-order they are defined in the parser. So: + * - args[0]: values + * - args[1]: tdigest config options + * - args[2]: hdr config options + * + * If `args` is null or empty, it means all were omitted. This is usually an anti-pattern for + * ConstructingObjectParser, but we're allowing it because of the above-mentioned reasons + */ + ConstructingObjectParser parser = new ConstructingObjectParser<>(aggName, false, (args, name) -> { + + if (args == null || args.length == 0) { + // Note: if this is a Percentiles agg, the null `values` will be converted into a default, + // whereas a Ranks agg will throw an exception due to missing a required param + return ctor.apply(name, null, defaultConfig.get()); + } + + PercentilesConfig tDigestConfig = (PercentilesConfig) args[1]; + PercentilesConfig hdrConfig = (PercentilesConfig) args[2]; + + double[] values = args[0] != null ? ((List) args[0]).stream().mapToDouble(Double::doubleValue).toArray() : null; + PercentilesConfig percentilesConfig; + + if (tDigestConfig != null && hdrConfig != null) { + throw new IllegalArgumentException("Only one percentiles method should be declared."); + } else if (tDigestConfig == null && hdrConfig == null) { + percentilesConfig = defaultConfig.get(); + } else if (tDigestConfig != null) { + percentilesConfig = tDigestConfig; + } else { + percentilesConfig = hdrConfig; + } + + return ctor.apply(name, values, percentilesConfig); + }); + + ValuesSourceParserHelper.declareAnyFields(parser, true, true); + parser.declareDoubleArray(ConstructingObjectParser.optionalConstructorArg(), valuesField); + parser.declareBoolean(T::keyed, KEYED_FIELD); + parser.declareObject(ConstructingObjectParser.optionalConstructorArg(), PercentilesMethod.TDIGEST_PARSER, + PercentilesMethod.TDIGEST.getParseField()); + parser.declareObject(ConstructingObjectParser.optionalConstructorArg(), PercentilesMethod.HDR_PARSER, + PercentilesMethod.HDR.getParseField()); + + return parser; + } + + AbstractPercentilesAggregationBuilder(String name, double[] values, PercentilesConfig percentilesConfig, + ParseField valuesField) { + super(name, CoreValuesSourceType.NUMERIC, ValueType.NUMERIC); + if (values == null) { + throw new IllegalArgumentException("[" + valuesField.getPreferredName() + "] must not be null: [" + name + "]"); + } + if (values.length == 0) { + throw new IllegalArgumentException("[" + valuesField.getPreferredName() + "] must not be an empty array: [" + name + "]"); + } + double[] sortedValues = Arrays.copyOf(values, values.length); + Arrays.sort(sortedValues); + this.values = sortedValues; + this.percentilesConfig = percentilesConfig; + this.valuesField = valuesField; + } + + AbstractPercentilesAggregationBuilder(AbstractPercentilesAggregationBuilder clone, + AggregatorFactories.Builder factoriesBuilder, Map metaData) { + super(clone, factoriesBuilder, metaData); + this.percentilesConfig = clone.percentilesConfig; + this.keyed = clone.keyed; + this.values = clone.values; + this.valuesField = clone.valuesField; + } + + AbstractPercentilesAggregationBuilder(StreamInput in) throws IOException { + super(in, CoreValuesSourceType.NUMERIC, ValueType.NUMERIC); + values = in.readDoubleArray(); + keyed = in.readBoolean(); + if (in.getVersion().onOrAfter(Version.V_7_8_0)) { + percentilesConfig + = (PercentilesConfig) in.readOptionalWriteable((Reader) PercentilesConfig::fromStream); + } else { + int numberOfSignificantValueDigits = in.readVInt(); + double compression = in.readDouble(); + PercentilesMethod method = PercentilesMethod.readFromStream(in); + percentilesConfig = PercentilesConfig.fromLegacy(method, compression, numberOfSignificantValueDigits); + } + } + + @Override + protected void innerWriteTo(StreamOutput out) throws IOException { + out.writeDoubleArray(values); + out.writeBoolean(keyed); + if (out.getVersion().onOrAfter(Version.V_7_8_0)) { + out.writeOptionalWriteable(percentilesConfig); + } else { + // Legacy method serialized both SigFigs and compression, even though we only need one. So we need + // to serialize the default for the unused method + int numberOfSignificantValueDigits = percentilesConfig.getMethod().equals(PercentilesMethod.HDR) + ? ((PercentilesConfig.Hdr)percentilesConfig).getNumberOfSignificantValueDigits() + : PercentilesConfig.Hdr.DEFAULT_NUMBER_SIG_FIGS; + + double compression = percentilesConfig.getMethod().equals(PercentilesMethod.TDIGEST) + ? ((PercentilesConfig.TDigest)percentilesConfig).getCompression() + : PercentilesConfig.TDigest.DEFAULT_COMPRESSION; + + out.writeVInt(numberOfSignificantValueDigits); + out.writeDouble(compression); + percentilesConfig.getMethod().writeTo(out); + } + } + + /** + * Set whether the XContent response should be keyed + */ + public T keyed(boolean keyed) { + this.keyed = keyed; + return (T) this; + } + + /** + * Get whether the XContent response should be keyed + */ + public boolean keyed() { + return keyed; + } + + /** + * Expert: set the number of significant digits in the values. Only relevant + * when using {@link PercentilesMethod#HDR}. + * + * Deprecated: set numberOfSignificantValueDigits by configuring a {@link PercentilesConfig.Hdr} instead + * and set via {@link PercentilesAggregationBuilder#percentilesConfig(PercentilesConfig)} + */ + @Deprecated + public T numberOfSignificantValueDigits(int numberOfSignificantValueDigits) { + if (percentilesConfig == null || percentilesConfig.getMethod().equals(PercentilesMethod.HDR)) { + percentilesConfig = new PercentilesConfig.Hdr(numberOfSignificantValueDigits); + } else { + throw new IllegalArgumentException("Cannot set [numberOfSignificantValueDigits] because the method " + + "has already been configured for TDigest"); + } + + return (T) this; + } + + /** + * Expert: get the number of significant digits in the values. Only relevant + * when using {@link PercentilesMethod#HDR}. + * + * Deprecated: get numberOfSignificantValueDigits by inspecting the {@link PercentilesConfig} returned from + * {@link PercentilesAggregationBuilder#percentilesConfig()} instead + */ + @Deprecated + public int numberOfSignificantValueDigits() { + if (percentilesConfig != null && percentilesConfig.getMethod().equals(PercentilesMethod.HDR)) { + return ((PercentilesConfig.Hdr)percentilesConfig).getNumberOfSignificantValueDigits(); + } + throw new IllegalStateException("Percentiles [method] has not been configured yet, or is a TDigest"); + } + + /** + * Expert: set the compression. Higher values improve accuracy but also + * memory usage. Only relevant when using {@link PercentilesMethod#TDIGEST}. + * + * Deprecated: set compression by configuring a {@link PercentilesConfig.TDigest} instead + * and set via {@link PercentilesAggregationBuilder#percentilesConfig(PercentilesConfig)} + */ + @Deprecated + public T compression(double compression) { + if (percentilesConfig == null || percentilesConfig.getMethod().equals(PercentilesMethod.TDIGEST)) { + percentilesConfig = new PercentilesConfig.TDigest(compression); + } else { + throw new IllegalArgumentException("Cannot set [compression] because the method has already been configured for HDRHistogram"); + } + return (T) this; + } + + /** + * Expert: get the compression. Higher values improve accuracy but also + * memory usage. Only relevant when using {@link PercentilesMethod#TDIGEST}. + * + * Deprecated: get compression by inspecting the {@link PercentilesConfig} returned from + * {@link PercentilesAggregationBuilder#percentilesConfig()} instead + */ + @Deprecated + public double compression() { + if (percentilesConfig != null && percentilesConfig.getMethod().equals(PercentilesMethod.TDIGEST)) { + return ((PercentilesConfig.TDigest)percentilesConfig).getCompression(); + } + throw new IllegalStateException("Percentiles [method] has not been configured yet, or is a HdrHistogram"); + } + + + /** + * Deprecated: set method by configuring a {@link PercentilesConfig} instead + * and set via {@link PercentilesAggregationBuilder#percentilesConfig(PercentilesConfig)} + */ + @Deprecated + public T method(PercentilesMethod method) { + if (method == null) { + throw new IllegalArgumentException("[method] must not be null: [" + name + "]"); + } + if (percentilesConfig == null) { + if (method.equals(PercentilesMethod.TDIGEST) ) { + this.percentilesConfig = new PercentilesConfig.TDigest(); + } else { + this.percentilesConfig = new PercentilesConfig.Hdr(); + } + } else if (percentilesConfig.getMethod().equals(method) == false) { + // we already have an algo configured, but it's different from the requested method + // reset to default for the requested method + if (method.equals(PercentilesMethod.TDIGEST) ) { + this.percentilesConfig = new PercentilesConfig.TDigest(); + } else { + this.percentilesConfig = new PercentilesConfig.Hdr(); + } + } // if method and config were same, this is a no-op so we don't overwrite settings + + return (T) this; + } + + /** + * Deprecated: get method by inspecting the {@link PercentilesConfig} returned from + * {@link PercentilesAggregationBuilder#percentilesConfig()} instead + */ + @Nullable + @Deprecated + public PercentilesMethod method() { + return percentilesConfig == null ? null : percentilesConfig.getMethod(); + } + + /** + * Returns how the percentiles algorithm has been configured, or null if it has not been configured yet + */ + @Nullable + public PercentilesConfig percentilesConfig() { + return percentilesConfig; + } + + /** + * Sets how the percentiles algorithm should be configured + */ + public T percentilesConfig(PercentilesConfig percentilesConfig) { + this.percentilesConfig = percentilesConfig; + return (T) this; + } + + /** + * Return the current algo configuration, or a default (Tdigest) otherwise + * + * This is needed because builders don't have a "build" or "finalize" method, but + * the old API did bake in defaults. Certain operations like xcontent, equals, hashcode + * will use the values in the builder at any time and need to be aware of defaults. + * + * But to maintain BWC behavior as much as possible, we allow the user to set + * algo settings independent of method. To keep life simple we use a null to track + * if any method has been selected yet. + * + * However, this means we need a way to fetch the default if the user hasn't + * selected any method and uses a builder-side feature like xcontent + */ + PercentilesConfig configOrDefault() { + if (percentilesConfig == null) { + return new PercentilesConfig.TDigest(); + } + return percentilesConfig; + } + + @Override + protected XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { + builder.array(valuesField.getPreferredName(), values); + builder.field(KEYED_FIELD.getPreferredName(), keyed); + builder = configOrDefault().toXContent(builder, params); + return builder; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) return true; + if (obj == null || getClass() != obj.getClass()) return false; + if (super.equals(obj) == false) return false; + + AbstractPercentilesAggregationBuilder other = (AbstractPercentilesAggregationBuilder) obj; + return Objects.deepEquals(values, other.values) + && Objects.equals(keyed, other.keyed) + && Objects.equals(configOrDefault(), other.configOrDefault()); + } + + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), Arrays.hashCode(values), keyed, configOrDefault()); + } +} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksAggregatorFactory.java deleted file mode 100644 index 1c689f92905..00000000000 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksAggregatorFactory.java +++ /dev/null @@ -1,73 +0,0 @@ -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.elasticsearch.search.aggregations.metrics; - -import org.elasticsearch.index.query.QueryShardContext; -import org.elasticsearch.search.aggregations.Aggregator; -import org.elasticsearch.search.aggregations.AggregatorFactories; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; -import org.elasticsearch.search.aggregations.support.ValuesSource; -import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; -import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; -import org.elasticsearch.search.internal.SearchContext; - -import java.io.IOException; -import java.util.List; -import java.util.Map; - -class HDRPercentileRanksAggregatorFactory - extends ValuesSourceAggregatorFactory { - - private final double[] values; - private final int numberOfSignificantValueDigits; - private final boolean keyed; - - HDRPercentileRanksAggregatorFactory(String name, ValuesSourceConfig config, double[] values, - int numberOfSignificantValueDigits, boolean keyed, QueryShardContext queryShardContext, - AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, - Map metadata) throws IOException { - super(name, config, queryShardContext, parent, subFactoriesBuilder, metadata); - this.values = values; - this.numberOfSignificantValueDigits = numberOfSignificantValueDigits; - this.keyed = keyed; - } - - @Override - protected Aggregator createUnmapped(SearchContext searchContext, - Aggregator parent, - List pipelineAggregators, - Map metadata) throws IOException { - return new HDRPercentileRanksAggregator(name, null, searchContext, parent, values, numberOfSignificantValueDigits, keyed, - config.format(), pipelineAggregators, metadata); - } - - @Override - protected Aggregator doCreateInternal(ValuesSource valuesSource, - SearchContext searchContext, - Aggregator parent, - boolean collectsFromSingleBucket, - List pipelineAggregators, - Map metadata) throws IOException { - return new HDRPercentileRanksAggregator(name, valuesSource, searchContext, parent, values, numberOfSignificantValueDigits, keyed, - config.format(), pipelineAggregators, metadata); - } - -} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregationBuilder.java index 2005f81de87..a61e57bd628 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregationBuilder.java @@ -21,146 +21,57 @@ package org.elasticsearch.search.aggregations.metrics; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ConstructingObjectParser; -import org.elasticsearch.common.xcontent.ObjectParser; -import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; -import org.elasticsearch.search.aggregations.support.ValueType; import org.elasticsearch.search.aggregations.support.ValuesSource; -import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder.LeafOnly; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; -import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper; import java.io.IOException; -import java.util.Arrays; -import java.util.List; import java.util.Map; -import java.util.Objects; -import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; - -public class PercentileRanksAggregationBuilder extends LeafOnly { +public class PercentileRanksAggregationBuilder extends AbstractPercentilesAggregationBuilder { public static final String NAME = PercentileRanks.TYPE_NAME; - public static final ParseField VALUES_FIELD = new ParseField("values"); - - private static class TDigestOptions { - Double compression; - } - - private static final ObjectParser TDIGEST_OPTIONS_PARSER = - new ObjectParser<>(PercentilesMethod.TDIGEST.getParseField().getPreferredName(), TDigestOptions::new); - static { - TDIGEST_OPTIONS_PARSER.declareDouble((opts, compression) -> opts.compression = compression, new ParseField("compression")); - } - - private static class HDROptions { - Integer numberOfSigDigits; - } - - private static final ObjectParser HDR_OPTIONS_PARSER = - new ObjectParser<>(PercentilesMethod.HDR.getParseField().getPreferredName(), HDROptions::new); - static { - HDR_OPTIONS_PARSER.declareInt((opts, numberOfSigDigits) -> opts.numberOfSigDigits = numberOfSigDigits, - new ParseField("number_of_significant_value_digits")); - } - - // The builder requires two parameters for the constructor: aggregation name and values array. The - // agg name is supplied externally via the Parser's context (as a String), while the values array - // is parsed from the request and supplied to the ConstructingObjectParser as a ctor argument + private static final ParseField VALUES_FIELD = new ParseField("values"); private static final ConstructingObjectParser PARSER; static { - PARSER = new ConstructingObjectParser<>(PercentileRanksAggregationBuilder.NAME, false, - (a, context) -> new PercentileRanksAggregationBuilder(context, (List) a[0])); - ValuesSourceParserHelper.declareAnyFields(PARSER, true, true); - PARSER.declareDoubleArray(constructorArg(), VALUES_FIELD); - PARSER.declareBoolean(PercentileRanksAggregationBuilder::keyed, PercentilesAggregationBuilder.KEYED_FIELD); - - PARSER.declareField((b, v) -> { - b.method(PercentilesMethod.TDIGEST); - if (v.compression != null) { - b.compression(v.compression); - } - }, TDIGEST_OPTIONS_PARSER::parse, PercentilesMethod.TDIGEST.getParseField(), ObjectParser.ValueType.OBJECT); - - PARSER.declareField((b, v) -> { - b.method(PercentilesMethod.HDR); - if (v.numberOfSigDigits != null) { - b.numberOfSignificantValueDigits(v.numberOfSigDigits); - } - }, HDR_OPTIONS_PARSER::parse, PercentilesMethod.HDR.getParseField(), ObjectParser.ValueType.OBJECT); + PARSER = AbstractPercentilesAggregationBuilder.createParser( + PercentileRanksAggregationBuilder.NAME, + PercentileRanksAggregationBuilder::new, + PercentilesConfig.TDigest::new, + VALUES_FIELD); } public static AggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { - // the aggregation name is supplied to the parser as a Context. See note at top of Parser for more details return PARSER.parse(parser, aggregationName); } - private double[] values; - private PercentilesMethod method = PercentilesMethod.TDIGEST; - private int numberOfSignificantValueDigits = 3; - private double compression = 100.0; - private boolean keyed = true; - - private PercentileRanksAggregationBuilder(String name, List values) { - this(name, values.stream().mapToDouble(Double::doubleValue).toArray()); - } - public PercentileRanksAggregationBuilder(String name, double[] values) { - super(name, CoreValuesSourceType.NUMERIC, ValueType.NUMERIC); - if (values == null) { - throw new IllegalArgumentException("[values] must not be null: [" + name + "]"); - } - if (values.length == 0) { - throw new IllegalArgumentException("[values] must not be an empty array: [" + name + "]"); - } - double[] sortedValues = Arrays.copyOf(values, values.length); - Arrays.sort(sortedValues); - this.values = sortedValues; + this(name, values, null); } - protected PercentileRanksAggregationBuilder(PercentileRanksAggregationBuilder clone, - Builder factoriesBuilder, - Map metadata) { - super(clone, factoriesBuilder, metadata); - this.values = clone.values; - this.method = clone.method; - this.numberOfSignificantValueDigits = clone.numberOfSignificantValueDigits; - this.compression = clone.compression; - this.keyed = clone.keyed; + private PercentileRanksAggregationBuilder(String name, double[] values, PercentilesConfig percentilesConfig) { + super(name, values, percentilesConfig, VALUES_FIELD); } - @Override - protected AggregationBuilder shallowCopy(Builder factoriesBuilder, Map metadata) { - return new PercentileRanksAggregationBuilder(this, factoriesBuilder, metadata); - } - - /** - * Read from a stream. - */ public PercentileRanksAggregationBuilder(StreamInput in) throws IOException { - super(in, CoreValuesSourceType.NUMERIC, ValueType.NUMERIC); - values = in.readDoubleArray(); - keyed = in.readBoolean(); - numberOfSignificantValueDigits = in.readVInt(); - compression = in.readDouble(); - method = PercentilesMethod.readFromStream(in); + super(in); + } + + private PercentileRanksAggregationBuilder(PercentileRanksAggregationBuilder clone, + Builder factoriesBuilder, + Map metaData) { + super(clone, factoriesBuilder, metaData); } @Override - protected void innerWriteTo(StreamOutput out) throws IOException { - out.writeDoubleArray(values); - out.writeBoolean(keyed); - out.writeVInt(numberOfSignificantValueDigits); - out.writeDouble(compression); - method.writeTo(out); + protected AggregationBuilder shallowCopy(Builder factoriesBuilder, Map metaData) { + return new PercentileRanksAggregationBuilder(this, factoriesBuilder, metaData); } /** @@ -170,141 +81,13 @@ public class PercentileRanksAggregationBuilder extends LeafOnly 5) { - throw new IllegalArgumentException("[numberOfSignificantValueDigits] must be between 0 and 5: [" + name + "]"); - } - this.numberOfSignificantValueDigits = numberOfSignificantValueDigits; - return this; - } - - /** - * Expert: get the number of significant digits in the values. Only relevant - * when using {@link PercentilesMethod#HDR}. - */ - public int numberOfSignificantValueDigits() { - return numberOfSignificantValueDigits; - } - - /** - * Expert: set the compression. Higher values improve accuracy but also - * memory usage. Only relevant when using {@link PercentilesMethod#TDIGEST}. - */ - public PercentileRanksAggregationBuilder compression(double compression) { - if (compression < 0.0) { - throw new IllegalArgumentException( - "[compression] must be greater than or equal to 0. Found [" + compression + "] in [" + name + "]"); - } - this.compression = compression; - return this; - } - - /** - * Expert: get the compression. Higher values improve accuracy but also - * memory usage. Only relevant when using {@link PercentilesMethod#TDIGEST}. - */ - public double compression() { - return compression; - } - - public PercentileRanksAggregationBuilder method(PercentilesMethod method) { - if (method == null) { - throw new IllegalArgumentException("[method] must not be null: [" + name + "]"); - } - this.method = method; - return this; - } - - public PercentilesMethod method() { - return method; - } - @Override protected ValuesSourceAggregatorFactory innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { - switch (method) { - case TDIGEST: - return new TDigestPercentileRanksAggregatorFactory(name, config, values, compression, keyed, queryShardContext, parent, - subFactoriesBuilder, metadata); - case HDR: - return new HDRPercentileRanksAggregatorFactory(name, config, values, numberOfSignificantValueDigits, keyed, queryShardContext, + return new PercentileRanksAggregatorFactory(name, config, values, configOrDefault(), keyed, queryShardContext, parent, subFactoriesBuilder, metadata); - default: - throw new IllegalStateException("Illegal method [" + method + "]"); - } - } - - @Override - protected XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { - builder.array(VALUES_FIELD.getPreferredName(), values); - builder.field(PercentilesAggregationBuilder.KEYED_FIELD.getPreferredName(), keyed); - builder.startObject(method.toString()); - if (method == PercentilesMethod.TDIGEST) { - builder.field(PercentilesAggregationBuilder.COMPRESSION_FIELD.getPreferredName(), compression); - } else { - builder.field(PercentilesAggregationBuilder.NUMBER_SIGNIFICANT_DIGITS_FIELD.getPreferredName(), numberOfSignificantValueDigits); - } - builder.endObject(); - return builder; - } - - @Override - public boolean equals(Object obj) { - if (this == obj) return true; - if (obj == null || getClass() != obj.getClass()) return false; - if (super.equals(obj) == false) return false; - PercentileRanksAggregationBuilder other = (PercentileRanksAggregationBuilder) obj; - if (Objects.equals(method, other.method) == false) { - return false; - } - boolean equalSettings = false; - switch (method) { - case HDR: - equalSettings = Objects.equals(numberOfSignificantValueDigits, other.numberOfSignificantValueDigits); - break; - case TDIGEST: - equalSettings = Objects.equals(compression, other.compression); - break; - default: - throw new IllegalStateException("Illegal method [" + method + "]"); - } - return equalSettings - && Objects.deepEquals(values, other.values) - && Objects.equals(keyed, other.keyed) - && Objects.equals(method, other.method); - } - - @Override - public int hashCode() { - switch (method) { - case HDR: - return Objects.hash(super.hashCode(), Arrays.hashCode(values), keyed, numberOfSignificantValueDigits, method); - case TDIGEST: - return Objects.hash(super.hashCode(), Arrays.hashCode(values), keyed, compression, method); - default: - throw new IllegalStateException("Illegal method [" + method + "]"); - } } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregatorFactory.java similarity index 51% rename from server/src/main/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesAggregatorFactory.java rename to server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregatorFactory.java index 9814ebf6d69..99ddd0162be 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregatorFactory.java @@ -33,46 +33,45 @@ import java.io.IOException; import java.util.List; import java.util.Map; -class HDRPercentilesAggregatorFactory extends ValuesSourceAggregatorFactory { +class PercentileRanksAggregatorFactory extends ValuesSourceAggregatorFactory { private final double[] percents; - private final int numberOfSignificantValueDigits; + private final PercentilesConfig percentilesConfig; private final boolean keyed; - HDRPercentilesAggregatorFactory(String name, - ValuesSourceConfig config, - double[] percents, - int numberOfSignificantValueDigits, - boolean keyed, - QueryShardContext queryShardContext, - AggregatorFactory parent, - AggregatorFactories.Builder subFactoriesBuilder, - Map metadata) throws IOException { - super(name, config, queryShardContext, parent, subFactoriesBuilder, metadata); + PercentileRanksAggregatorFactory(String name, + ValuesSourceConfig config, + double[] percents, + PercentilesConfig percentilesConfig, + boolean keyed, + QueryShardContext queryShardContext, + AggregatorFactory parent, + AggregatorFactories.Builder subFactoriesBuilder, + Map metaData) throws IOException { + super(name, config, queryShardContext, parent, subFactoriesBuilder, metaData); this.percents = percents; - this.numberOfSignificantValueDigits = numberOfSignificantValueDigits; + this.percentilesConfig = percentilesConfig; this.keyed = keyed; } @Override protected Aggregator createUnmapped(SearchContext searchContext, - Aggregator parent, - List pipelineAggregators, - Map metadata) - throws IOException { - return new HDRPercentilesAggregator(name, null, searchContext, parent, percents, numberOfSignificantValueDigits, keyed, - config.format(), pipelineAggregators, metadata); + Aggregator parent, + List pipelineAggregators, + Map metaData) throws IOException { + + return percentilesConfig.createPercentileRanksAggregator(name, null, searchContext, parent, percents, keyed, + config.format(), pipelineAggregators, metaData); } @Override protected Aggregator doCreateInternal(ValuesSource valuesSource, - SearchContext searchContext, - Aggregator parent, - boolean collectsFromSingleBucket, - List pipelineAggregators, - Map metadata) throws IOException { - return new HDRPercentilesAggregator(name, valuesSource, searchContext, parent, percents, numberOfSignificantValueDigits, keyed, - config.format(), pipelineAggregators, metadata); + SearchContext searchContext, + Aggregator parent, + boolean collectsFromSingleBucket, + List pipelineAggregators, + Map metaData) throws IOException { + return percentilesConfig.createPercentileRanksAggregator(name, valuesSource, searchContext, parent, percents, keyed, + config.format(), pipelineAggregators, metaData); } - } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java index 38e6a2834fd..ed69b796bec 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java @@ -21,249 +21,98 @@ package org.elasticsearch.search.aggregations.metrics; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.xcontent.ObjectParser; -import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; -import org.elasticsearch.search.aggregations.support.ValueType; import org.elasticsearch.search.aggregations.support.ValuesSource; -import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder.LeafOnly; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; -import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper; import java.io.IOException; import java.util.Arrays; import java.util.Map; -import java.util.Objects; -import java.util.function.Consumer; -public class PercentilesAggregationBuilder extends LeafOnly { +public class PercentilesAggregationBuilder extends AbstractPercentilesAggregationBuilder { public static final String NAME = Percentiles.TYPE_NAME; private static final double[] DEFAULT_PERCENTS = new double[] { 1, 5, 25, 50, 75, 95, 99 }; - public static final ParseField PERCENTS_FIELD = new ParseField("percents"); - public static final ParseField KEYED_FIELD = new ParseField("keyed"); - public static final ParseField COMPRESSION_FIELD = new ParseField("compression"); - public static final ParseField NUMBER_SIGNIFICANT_DIGITS_FIELD = new ParseField("number_of_significant_value_digits"); + private static final ParseField PERCENTS_FIELD = new ParseField("percents"); - private static class TDigestOptions { - Double compression; - } - - private static final ObjectParser TDIGEST_OPTIONS_PARSER = - new ObjectParser<>(PercentilesMethod.TDIGEST.getParseField().getPreferredName(), TDigestOptions::new); + private static final ConstructingObjectParser PARSER; static { - TDIGEST_OPTIONS_PARSER.declareDouble((opts, compression) -> opts.compression = compression, COMPRESSION_FIELD); + PARSER = AbstractPercentilesAggregationBuilder.createParser( + PercentilesAggregationBuilder.NAME, + (name, values, percentileConfig) -> { + if (values == null) { + values = DEFAULT_PERCENTS; // this is needed because Percentiles has a default, while Ranks does not + } else { + values = validatePercentiles(values, name); + } + return new PercentilesAggregationBuilder(name, values, percentileConfig); + }, + PercentilesConfig.TDigest::new, + PERCENTS_FIELD); } - private static class HDROptions { - Integer numberOfSigDigits; - } - - private static final ObjectParser HDR_OPTIONS_PARSER = - new ObjectParser<>(PercentilesMethod.HDR.getParseField().getPreferredName(), HDROptions::new); - static { - HDR_OPTIONS_PARSER.declareInt( - (opts, numberOfSigDigits) -> opts.numberOfSigDigits = numberOfSigDigits, - NUMBER_SIGNIFICANT_DIGITS_FIELD); - } - - private static final ObjectParser PARSER; - static { - PARSER = new ObjectParser<>(PercentilesAggregationBuilder.NAME); - ValuesSourceParserHelper.declareAnyFields(PARSER, true, true); - - PARSER.declareDoubleArray( - (b, v) -> b.percentiles(v.stream().mapToDouble(Double::doubleValue).toArray()), - PERCENTS_FIELD); - - PARSER.declareBoolean(PercentilesAggregationBuilder::keyed, KEYED_FIELD); - - PARSER.declareField((b, v) -> { - b.method(PercentilesMethod.TDIGEST); - if (v.compression != null) { - b.compression(v.compression); - } - }, TDIGEST_OPTIONS_PARSER::parse, PercentilesMethod.TDIGEST.getParseField(), ObjectParser.ValueType.OBJECT); - - PARSER.declareField((b, v) -> { - b.method(PercentilesMethod.HDR); - if (v.numberOfSigDigits != null) { - b.numberOfSignificantValueDigits(v.numberOfSigDigits); - } - }, HDR_OPTIONS_PARSER::parse, PercentilesMethod.HDR.getParseField(), ObjectParser.ValueType.OBJECT); + public PercentilesAggregationBuilder(StreamInput in) throws IOException { + super(in); } public static AggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { - InternalBuilder internal = PARSER.parse(parser, new InternalBuilder(aggregationName), null); - // we need to return a PercentilesAggregationBuilder for equality checks to work - PercentilesAggregationBuilder returnedAgg = new PercentilesAggregationBuilder(internal.name); - setIfNotNull(returnedAgg::valueType, internal.valueType()); - setIfNotNull(returnedAgg::format, internal.format()); - setIfNotNull(returnedAgg::missing, internal.missing()); - setIfNotNull(returnedAgg::field, internal.field()); - setIfNotNull(returnedAgg::script, internal.script()); - setIfNotNull(returnedAgg::method, internal.method()); - setIfNotNull(returnedAgg::percentiles, internal.percentiles()); - returnedAgg.keyed(internal.keyed()); - returnedAgg.compression(internal.compression()); - returnedAgg.numberOfSignificantValueDigits(internal.numberOfSignificantValueDigits()); - return returnedAgg; + return PARSER.parse(parser, aggregationName); } - private static void setIfNotNull(Consumer consumer, T value) { - if (value != null) { - consumer.accept(value); - } - } - - private double[] percents = DEFAULT_PERCENTS; - private PercentilesMethod method = PercentilesMethod.TDIGEST; - private int numberOfSignificantValueDigits = 3; - private double compression = 100.0; - private boolean keyed = true; - public PercentilesAggregationBuilder(String name) { - super(name, CoreValuesSourceType.NUMERIC, ValueType.NUMERIC); + this(name, DEFAULT_PERCENTS, null); + } + + public PercentilesAggregationBuilder(String name, double[] values, PercentilesConfig percentilesConfig) { + super(name, values, percentilesConfig, PERCENTS_FIELD); } protected PercentilesAggregationBuilder(PercentilesAggregationBuilder clone, - Builder factoriesBuilder, Map metadata) { - super(clone, factoriesBuilder, metadata); - this.percents = clone.percents; - this.method = clone.method; - this.numberOfSignificantValueDigits = clone.numberOfSignificantValueDigits; - this.compression = clone.compression; - this.keyed = clone.keyed; + Builder factoriesBuilder, Map metaData) { + super(clone, factoriesBuilder, metaData); } @Override - protected AggregationBuilder shallowCopy(Builder factoriesBuilder, Map metadata) { - return new PercentilesAggregationBuilder(this, factoriesBuilder, metadata); - } - - /** - * Read from a stream. - */ - public PercentilesAggregationBuilder(StreamInput in) throws IOException { - super(in, CoreValuesSourceType.NUMERIC, ValueType.NUMERIC); - percents = in.readDoubleArray(); - keyed = in.readBoolean(); - numberOfSignificantValueDigits = in.readVInt(); - compression = in.readDouble(); - method = PercentilesMethod.readFromStream(in); - } - - @Override - protected void innerWriteTo(StreamOutput out) throws IOException { - out.writeDoubleArray(percents); - out.writeBoolean(keyed); - out.writeVInt(numberOfSignificantValueDigits); - out.writeDouble(compression); - method.writeTo(out); + protected AggregationBuilder shallowCopy(Builder factoriesBuilder, Map metaData) { + return new PercentilesAggregationBuilder(this, factoriesBuilder, metaData); } /** * Set the values to compute percentiles from. */ public PercentilesAggregationBuilder percentiles(double... percents) { + this.values = validatePercentiles(percents, name); + return this; + } + + private static double[] validatePercentiles(double[] percents, String aggName) { if (percents == null) { - throw new IllegalArgumentException("[percents] must not be null: [" + name + "]"); + throw new IllegalArgumentException("[percents] must not be null: [" + aggName + "]"); } if (percents.length == 0) { - throw new IllegalArgumentException("[percents] must not be empty: [" + name + "]"); + throw new IllegalArgumentException("[percents] must not be empty: [" + aggName + "]"); } double[] sortedPercents = Arrays.copyOf(percents, percents.length); Arrays.sort(sortedPercents); for (double percent : sortedPercents) { if (percent < 0.0 || percent > 100.0) { - throw new IllegalArgumentException("percent must be in [0,100], got [" + percent + "]: [" + name + "]"); + throw new IllegalArgumentException("percent must be in [0,100], got [" + percent + "]: [" + aggName + "]"); } } - - this.percents = sortedPercents; - return this; + return sortedPercents; } /** * Get the values to compute percentiles from. */ public double[] percentiles() { - return percents; - } - - /** - * Set whether the XContent response should be keyed - */ - public PercentilesAggregationBuilder keyed(boolean keyed) { - this.keyed = keyed; - return this; - } - - /** - * Get whether the XContent response should be keyed - */ - public boolean keyed() { - return keyed; - } - - /** - * Expert: set the number of significant digits in the values. Only relevant - * when using {@link PercentilesMethod#HDR}. - */ - public PercentilesAggregationBuilder numberOfSignificantValueDigits(int numberOfSignificantValueDigits) { - if (numberOfSignificantValueDigits < 0 || numberOfSignificantValueDigits > 5) { - throw new IllegalArgumentException("[numberOfSignificantValueDigits] must be between 0 and 5: [" + name + "]"); - } - this.numberOfSignificantValueDigits = numberOfSignificantValueDigits; - return this; - } - - /** - * Expert: get the number of significant digits in the values. Only relevant - * when using {@link PercentilesMethod#HDR}. - */ - public int numberOfSignificantValueDigits() { - return numberOfSignificantValueDigits; - } - - /** - * Expert: set the compression. Higher values improve accuracy but also - * memory usage. Only relevant when using {@link PercentilesMethod#TDIGEST}. - */ - public PercentilesAggregationBuilder compression(double compression) { - if (compression < 0.0) { - throw new IllegalArgumentException( - "[compression] must be greater than or equal to 0. Found [" + compression + "] in [" + name + "]"); - } - this.compression = compression; - return this; - } - - /** - * Expert: get the compression. Higher values improve accuracy but also - * memory usage. Only relevant when using {@link PercentilesMethod#TDIGEST}. - */ - public double compression() { - return compression; - } - - public PercentilesAggregationBuilder method(PercentilesMethod method) { - if (method == null) { - throw new IllegalArgumentException("[method] must not be null: [" + name + "]"); - } - this.method = method; - return this; - } - - public PercentilesMethod method() { - return method; + return values; } @Override @@ -271,98 +120,12 @@ public class PercentilesAggregationBuilder extends LeafOnly config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { - switch (method) { - case TDIGEST: - return new TDigestPercentilesAggregatorFactory(name, config, percents, compression, keyed, queryShardContext, parent, - subFactoriesBuilder, metadata); - case HDR: - return new HDRPercentilesAggregatorFactory(name, config, percents, - numberOfSignificantValueDigits, keyed, queryShardContext, parent, subFactoriesBuilder, metadata); - default: - throw new IllegalStateException("Illegal method [" + method + "]"); - } - } - - @Override - protected XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { - builder.array(PERCENTS_FIELD.getPreferredName(), percents); - builder.field(KEYED_FIELD.getPreferredName(), keyed); - builder.startObject(method.toString()); - if (method == PercentilesMethod.TDIGEST) { - builder.field(COMPRESSION_FIELD.getPreferredName(), compression); - } else { - builder.field(NUMBER_SIGNIFICANT_DIGITS_FIELD.getPreferredName(), numberOfSignificantValueDigits); - } - builder.endObject(); - return builder; - } - - @Override - public boolean equals(Object obj) { - if (this == obj) return true; - if (obj == null || getClass() != obj.getClass()) return false; - if (super.equals(obj) == false) return false; - - PercentilesAggregationBuilder other = (PercentilesAggregationBuilder) obj; - if (Objects.equals(method, other.method) == false) { - return false; - } - boolean equalSettings = false; - switch (method) { - case HDR: - equalSettings = Objects.equals(numberOfSignificantValueDigits, other.numberOfSignificantValueDigits); - break; - case TDIGEST: - equalSettings = Objects.equals(compression, other.compression); - break; - default: - throw new IllegalStateException("Illegal method [" + method.toString() + "]"); - } - return equalSettings - && Objects.deepEquals(percents, other.percents) - && Objects.equals(keyed, other.keyed) - && Objects.equals(method, other.method); - } - - @Override - public int hashCode() { - switch (method) { - case HDR: - return Objects.hash(super.hashCode(), Arrays.hashCode(percents), keyed, numberOfSignificantValueDigits, method); - case TDIGEST: - return Objects.hash(super.hashCode(), Arrays.hashCode(percents), keyed, compression, method); - default: - throw new IllegalStateException("Illegal method [" + method.toString() + "]"); - } + return new PercentilesAggregatorFactory(name, config, values, configOrDefault(), keyed, + queryShardContext, parent, subFactoriesBuilder, metadata); } @Override public String getType() { return NAME; } - - /** - * Private specialization of this builder that should only be used by the parser, this enables us to - * overwrite {@link #method()} to check that it is not defined twice in xContent and throw - * an error, while the Java API should allow to overwrite the method - */ - private static class InternalBuilder extends PercentilesAggregationBuilder { - - private boolean setOnce = false; - - private InternalBuilder(String name) { - super(name); - } - - @Override - public InternalBuilder method(PercentilesMethod method) { - if (setOnce == false) { - super.method(method); - setOnce = true; - return this; - } else { - throw new IllegalStateException("Only one percentiles method should be declared."); - } - } - } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregatorFactory.java similarity index 52% rename from server/src/main/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesAggregatorFactory.java rename to server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregatorFactory.java index a292fe1705d..570125f3ea1 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentilesAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregatorFactory.java @@ -33,40 +33,45 @@ import java.io.IOException; import java.util.List; import java.util.Map; -class TDigestPercentilesAggregatorFactory - extends ValuesSourceAggregatorFactory { +/** + * This factory is used to generate both TDigest and HDRHisto aggregators, depending + * on the selected method + */ +class PercentilesAggregatorFactory extends ValuesSourceAggregatorFactory { private final double[] percents; - private final double compression; + private final PercentilesConfig percentilesConfig; private final boolean keyed; - TDigestPercentilesAggregatorFactory(String name, ValuesSourceConfig config, double[] percents, - double compression, boolean keyed, QueryShardContext queryShardContext, AggregatorFactory parent, - AggregatorFactories.Builder subFactoriesBuilder, Map metadata) throws IOException { - super(name, config, queryShardContext, parent, subFactoriesBuilder, metadata); + PercentilesAggregatorFactory(String name, ValuesSourceConfig config, double[] percents, + PercentilesConfig percentilesConfig, boolean keyed, QueryShardContext queryShardContext, + AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, + Map metaData) throws IOException { + super(name, config, queryShardContext, parent, subFactoriesBuilder, metaData); this.percents = percents; - this.compression = compression; + this.percentilesConfig = percentilesConfig; this.keyed = keyed; } @Override protected Aggregator createUnmapped(SearchContext searchContext, - Aggregator parent, - List pipelineAggregators, - Map metadata) throws IOException { - return new TDigestPercentilesAggregator(name, null, searchContext, parent, percents, compression, keyed, config.format(), - pipelineAggregators, metadata); + Aggregator parent, + List pipelineAggregators, + Map metaData) throws IOException { + + return percentilesConfig.createPercentilesAggregator(name, null, searchContext, parent, percents, keyed, + config.format(), pipelineAggregators, metaData); } @Override protected Aggregator doCreateInternal(ValuesSource valuesSource, - SearchContext searchContext, - Aggregator parent, - boolean collectsFromSingleBucket, - List pipelineAggregators, - Map metadata) throws IOException { - return new TDigestPercentilesAggregator(name, valuesSource, searchContext, parent, percents, compression, keyed, config.format(), - pipelineAggregators, metadata); - } + SearchContext searchContext, + Aggregator parent, + boolean collectsFromSingleBucket, + List pipelineAggregators, + Map metaData) throws IOException { + return percentilesConfig.createPercentilesAggregator(name, valuesSource, searchContext, parent, percents, keyed, + config.format(), pipelineAggregators, metaData); + } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesConfig.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesConfig.java new file mode 100644 index 00000000000..af7f9ed5ec4 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesConfig.java @@ -0,0 +1,253 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.search.aggregations.metrics; + +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.aggregations.Aggregator; +import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import org.elasticsearch.search.aggregations.support.ValuesSource; +import org.elasticsearch.search.internal.SearchContext; + +import java.io.IOException; +import java.util.List; +import java.util.Map; +import java.util.Objects; + +/** + * A small config object that carries algo-specific settings. This allows the factory to have + * a single unified constructor for both algos, but internally switch execution + * depending on which algo is selected + */ +public abstract class PercentilesConfig implements ToXContent, Writeable { + private final PercentilesMethod method; + + PercentilesConfig(PercentilesMethod method) { + this.method = method; + } + + public static PercentilesConfig fromStream(StreamInput in) throws IOException { + PercentilesMethod method = PercentilesMethod.readFromStream(in); + return method.configFromStream(in); + } + + /** + * Deprecated: construct a {@link PercentilesConfig} directly instead + */ + @Deprecated + public static PercentilesConfig fromLegacy(PercentilesMethod method, double compression, int numberOfSignificantDigits) { + if (method.equals(PercentilesMethod.TDIGEST)) { + return new TDigest(compression); + } else if (method.equals(PercentilesMethod.HDR)) { + return new Hdr(numberOfSignificantDigits); + } + throw new IllegalArgumentException("Unsupported percentiles algorithm [" + method + "]"); + } + + public PercentilesMethod getMethod() { + return method; + } + + abstract Aggregator createPercentilesAggregator(String name, ValuesSource valuesSource, SearchContext context, Aggregator parent, + double[] values, boolean keyed, DocValueFormat formatter, + List pipelineAggregators, + Map metaData) throws IOException; + + abstract Aggregator createPercentileRanksAggregator(String name, ValuesSource valuesSource, SearchContext context, + Aggregator parent, double[] values, boolean keyed, + DocValueFormat formatter, List pipelineAggregators, + Map metaData) throws IOException; + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeEnum(method); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) return true; + if (obj == null || getClass() != obj.getClass()) return false; + + PercentilesConfig other = (PercentilesConfig) obj; + return method.equals(other.getMethod()); + } + + @Override + public int hashCode() { + return Objects.hash(method); + } + + public static class TDigest extends PercentilesConfig { + static final double DEFAULT_COMPRESSION = 100.0; + private double compression; + + TDigest() { + this(DEFAULT_COMPRESSION); + } + + TDigest(double compression) { + super(PercentilesMethod.TDIGEST); + setCompression(compression); + } + + TDigest(StreamInput in) throws IOException { + this(in.readDouble()); + } + + public void setCompression(double compression) { + if (compression < 0.0) { + throw new IllegalArgumentException( + "[compression] must be greater than or equal to 0. Found [" + compression + "]"); + } + this.compression = compression; + } + + public double getCompression() { + return compression; + } + + @Override + Aggregator createPercentilesAggregator(String name, ValuesSource valuesSource, SearchContext context, Aggregator parent, + double[] values, boolean keyed, DocValueFormat formatter, + List pipelineAggregators, + Map metaData) throws IOException { + return new TDigestPercentilesAggregator(name, valuesSource, context, parent, values, compression, keyed, formatter, + pipelineAggregators, metaData); + } + + @Override + Aggregator createPercentileRanksAggregator(String name, ValuesSource valuesSource, SearchContext context, Aggregator parent, + double[] values, boolean keyed, DocValueFormat formatter, + List pipelineAggregators, + Map metaData) throws IOException { + return new TDigestPercentileRanksAggregator(name, valuesSource, context, parent, values, compression, keyed, + formatter, pipelineAggregators, metaData); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + super.writeTo(out); + out.writeDouble(compression); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(getMethod().toString()); + builder.field(PercentilesMethod.COMPRESSION_FIELD.getPreferredName(), compression); + builder.endObject(); + return builder; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) return true; + if (obj == null || getClass() != obj.getClass()) return false; + if (super.equals(obj) == false) return false; + + TDigest other = (TDigest) obj; + return compression == other.getCompression(); + } + + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), compression); + } + } + + public static class Hdr extends PercentilesConfig { + static final int DEFAULT_NUMBER_SIG_FIGS = 3; + private int numberOfSignificantValueDigits; + + Hdr() { + this(DEFAULT_NUMBER_SIG_FIGS); + } + + Hdr(int numberOfSignificantValueDigits) { + super(PercentilesMethod.HDR); + setNumberOfSignificantValueDigits(numberOfSignificantValueDigits); + } + + Hdr(StreamInput in) throws IOException { + this(in.readVInt()); + } + + public void setNumberOfSignificantValueDigits(int numberOfSignificantValueDigits) { + if (numberOfSignificantValueDigits < 0 || numberOfSignificantValueDigits > 5) { + throw new IllegalArgumentException("[numberOfSignificantValueDigits] must be between 0 and 5"); + } + this.numberOfSignificantValueDigits = numberOfSignificantValueDigits; + } + + public int getNumberOfSignificantValueDigits() { + return numberOfSignificantValueDigits; + } + + @Override + Aggregator createPercentilesAggregator(String name, ValuesSource valuesSource, SearchContext context, Aggregator parent, + double[] values, boolean keyed, DocValueFormat formatter, + List pipelineAggregators, + Map metaData) throws IOException { + return new HDRPercentilesAggregator(name, valuesSource, context, parent, values, numberOfSignificantValueDigits, keyed, + formatter, pipelineAggregators, metaData); + } + + @Override + Aggregator createPercentileRanksAggregator(String name, ValuesSource valuesSource, SearchContext context, Aggregator parent, + double[] values, boolean keyed, DocValueFormat formatter, + List pipelineAggregators, + Map metaData) throws IOException { + return new HDRPercentileRanksAggregator(name, valuesSource, context, parent, values, numberOfSignificantValueDigits, keyed, + formatter, pipelineAggregators, metaData); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + super.writeTo(out); + out.writeVInt(numberOfSignificantValueDigits); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(getMethod().toString()); + builder.field(PercentilesMethod.NUMBER_SIGNIFICANT_DIGITS_FIELD.getPreferredName(), numberOfSignificantValueDigits); + builder.endObject(); + return builder; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) return true; + if (obj == null || getClass() != obj.getClass()) return false; + if (super.equals(obj) == false) return false; + + Hdr other = (Hdr) obj; + return numberOfSignificantValueDigits == other.getNumberOfSignificantValueDigits(); + } + + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), numberOfSignificantValueDigits); + } + } +} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesMethod.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesMethod.java index 3797e01e899..9ae83b6e982 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesMethod.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesMethod.java @@ -23,6 +23,7 @@ import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ObjectParser; import java.io.IOException; @@ -33,11 +34,36 @@ public enum PercentilesMethod implements Writeable { /** * The TDigest method for calculating percentiles */ - TDIGEST("tdigest", "TDigest", "TDIGEST"), + TDIGEST("tdigest", "TDigest", "TDIGEST") { + @Override + PercentilesConfig configFromStream(StreamInput in) throws IOException { + return new PercentilesConfig.TDigest(in); + } + }, /** * The HDRHistogram method of calculating percentiles */ - HDR("hdr", "HDR"); + HDR("hdr", "HDR") { + @Override + PercentilesConfig configFromStream(StreamInput in) throws IOException { + return new PercentilesConfig.Hdr(in); + } + }; + + public static final ParseField COMPRESSION_FIELD = new ParseField("compression"); + public static final ParseField NUMBER_SIGNIFICANT_DIGITS_FIELD = new ParseField("number_of_significant_value_digits"); + + public static final ObjectParser TDIGEST_PARSER; + static { + TDIGEST_PARSER = new ObjectParser<>(PercentilesMethod.TDIGEST.getParseField().getPreferredName(), PercentilesConfig.TDigest::new); + TDIGEST_PARSER.declareDouble(PercentilesConfig.TDigest::setCompression, COMPRESSION_FIELD); + } + + public static final ObjectParser HDR_PARSER; + static { + HDR_PARSER = new ObjectParser<>(PercentilesMethod.HDR.getParseField().getPreferredName(), PercentilesConfig.Hdr::new); + HDR_PARSER.declareInt(PercentilesConfig.Hdr::setNumberOfSignificantValueDigits, NUMBER_SIGNIFICANT_DIGITS_FIELD); + } private final ParseField parseField; @@ -45,6 +71,8 @@ public enum PercentilesMethod implements Writeable { this.parseField = new ParseField(name, deprecatedNames); } + abstract PercentilesConfig configFromStream(StreamInput in) throws IOException; + /** * @return the name of the method */ @@ -65,4 +93,5 @@ public enum PercentilesMethod implements Writeable { public String toString() { return parseField.getPreferredName(); } + } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentileRanksAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentileRanksAggregatorFactory.java deleted file mode 100644 index 65a9b9b6519..00000000000 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/TDigestPercentileRanksAggregatorFactory.java +++ /dev/null @@ -1,78 +0,0 @@ -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.elasticsearch.search.aggregations.metrics; - -import org.elasticsearch.index.query.QueryShardContext; -import org.elasticsearch.search.aggregations.Aggregator; -import org.elasticsearch.search.aggregations.AggregatorFactories; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; -import org.elasticsearch.search.aggregations.support.ValuesSource; -import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; -import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; -import org.elasticsearch.search.internal.SearchContext; - -import java.io.IOException; -import java.util.List; -import java.util.Map; - -class TDigestPercentileRanksAggregatorFactory - extends ValuesSourceAggregatorFactory { - - private final double[] percents; - private final double compression; - private final boolean keyed; - - TDigestPercentileRanksAggregatorFactory(String name, - ValuesSourceConfig config, - double[] percents, - double compression, - boolean keyed, - QueryShardContext queryShardContext, - AggregatorFactory parent, - AggregatorFactories.Builder subFactoriesBuilder, - Map metadata) throws IOException { - super(name, config, queryShardContext, parent, subFactoriesBuilder, metadata); - this.percents = percents; - this.compression = compression; - this.keyed = keyed; - } - - @Override - protected Aggregator createUnmapped(SearchContext searchContext, - Aggregator parent, - List pipelineAggregators, - Map metadata) throws IOException { - return new TDigestPercentileRanksAggregator(name, null, searchContext, parent, percents, compression, keyed, config.format(), - pipelineAggregators, metadata); - } - - @Override - protected Aggregator doCreateInternal(ValuesSource valuesSource, - SearchContext searchContext, - Aggregator parent, - boolean collectsFromSingleBucket, - List pipelineAggregators, - Map metadata) throws IOException { - return new TDigestPercentileRanksAggregator(name, valuesSource, searchContext, parent, - percents, compression, keyed, config.format(), pipelineAggregators, metadata); - } - -} diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesAggregatorTests.java index f08a89657c6..40ba3ef617b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesAggregatorTests.java @@ -34,10 +34,6 @@ import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.NumberFieldMapper; import org.elasticsearch.search.aggregations.AggregatorTestCase; -import org.elasticsearch.search.aggregations.metrics.HDRPercentilesAggregator; -import org.elasticsearch.search.aggregations.metrics.InternalHDRPercentiles; -import org.elasticsearch.search.aggregations.metrics.PercentilesAggregationBuilder; -import org.elasticsearch.search.aggregations.metrics.PercentilesMethod; import org.elasticsearch.search.aggregations.support.AggregationInspectionHelper; import java.io.IOException; @@ -45,6 +41,8 @@ import java.util.function.Consumer; import static java.util.Arrays.asList; import static java.util.Collections.singleton; +import static org.elasticsearch.search.aggregations.AggregationBuilders.percentiles; +import static org.hamcrest.Matchers.equalTo; public class HDRPercentilesAggregatorTests extends AggregatorTestCase { @@ -121,6 +119,18 @@ public class HDRPercentilesAggregatorTests extends AggregatorTestCase { }); } + public void testHdrThenTdigestSettings() throws Exception { + int sigDigits = randomIntBetween(1, 5); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> { + percentiles("percentiles") + .numberOfSignificantValueDigits(sigDigits) + .method(PercentilesMethod.HDR) + .compression(100.0) // <-- this should trigger an exception + .field("value"); + }); + assertThat(e.getMessage(), equalTo("Cannot set [compression] because the method has already been configured for HDRHistogram")); + } + private void testCase(Query query, CheckedConsumer buildIndex, Consumer verify) throws IOException { try (Directory directory = newDirectory()) { @@ -131,8 +141,14 @@ public class HDRPercentilesAggregatorTests extends AggregatorTestCase { try (IndexReader indexReader = DirectoryReader.open(directory)) { IndexSearcher indexSearcher = newSearcher(indexReader, true, true); - PercentilesAggregationBuilder builder = - new PercentilesAggregationBuilder("test").field("number").method(PercentilesMethod.HDR); + PercentilesAggregationBuilder builder; + // TODO this randomization path should be removed when the old settings are removed + if (randomBoolean()) { + builder = new PercentilesAggregationBuilder("test").field("number").method(PercentilesMethod.HDR); + } else { + PercentilesConfig hdr = new PercentilesConfig.Hdr(); + builder = new PercentilesAggregationBuilder("test").field("number").percentilesConfig(hdr); + } MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG); fieldType.setName("number"); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksTests.java index 6483dbbc6e3..67ee89db206 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksTests.java @@ -37,8 +37,7 @@ public class PercentileRanksTests extends BaseAggregationTestCase { + percentiles("percentiles") + .compression(100.0) + .method(PercentilesMethod.TDIGEST) + .numberOfSignificantValueDigits(sigDigits) // <-- this should trigger an exception + .field("value"); + }); + assertThat(e.getMessage(), equalTo("Cannot set [numberOfSignificantValueDigits] because the " + + "method has already been configured for TDigest")); + } + private void testCase(Query query, CheckedConsumer buildIndex, Consumer verify) throws IOException { try (Directory directory = newDirectory()) { @@ -152,8 +163,14 @@ public class TDigestPercentilesAggregatorTests extends AggregatorTestCase { try (IndexReader indexReader = DirectoryReader.open(directory)) { IndexSearcher indexSearcher = newSearcher(indexReader, true, true); - PercentilesAggregationBuilder builder = - new PercentilesAggregationBuilder("test").field("number").method(PercentilesMethod.TDIGEST); + PercentilesAggregationBuilder builder; + // TODO this randomization path should be removed when the old settings are removed + if (randomBoolean()) { + builder = new PercentilesAggregationBuilder("test").field("number").method(PercentilesMethod.TDIGEST); + } else { + PercentilesConfig hdr = new PercentilesConfig.TDigest(); + builder = new PercentilesAggregationBuilder("test").field("number").percentilesConfig(hdr); + } MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG); fieldType.setName("number"); diff --git a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregationBuilder.java b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregationBuilder.java index 866f6560d4b..daba298b19f 100644 --- a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregationBuilder.java +++ b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregationBuilder.java @@ -26,7 +26,7 @@ import java.io.IOException; import java.util.Map; import java.util.Objects; -import static org.elasticsearch.search.aggregations.metrics.PercentilesAggregationBuilder.COMPRESSION_FIELD; +import static org.elasticsearch.search.aggregations.metrics.PercentilesMethod.COMPRESSION_FIELD; public class BoxplotAggregationBuilder extends ValuesSourceAggregationBuilder.LeafOnly {