From 88d5189ad1f922505ac3fb5f17975926bd327563 Mon Sep 17 00:00:00 2001 From: Andy Bristol Date: Thu, 27 Sep 2018 15:57:37 -0700 Subject: [PATCH] only reduce_percentiles tests pass 500 iters --- .../aggregations/metrics/mad/InternalMAD.java | 25 ++------ .../metrics/mad/MADAggregationBuilder.java | 32 ++-------- .../metrics/mad/MADAggregator.java | 63 +++---------------- .../metrics/mad/MADAggregatorFactory.java | 9 +-- .../metrics/mad/MADAggregatorTests.java | 4 +- .../mad/MedianAbsoluteDeviationIT.java | 3 +- 6 files changed, 22 insertions(+), 114 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/InternalMAD.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/InternalMAD.java index 994cc7650b05d..dc7e686f4e408 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/InternalMAD.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/InternalMAD.java @@ -38,57 +38,42 @@ public class InternalMAD extends InternalNumericMetricsAggregation.SingleValue implements MedianAbsoluteDeviation { private final TDigestState valueSketch; - private final TDigestState deviationSketch; - private final String method; public InternalMAD(String name, List pipelineAggregators, Map metaData, DocValueFormat format, - TDigestState valueSketch, - TDigestState deviationSketch, - String method) { + TDigestState valueSketch) { super(name, pipelineAggregators, metaData); this.format = format; this.valueSketch = valueSketch; - this.deviationSketch = deviationSketch; - this.method = method; } public InternalMAD(StreamInput in) throws IOException { super(in); format = in.readNamedWriteable(DocValueFormat.class); valueSketch = TDigestState.read(in); - deviationSketch = TDigestState.read(in); - method = in.readString(); } @Override protected void doWriteTo(StreamOutput out) throws IOException { out.writeNamedWriteable(format); TDigestState.write(valueSketch, out); - TDigestState.write(deviationSketch, out); - out.writeString(method); } @Override public InternalAggregation doReduce(List aggregations, ReduceContext reduceContext) { TDigestState valueMerged = null; - TDigestState deviationMerged = null; for (InternalAggregation aggregation : aggregations) { final InternalMAD magAgg = (InternalMAD) aggregation; if (valueMerged == null) { valueMerged = new TDigestState(magAgg.valueSketch.compression()); } - if (deviationMerged == null) { - deviationMerged = new TDigestState(magAgg.deviationSketch.compression()); - } valueMerged.add(magAgg.valueSketch); - deviationMerged.add(magAgg.deviationSketch); } - return new InternalMAD(name, pipelineAggregators(), metaData, format, valueMerged, deviationSketch, method); + return new InternalMAD(name, pipelineAggregators(), metaData, format, valueMerged); } @Override @@ -114,9 +99,7 @@ protected int doHashCode() { @Override protected boolean doEquals(Object obj) { InternalMAD other = (InternalMAD) obj; - return Objects.equals(valueSketch, other.valueSketch) - && Objects.equals(deviationSketch, other.deviationSketch) - && Objects.equals(method, other.method); + return Objects.equals(valueSketch, other.valueSketch); } @Override @@ -132,6 +115,6 @@ public double value() { // todo maybe - compute this when the object is constructed so we don't have to build a new tdigest for the deviations every time @Override public double getMAD() { - return computeMAD(valueSketch, deviationSketch, method); + return computeMAD(valueSketch); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregationBuilder.java index 2de9da52fde4b..c7898104bb06a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregationBuilder.java @@ -48,9 +48,6 @@ public class MADAggregationBuilder extends LeafOnly METHODS = Arrays.asList("collection_median", "reduce_percentiles", "reduce_centroids"); // todo remove private static final ObjectParser PARSER; @@ -58,7 +55,6 @@ public class MADAggregationBuilder extends LeafOnly(NAME); ValuesSourceParserHelper.declareNumericFields(PARSER, true, true, false); // todo verify these arguments PARSER.declareDouble(MADAggregationBuilder::setCompression, COMPRESSION_FIELD); - PARSER.declareString(MADAggregationBuilder::setMethod, METHOD_FIELD); // todo remove } public static MADAggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { @@ -66,7 +62,6 @@ public static MADAggregationBuilder parse(String aggregationName, XContentParser } private double compression = 100.0d; - private String method = "collection_median"; // todo remove public MADAggregationBuilder(String name) { super(name, ValuesSourceType.NUMERIC, ValueType.NUMERIC); @@ -75,7 +70,6 @@ public MADAggregationBuilder(String name) { public MADAggregationBuilder(StreamInput in) throws IOException { super(in, ValuesSourceType.NUMERIC, ValueType.NUMERIC); compression = in.readDouble(); - method = in.readString(); //todo remove } protected MADAggregationBuilder(MADAggregationBuilder clone, @@ -83,7 +77,6 @@ protected MADAggregationBuilder(MADAggregationBuilder clone, Map metaData) { super(clone, factoriesBuilder, metaData); this.compression = clone.compression; - this.method = clone.method; // todo remove } /** @@ -105,19 +98,6 @@ public MADAggregationBuilder setCompression(double compression) { return this; } - public String getMethod() { // todo remove - return method; - } - - public MADAggregationBuilder setMethod(String method) { // todo remove - if (METHODS.contains(method) == false) { - throw new IllegalArgumentException("Invalid MAD method [" + method + "]"); - } - - this.method = method; - return this; - } - @Override protected AggregationBuilder shallowCopy(AggregatorFactories.Builder factoriesBuilder, Map metaData) { return new MADAggregationBuilder(this, factoriesBuilder, metaData); @@ -126,7 +106,6 @@ protected AggregationBuilder shallowCopy(AggregatorFactories.Builder factoriesBu @Override protected void innerWriteTo(StreamOutput out) throws IOException { out.writeDouble(compression); - out.writeString(method); // todo remove } @Override @@ -136,27 +115,24 @@ protected void innerWriteTo(StreamOutput out) throws IOException { AggregatorFactories.Builder subFactoriesBuilder) throws IOException { - return new MADAggregatorFactory(name, config, context, parent, subFactoriesBuilder, metaData, compression, method); - // todo remove method + return new MADAggregatorFactory(name, config, context, parent, subFactoriesBuilder, metaData, compression); } @Override protected XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { builder.field(COMPRESSION_FIELD.getPreferredName(), compression); - builder.field(METHOD_FIELD.getPreferredName(), method); // todo remove return builder; } @Override protected int innerHashCode() { - return Objects.hash(compression, method); - } // todo remove method + return Objects.hash(compression); + } @Override protected boolean innerEquals(Object obj) { MADAggregationBuilder other = (MADAggregationBuilder) obj; - return Objects.equals(compression, other.compression) - && Objects.equals(method, other.method); // todo remove + return Objects.equals(compression, other.compression); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregator.java index 670b2449a68fa..898fbcb40d6c9 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregator.java @@ -23,7 +23,6 @@ import org.apache.lucene.search.ScoreMode; import org.elasticsearch.common.lease.Releasables; import org.elasticsearch.common.util.BigArrays; -import org.elasticsearch.common.util.DoubleArray; import org.elasticsearch.common.util.ObjectArray; import org.elasticsearch.index.fielddata.SortedNumericDoubleValues; import org.elasticsearch.search.DocValueFormat; @@ -47,11 +46,8 @@ public class MADAggregator extends NumericMetricsAggregator.SingleValue { private final DocValueFormat format; private final double compression; - private final String method; // todo remove private ObjectArray valueSketches; - private ObjectArray deviationSketches; - private DoubleArray approximateMedians; public MADAggregator(String name, SearchContext context, @@ -60,8 +56,7 @@ public MADAggregator(String name, Map metaData, ValuesSource.Numeric valuesSource, DocValueFormat format, - double compression, - String method) throws IOException { // todo remov methhod + double compression) throws IOException { super(name, context, parent, pipelineAggregators, metaData); @@ -69,10 +64,7 @@ public MADAggregator(String name, this.format = format; this.valueSketches = context.bigArrays().newObjectArray(1); - this.deviationSketches = context.bigArrays().newObjectArray(1); - this.approximateMedians = context.bigArrays().newDoubleArray(1); this.compression = compression; - this.method = method; // todo remove } /* @@ -85,7 +77,7 @@ public double metric(long owningBucketOrd) { if (owningBucketOrd >= valueSketches.size() || valueSketches.get(owningBucketOrd) == null) { return Double.NaN; } else { - return computeMAD(valueSketches.get(owningBucketOrd), deviationSketches.get(owningBucketOrd), method); + return computeMAD(valueSketches.get(owningBucketOrd)); } } @@ -112,13 +104,6 @@ protected LeafBucketCollector getLeafCollector(LeafReaderContext ctx, LeafBucket public void collect(int doc, long bucket) throws IOException { valueSketches = bigArrays.grow(valueSketches, bucket + 1); - deviationSketches = bigArrays.grow(deviationSketches, bucket + 1); - - if (bucket >= approximateMedians.size()) { - final long from = approximateMedians.size(); - approximateMedians = bigArrays.grow(approximateMedians, bucket + 1); - approximateMedians.fill(from, approximateMedians.size(), Double.NEGATIVE_INFINITY); // todo use NaN? is neginf right - } TDigestState valueSketch = valueSketches.get(bucket); if (valueSketch == null) { @@ -126,27 +111,11 @@ public void collect(int doc, long bucket) throws IOException { valueSketches.set(bucket, valueSketch); } - TDigestState deviationSketch = deviationSketches.get(bucket); - if (deviationSketch == null) { - deviationSketch = new TDigestState(compression); - deviationSketches.set(bucket, deviationSketch); - } - - double approximateMedian = approximateMedians.get(bucket); - if (values.advanceExact(doc)) { final int valueCount = values.docValueCount(); for (int i = 0; i < valueCount; i++) { final double value = values.nextValue(); - if (approximateMedian == Double.NEGATIVE_INFINITY) { - approximateMedian = value; // when starting out, we set approx median to the first value - } valueSketch.add(value); - final double deviation = Math.abs(approximateMedian - value); - deviationSketch.add(deviation); - - approximateMedian = valueSketch.quantile(0.5); - approximateMedians.set(bucket, approximateMedian); } } } @@ -159,29 +128,24 @@ public InternalAggregation buildAggregation(long bucket) throws IOException { return buildEmptyAggregation(); } else { final TDigestState valueSketch = valueSketches.get(bucket); - final TDigestState deviationSketch = deviationSketches.get(bucket); - return new InternalMAD(name, pipelineAggregators(), metaData(), format, valueSketch, deviationSketch, method); + return new InternalMAD(name, pipelineAggregators(), metaData(), format, valueSketch); } } @Override public InternalAggregation buildEmptyAggregation() { - return new InternalMAD(name, pipelineAggregators(), metaData(), format, - new TDigestState(compression), new TDigestState(compression), method); + return new InternalMAD(name, pipelineAggregators(), metaData(), format, new TDigestState(compression)); } @Override public void doClose() { - Releasables.close(valueSketches, deviationSketches, approximateMedians); + Releasables.close(valueSketches); } // todo maybe this should live elsewhere - public static double computeMAD(TDigestState valuesSketch, TDigestState deviationsSketch, String method) { - if (method.equals("collection_median")) { + public static double computeMAD(TDigestState valuesSketch) { - return deviationsSketch.quantile(0.5); - - } else if (method.equals("reduce_percentiles")) { + if (valuesSketch.size() > 0) { final double approximateMedian = valuesSketch.quantile(0.5); final TDigestState approximateDeviationsSketch = new TDigestState(valuesSketch.compression()); @@ -194,19 +158,8 @@ public static double computeMAD(TDigestState valuesSketch, TDigestState deviatio } return approximateDeviationsSketch.quantile(0.5); - - } else if (method.equals("reduce_centroids")) { - final double approximateMedian = valuesSketch.quantile(0.5); - final TDigestState approximatedDeviationsSketch = new TDigestState(valuesSketch.compression()); - - valuesSketch.centroids().forEach(centroid -> { - final double deviation = Math.abs(approximateMedian - centroid.mean()); - approximatedDeviationsSketch.add(deviation, centroid.count()); - }); - - return approximatedDeviationsSketch.quantile(0.5); } else { - throw new IllegalStateException("Invalid MAD method [" + method + "]"); + return Double.NaN; } } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregatorFactory.java index a808b4a399a37..54e3d039f2af3 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregatorFactory.java @@ -35,7 +35,6 @@ public class MADAggregatorFactory extends ValuesSourceAggregatorFactory { private final double compression; - private final String method; // todo remove public MADAggregatorFactory(String name, ValuesSourceConfig config, @@ -43,11 +42,9 @@ public MADAggregatorFactory(String name, AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, Map metaData, - double compression, - String method) throws IOException { // todo remove method + double compression) throws IOException { super(name, config, context, parent, subFactoriesBuilder, metaData); this.compression = compression; - this.method = method; } @Override @@ -55,7 +52,7 @@ protected Aggregator createUnmapped(Aggregator parent, List pipelineAggregators, Map metaData) throws IOException { - return new MADAggregator(name, context, parent, pipelineAggregators, metaData, null, config.format(), compression, method); + return new MADAggregator(name, context, parent, pipelineAggregators, metaData, null, config.format(), compression); } @Override @@ -65,6 +62,6 @@ protected Aggregator doCreateInternal(ValuesSource.Numeric valuesSource, List pipelineAggregators, Map metaData) throws IOException { - return new MADAggregator(name, context, parent, pipelineAggregators, metaData, valuesSource, config.format(), compression, method); + return new MADAggregator(name, context, parent, pipelineAggregators, metaData, valuesSource, config.format(), compression); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregatorTests.java index 7779952044d9d..97713ecc12e65 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/mad/MADAggregatorTests.java @@ -149,8 +149,8 @@ private void testCase(Query query, IndexSearcher indexSearcher = newSearcher(indexReader, true, true); MADAggregationBuilder builder = new MADAggregationBuilder("mad") - .setMethod("reduce_centroids") - .field("number"); + .field("number") + .setCompression(randomDoubleBetween(20, 1000, true)); MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG); fieldType.setName("number"); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/mad/MedianAbsoluteDeviationIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/mad/MedianAbsoluteDeviationIT.java index 5a1d800fd4924..44a50a793c1cf 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/mad/MedianAbsoluteDeviationIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/mad/MedianAbsoluteDeviationIT.java @@ -150,9 +150,8 @@ protected Collection> nodePlugins() { private static MADAggregationBuilder randomBuilder() { final MADAggregationBuilder builder = new MADAggregationBuilder("mad"); if (randomBoolean()) { - builder.setCompression(randomDoubleBetween(0, 1000, false)); + builder.setCompression(randomDoubleBetween(20, 1000, false)); } - builder.setMethod("reduce_centroids"); return builder; }