From 3067499a42e281b26cf05c8d0430bddbb581c850 Mon Sep 17 00:00:00 2001 From: fanwenqi Date: Mon, 30 Dec 2019 14:42:42 +0800 Subject: [PATCH 1/4] debug sum aggregator Change-Id: I6e4b1dfd590aea68dd092318edb11a2baa2f9d65 --- .../aggregations/metrics/SumAggregator.java | 72 ++++++++++++++----- 1 file changed, 55 insertions(+), 17 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregator.java index ebb0e36dbf5db..79814770c7b45 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregator.java @@ -18,11 +18,14 @@ */ package org.elasticsearch.search.aggregations.metrics; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.lucene.index.LeafReaderContext; 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.LongArray; import org.elasticsearch.index.fielddata.SortedNumericDoubleValues; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.Aggregator; @@ -39,10 +42,13 @@ class SumAggregator extends NumericMetricsAggregator.SingleValue { + private static final Logger LOG = LogManager.getLogger(SumAggregator.class); + private final ValuesSource.Numeric valuesSource; private final DocValueFormat format; private DoubleArray sums; + private LongArray sumsLong; private DoubleArray compensations; SumAggregator(String name, ValuesSource.Numeric valuesSource, DocValueFormat formatter, SearchContext context, @@ -52,6 +58,7 @@ class SumAggregator extends NumericMetricsAggregator.SingleValue { this.format = formatter; if (valuesSource != null) { sums = context.bigArrays().newDoubleArray(1, true); + sumsLong = context.bigArrays().newLongArray(1, true); compensations = context.bigArrays().newDoubleArray(1, true); } } @@ -74,23 +81,42 @@ public LeafBucketCollector getLeafCollector(LeafReaderContext ctx, @Override public void collect(int doc, long bucket) throws IOException { sums = bigArrays.grow(sums, bucket + 1); + sumsLong = bigArrays.grow(sumsLong, bucket + 1); compensations = bigArrays.grow(compensations, bucket + 1); if (values.advanceExact(doc)) { final int valuesCount = values.docValueCount(); - // Compute the sum of double values with Kahan summation algorithm which is more - // accurate than naive summation. - double sum = sums.get(bucket); - double compensation = compensations.get(bucket); - kahanSummation.reset(sum, compensation); - - for (int i = 0; i < valuesCount; i++) { - double value = values.nextValue(); - kahanSummation.add(value); - } + if (valuesSource.isFloatingPoint()) { + // Compute the sum of double values with Kahan summation algorithm which is more + // accurate than naive summation. + double sum = sums.get(bucket); + double compensation = compensations.get(bucket); + kahanSummation.reset(sum, compensation); + LOG.info("floating sums get sum: {}, compensation: {}", sum, compensation); + + for (int i = 0; i < valuesCount; i++) { + double value = values.nextValue(); + kahanSummation.add(value); + } - compensations.set(bucket, kahanSummation.delta()); - sums.set(bucket, kahanSummation.value()); + compensations.set(bucket, kahanSummation.delta()); + sums.set(bucket, kahanSummation.value()); + } else { + // Compute the sum of long values with naive summation. + long sum = sumsLong.get(bucket); + LOG.info("long sums get sum: {}, long sum: {}", sumsLong.get(bucket), sum); + for (int i = 0; i < valuesCount; i++) { + double doubleValue = values.nextValue(); + long value = (long) doubleValue; + sum += value; + LOG.info("summing... doubleValue: {}, value: {}, sum: {}", doubleValue, value, sum); + } + sumsLong.set(bucket, sum); +// long checkSum = sumsLong.get(bucket); +// if (checkSum != sum) { +// LOG.info("Parsing type warning, sum: {}, checkSum: {}", sum, checkSum); +// } + } } } }; @@ -98,18 +124,30 @@ public void collect(int doc, long bucket) throws IOException { @Override public double metric(long owningBucketOrd) { - if (valuesSource == null || owningBucketOrd >= sums.size()) { + if (valuesSource == null || owningBucketOrd >= sums.size() || owningBucketOrd >= sumsLong.size()) { return 0.0; } - return sums.get(owningBucketOrd); + if (valuesSource.isFloatingPoint()) { + LOG.info("get metric double sum = {}", sums.get(owningBucketOrd)); + return sums.get(owningBucketOrd); + } else { + LOG.info("get metric long sum = {}", sumsLong.get(owningBucketOrd)); + return sumsLong.get(owningBucketOrd); + } } @Override public InternalAggregation buildAggregation(long bucket) { - if (valuesSource == null || bucket >= sums.size()) { + if (valuesSource == null || bucket >= sums.size() || bucket >= sumsLong.size()) { return buildEmptyAggregation(); } - return new InternalSum(name, sums.get(bucket), format, pipelineAggregators(), metaData()); + if (valuesSource.isFloatingPoint()) { + LOG.info("build agg double sum = {}", sums.get(bucket)); + return new InternalSum(name, sums.get(bucket), format, pipelineAggregators(), metaData()); + } else { + LOG.info("build agg long sum = {}", sumsLong.get(bucket)); + return new InternalSum(name, sumsLong.get(bucket), format, pipelineAggregators(), metaData()); + } } @Override @@ -119,6 +157,6 @@ public InternalAggregation buildEmptyAggregation() { @Override public void doClose() { - Releasables.close(sums, compensations); + Releasables.close(sums, sumsLong, compensations); } } From 9055a61ef47b98e1a7f9d633eae58be71361978a Mon Sep 17 00:00:00 2001 From: fanwenqi Date: Tue, 31 Dec 2019 18:23:22 +0800 Subject: [PATCH 2/4] add long values into sum aggregations Change-Id: Ied28bb7b561fd58fdb438199a9be85ea91dc5536 --- .../aggregations/metrics/InternalSum.java | 83 +++++++++++++---- .../metrics/NumericMetricsAggregation.java | 4 + .../search/aggregations/metrics/Sum.java | 9 +- .../aggregations/metrics/SumAggregator.java | 93 ++++++++++--------- 4 files changed, 126 insertions(+), 63 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalSum.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalSum.java index 5778edb4da19a..2288607b2de1e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalSum.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalSum.java @@ -31,13 +31,26 @@ import java.util.Objects; public class InternalSum extends InternalNumericMetricsAggregation.SingleValue implements Sum { - private final double sum; + private final double doubleSum; + private final long longSum; + private final boolean isFloating; InternalSum(String name, double sum, DocValueFormat formatter, List pipelineAggregators, Map metaData) { super(name, pipelineAggregators, metaData); - this.sum = sum; + this.doubleSum = sum; + this.longSum = (long) sum; this.format = formatter; + this.isFloating = true; + } + + InternalSum(String name, long sum, DocValueFormat formatter, List pipelineAggregators, + Map metaData) { + super(name, pipelineAggregators, metaData); + this.doubleSum = (double) sum; + this.longSum = sum; + this.format = formatter; + this.isFloating = false; } /** @@ -46,13 +59,17 @@ public class InternalSum extends InternalNumericMetricsAggregation.SingleValue i public InternalSum(StreamInput in) throws IOException { super(in); format = in.readNamedWriteable(DocValueFormat.class); - sum = in.readDouble(); + doubleSum = in.readDouble(); + longSum = in.readLong(); + isFloating = in.readBoolean(); } @Override protected void doWriteTo(StreamOutput out) throws IOException { out.writeNamedWriteable(format); - out.writeDouble(sum); + out.writeDouble(doubleSum); + out.writeLong(longSum); + out.writeBoolean(isFloating); } @Override @@ -62,38 +79,66 @@ public String getWriteableName() { @Override public double value() { - return sum; + return doubleSum; + } + + @Override + public long longValue() { + return longSum; } @Override public double getValue() { - return sum; + return doubleSum; + } + + @Override + public long getLongValue() { + return longSum; } @Override public InternalSum reduce(List aggregations, ReduceContext reduceContext) { - // Compute the sum of double values with Kahan summation algorithm which is more - // accurate than naive summation. - CompensatedSum kahanSummation = new CompensatedSum(0, 0); - for (InternalAggregation aggregation : aggregations) { - double value = ((InternalSum) aggregation).sum; - kahanSummation.add(value); + if (isFloating) { + // Compute the sum of double values with Kahan summation algorithm which is more + // accurate than naive summation. + CompensatedSum kahanSummation = new CompensatedSum(0, 0); + for (InternalAggregation aggregation : aggregations) { + double value = ((InternalSum) aggregation).doubleSum; + kahanSummation.add(value); + } + return new InternalSum(name, kahanSummation.value(), format, pipelineAggregators(), getMetaData()); + } else { + // Compute the sum of long values with naive summation. + long sum = 0L; + for (InternalAggregation aggregation : aggregations) { + long value = ((InternalSum) aggregation).longSum; + sum += value; + } + return new InternalSum(name, sum, format, pipelineAggregators(), getMetaData()); } - return new InternalSum(name, kahanSummation.value(), format, pipelineAggregators(), getMetaData()); } @Override public XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { - builder.field(CommonFields.VALUE.getPreferredName(), sum); - if (format != DocValueFormat.RAW) { - builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), format.format(sum).toString()); + if (isFloating) { + builder.field(CommonFields.VALUE.getPreferredName(), doubleSum); + if (format != DocValueFormat.RAW) { + builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), format.format(doubleSum).toString()); + } + return builder; + } else { + builder.field(CommonFields.VALUE.getPreferredName(), longSum); + if (format != DocValueFormat.RAW) { + builder.field(CommonFields.VALUE_AS_STRING.getPreferredName(), format.format(longSum).toString()); + } + return builder; } - return builder; } @Override public int hashCode() { - return Objects.hash(super.hashCode(), sum); + return Objects.hash(super.hashCode(), doubleSum, longSum); } @Override @@ -103,6 +148,6 @@ public boolean equals(Object obj) { if (super.equals(obj) == false) return false; InternalSum that = (InternalSum) obj; - return Objects.equals(sum, that.sum); + return Objects.equals(doubleSum, that.doubleSum) && Objects.equals(longSum, that.longSum); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/NumericMetricsAggregation.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/NumericMetricsAggregation.java index 046b9825acc30..ad7966d04df00 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/NumericMetricsAggregation.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/NumericMetricsAggregation.java @@ -29,6 +29,10 @@ interface SingleValue extends NumericMetricsAggregation { String getValueAsString(); + default long longValue() { + return 0L; + } + } interface MultiValue extends NumericMetricsAggregation { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/Sum.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/Sum.java index f499b3ecc6ebd..68ca585253745 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/Sum.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/Sum.java @@ -24,7 +24,14 @@ public interface Sum extends NumericMetricsAggregation.SingleValue { /** - * The sum. + * The sum, default for double values. */ double getValue(); + + /** + * The sum for long values. + */ + default long getLongValue() { + return 0L; + } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregator.java index 79814770c7b45..a8f8aa854aa09 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregator.java @@ -21,6 +21,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.index.SortedNumericDocValues; import org.apache.lucene.search.ScoreMode; import org.elasticsearch.common.lease.Releasables; import org.elasticsearch.common.util.BigArrays; @@ -47,8 +48,8 @@ class SumAggregator extends NumericMetricsAggregator.SingleValue { private final ValuesSource.Numeric valuesSource; private final DocValueFormat format; - private DoubleArray sums; - private LongArray sumsLong; + private DoubleArray doubleSums; + private LongArray longSums; private DoubleArray compensations; SumAggregator(String name, ValuesSource.Numeric valuesSource, DocValueFormat formatter, SearchContext context, @@ -57,8 +58,8 @@ class SumAggregator extends NumericMetricsAggregator.SingleValue { this.valuesSource = valuesSource; this.format = formatter; if (valuesSource != null) { - sums = context.bigArrays().newDoubleArray(1, true); - sumsLong = context.bigArrays().newLongArray(1, true); + doubleSums = context.bigArrays().newDoubleArray(1, true); + longSums = context.bigArrays().newLongArray(1, true); compensations = context.bigArrays().newDoubleArray(1, true); } } @@ -75,24 +76,22 @@ public LeafBucketCollector getLeafCollector(LeafReaderContext ctx, return LeafBucketCollector.NO_OP_COLLECTOR; } final BigArrays bigArrays = context.bigArrays(); - final SortedNumericDoubleValues values = valuesSource.doubleValues(ctx); - final CompensatedSum kahanSummation = new CompensatedSum(0, 0); - return new LeafBucketCollectorBase(sub, values) { - @Override - public void collect(int doc, long bucket) throws IOException { - sums = bigArrays.grow(sums, bucket + 1); - sumsLong = bigArrays.grow(sumsLong, bucket + 1); - compensations = bigArrays.grow(compensations, bucket + 1); - - if (values.advanceExact(doc)) { - final int valuesCount = values.docValueCount(); - if (valuesSource.isFloatingPoint()) { + if (valuesSource.isFloatingPoint()) { + final SortedNumericDoubleValues values = valuesSource.doubleValues(ctx); + final CompensatedSum kahanSummation = new CompensatedSum(0, 0); + return new LeafBucketCollectorBase(sub, values) { + @Override + public void collect(int doc, long bucket) throws IOException { + doubleSums = bigArrays.grow(doubleSums, bucket + 1); + compensations = bigArrays.grow(compensations, bucket + 1); + + if (values.advanceExact(doc)) { + final int valuesCount = values.docValueCount(); // Compute the sum of double values with Kahan summation algorithm which is more // accurate than naive summation. - double sum = sums.get(bucket); + double sum = doubleSums.get(bucket); double compensation = compensations.get(bucket); kahanSummation.reset(sum, compensation); - LOG.info("floating sums get sum: {}, compensation: {}", sum, compensation); for (int i = 0; i < valuesCount; i++) { double value = values.nextValue(); @@ -100,53 +99,61 @@ public void collect(int doc, long bucket) throws IOException { } compensations.set(bucket, kahanSummation.delta()); - sums.set(bucket, kahanSummation.value()); - } else { + doubleSums.set(bucket, kahanSummation.value()); + LOG.debug("summing bucket is {}, {}", bucket, kahanSummation.value()); + } + } + }; + } else { + final SortedNumericDocValues values = valuesSource.longValues(ctx); + return new LeafBucketCollectorBase(sub, values) { + @Override + public void collect(int doc, long bucket) throws IOException { + longSums = bigArrays.grow(longSums, bucket + 1); + + if (values.advanceExact(doc)) { + final int valuesCount = values.docValueCount(); // Compute the sum of long values with naive summation. - long sum = sumsLong.get(bucket); - LOG.info("long sums get sum: {}, long sum: {}", sumsLong.get(bucket), sum); + long sum = longSums.get(bucket); + for (int i = 0; i < valuesCount; i++) { - double doubleValue = values.nextValue(); - long value = (long) doubleValue; + long value = values.nextValue(); sum += value; - LOG.info("summing... doubleValue: {}, value: {}, sum: {}", doubleValue, value, sum); } - sumsLong.set(bucket, sum); -// long checkSum = sumsLong.get(bucket); -// if (checkSum != sum) { -// LOG.info("Parsing type warning, sum: {}, checkSum: {}", sum, checkSum); -// } + + longSums.set(bucket, sum); + LOG.debug("summing bucket is {}, {}", bucket, sum); } } - } - }; + }; + } } @Override public double metric(long owningBucketOrd) { - if (valuesSource == null || owningBucketOrd >= sums.size() || owningBucketOrd >= sumsLong.size()) { + if (valuesSource == null || owningBucketOrd >= doubleSums.size() || owningBucketOrd >= longSums.size()) { return 0.0; } if (valuesSource.isFloatingPoint()) { - LOG.info("get metric double sum = {}", sums.get(owningBucketOrd)); - return sums.get(owningBucketOrd); + LOG.debug("get metric double sum = {}", doubleSums.get(owningBucketOrd)); + return doubleSums.get(owningBucketOrd); } else { - LOG.info("get metric long sum = {}", sumsLong.get(owningBucketOrd)); - return sumsLong.get(owningBucketOrd); + LOG.debug("get metric long sum = {}", longSums.get(owningBucketOrd)); + return (double) longSums.get(owningBucketOrd); } } @Override public InternalAggregation buildAggregation(long bucket) { - if (valuesSource == null || bucket >= sums.size() || bucket >= sumsLong.size()) { + if (valuesSource == null || bucket >= doubleSums.size() || bucket >= longSums.size()) { return buildEmptyAggregation(); } if (valuesSource.isFloatingPoint()) { - LOG.info("build agg double sum = {}", sums.get(bucket)); - return new InternalSum(name, sums.get(bucket), format, pipelineAggregators(), metaData()); + LOG.info("build agg double sum = {}", doubleSums.get(bucket)); + return new InternalSum(name, doubleSums.get(bucket), format, pipelineAggregators(), metaData()); } else { - LOG.info("build agg long sum = {}", sumsLong.get(bucket)); - return new InternalSum(name, sumsLong.get(bucket), format, pipelineAggregators(), metaData()); + LOG.info("build agg long sum = {}", longSums.get(bucket)); + return new InternalSum(name, longSums.get(bucket), format, pipelineAggregators(), metaData()); } } @@ -157,6 +164,6 @@ public InternalAggregation buildEmptyAggregation() { @Override public void doClose() { - Releasables.close(sums, sumsLong, compensations); + Releasables.close(doubleSums, longSums, compensations); } } From e89fcf1a377ce319f78c0f5f195b074aada41cc1 Mon Sep 17 00:00:00 2001 From: fanwenqi Date: Tue, 31 Dec 2019 19:25:46 +0800 Subject: [PATCH 3/4] remove long values of numeric metrics agg interface Change-Id: Iebd02310792eac05edb661816e1a455d148db23f --- .../search/aggregations/metrics/InternalSum.java | 10 ---------- .../metrics/NumericMetricsAggregation.java | 4 ---- .../elasticsearch/search/aggregations/metrics/Sum.java | 7 ------- 3 files changed, 21 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalSum.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalSum.java index 2288607b2de1e..6949c8ecf79f7 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalSum.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalSum.java @@ -82,21 +82,11 @@ public double value() { return doubleSum; } - @Override - public long longValue() { - return longSum; - } - @Override public double getValue() { return doubleSum; } - @Override - public long getLongValue() { - return longSum; - } - @Override public InternalSum reduce(List aggregations, ReduceContext reduceContext) { if (isFloating) { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/NumericMetricsAggregation.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/NumericMetricsAggregation.java index ad7966d04df00..046b9825acc30 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/NumericMetricsAggregation.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/NumericMetricsAggregation.java @@ -29,10 +29,6 @@ interface SingleValue extends NumericMetricsAggregation { String getValueAsString(); - default long longValue() { - return 0L; - } - } interface MultiValue extends NumericMetricsAggregation { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/Sum.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/Sum.java index 68ca585253745..02e6b272943bf 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/Sum.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/Sum.java @@ -27,11 +27,4 @@ public interface Sum extends NumericMetricsAggregation.SingleValue { * The sum, default for double values. */ double getValue(); - - /** - * The sum for long values. - */ - default long getLongValue() { - return 0L; - } } From 086a96d2bdd324e18ce2cb657d127c4742bc37b2 Mon Sep 17 00:00:00 2001 From: fanwenqi Date: Tue, 31 Dec 2019 20:22:54 +0800 Subject: [PATCH 4/4] add tests, remove debug of logs Change-Id: I4b93e948f0130727531cbbf9020168209e486941 --- .../aggregations/metrics/InternalSum.java | 5 +++ .../search/aggregations/metrics/Sum.java | 2 +- .../aggregations/metrics/SumAggregator.java | 10 ------ .../metrics/InternalSumTests.java | 33 +++++++++++++++++ .../metrics/SumAggregatorTests.java | 35 +++++++++++++++++++ 5 files changed, 74 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalSum.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalSum.java index 6949c8ecf79f7..d5fbdd55a03a4 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalSum.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalSum.java @@ -82,6 +82,11 @@ public double value() { return doubleSum; } + // For testing + public long longValue() { + return longSum; + } + @Override public double getValue() { return doubleSum; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/Sum.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/Sum.java index 02e6b272943bf..f499b3ecc6ebd 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/Sum.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/Sum.java @@ -24,7 +24,7 @@ public interface Sum extends NumericMetricsAggregation.SingleValue { /** - * The sum, default for double values. + * The sum. */ double getValue(); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregator.java index a8f8aa854aa09..c3f9f2182a2d2 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregator.java @@ -18,8 +18,6 @@ */ package org.elasticsearch.search.aggregations.metrics; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.index.SortedNumericDocValues; import org.apache.lucene.search.ScoreMode; @@ -43,8 +41,6 @@ class SumAggregator extends NumericMetricsAggregator.SingleValue { - private static final Logger LOG = LogManager.getLogger(SumAggregator.class); - private final ValuesSource.Numeric valuesSource; private final DocValueFormat format; @@ -100,7 +96,6 @@ public void collect(int doc, long bucket) throws IOException { compensations.set(bucket, kahanSummation.delta()); doubleSums.set(bucket, kahanSummation.value()); - LOG.debug("summing bucket is {}, {}", bucket, kahanSummation.value()); } } }; @@ -122,7 +117,6 @@ public void collect(int doc, long bucket) throws IOException { } longSums.set(bucket, sum); - LOG.debug("summing bucket is {}, {}", bucket, sum); } } }; @@ -135,10 +129,8 @@ public double metric(long owningBucketOrd) { return 0.0; } if (valuesSource.isFloatingPoint()) { - LOG.debug("get metric double sum = {}", doubleSums.get(owningBucketOrd)); return doubleSums.get(owningBucketOrd); } else { - LOG.debug("get metric long sum = {}", longSums.get(owningBucketOrd)); return (double) longSums.get(owningBucketOrd); } } @@ -149,10 +141,8 @@ public InternalAggregation buildAggregation(long bucket) { return buildEmptyAggregation(); } if (valuesSource.isFloatingPoint()) { - LOG.info("build agg double sum = {}", doubleSums.get(bucket)); return new InternalSum(name, doubleSums.get(bucket), format, pipelineAggregators(), metaData()); } else { - LOG.info("build agg long sum = {}", longSums.get(bucket)); return new InternalSum(name, longSums.get(bucket), format, pipelineAggregators(), metaData()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalSumTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalSumTests.java index 0fca0d43bd6f1..daeb9c891e014 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalSumTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalSumTests.java @@ -81,6 +81,29 @@ public void testSummationAccuracy() { verifySummationOfDoubles(largeValues, Double.NEGATIVE_INFINITY, 0d); } + public void testSummationAccuracyLong() { + // Summing up a normal array of long values + long[] longValues = new long[]{1, 17458313843517748L}; + verifySummationOfLongs(longValues, 17458313843517749L); + + // Double values precision + double[] doubleValues = new double[]{1, 17458313843517748d}; + verifySummationOfDoubles(doubleValues, 17458313843517748d, 0d); + + // Summing up an array which contains NaN and infinities and expect a result same as naive summation + long[] values; + int n = randomIntBetween(5, 10); + values = new long[n]; + long sum = 0; + for (int i = 0; i < n; i++) { + values[i] = frequently() + ? randomFrom(0L, Long.MIN_VALUE, Long.MAX_VALUE) + : randomLongBetween(Long.MIN_VALUE, Long.MAX_VALUE); + sum += values[i]; + } + verifySummationOfLongs(values, sum); + } + private void verifySummationOfDoubles(double[] values, double expected, double delta) { List aggregations = new ArrayList<>(values.length); for (double value : values) { @@ -91,6 +114,16 @@ private void verifySummationOfDoubles(double[] values, double expected, double d assertEquals(expected, reduced.value(), delta); } + private void verifySummationOfLongs(long[] values, long expected) { + List aggregations = new ArrayList<>(values.length); + for (long value : values) { + aggregations.add(new InternalSum("long1", value, null, null, null)); + } + InternalSum internalSum = new InternalSum("long", 0, null, null, null); + InternalSum reduced = internalSum.reduce(aggregations, null); + assertEquals(expected, reduced.longValue()); + } + @Override protected void assertFromXContent(InternalSum sum, ParsedAggregation parsedAggregation) { ParsedSum parsed = ((ParsedSum) parsedAggregation); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/SumAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/SumAggregatorTests.java index ff76aa4d0edef..0cfbcf92a93e2 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/SumAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/SumAggregatorTests.java @@ -164,6 +164,29 @@ public void testSummationAccuracy() throws IOException { verifySummationOfDoubles(largeValues, Double.NEGATIVE_INFINITY, 0d); } + public void testSummationAccuracyLong() throws IOException { + // Summing up a normal array of long values + long[] longValues = new long[]{1, 17458313843517748L}; + verifySummationOfLongs(longValues, 17458313843517749L); + + // Double values precision + double[] doubleValues = new double[]{1, 17458313843517748d}; + verifySummationOfDoubles(doubleValues, 17458313843517748d, 0d); + + // Summing up an array which contains NaN and infinities and expect a result same as naive summation + long[] values; + int n = randomIntBetween(5, 10); + values = new long[n]; + long sum = 0; + for (int i = 0; i < n; i++) { + values[i] = frequently() + ? randomFrom(0L, Long.MIN_VALUE, Long.MAX_VALUE) + : randomLongBetween(Long.MIN_VALUE, Long.MAX_VALUE); + sum += values[i]; + } + verifySummationOfLongs(values, sum); + } + private void verifySummationOfDoubles(double[] values, double expected, double delta) throws IOException { testCase(new MatchAllDocsQuery(), iw -> { @@ -176,6 +199,18 @@ private void verifySummationOfDoubles(double[] values, double expected, double d ); } + private void verifySummationOfLongs(long[] values, long expected) throws IOException { + testCase(new MatchAllDocsQuery(), + iw -> { + for (long value : values) { + iw.addDocument(singleton(new NumericDocValuesField(FIELD_NAME, value))); + } + }, + result -> assertEquals(expected, result.longValue()), + NumberFieldMapper.NumberType.LONG + ); + } + private void testCase(Query query, CheckedConsumer indexer, Consumer verify) throws IOException {