[GitHub] flink pull request #2639: [FLINK-4586] [core] Broken AverageAccumulator

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/flink/pull/2639


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2639: [FLINK-4586] [core] Broken AverageAccumulator

[greghogan]

Github user greghogan commented on a diff in the pull request:

https://github.com/apache/flink/pull/2639#discussion_r84054277
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/api/common/accumulators/AverageAccumulator.java
 ---
@@ -28,51 +28,52 @@
 public class AverageAccumulator implements SimpleAccumulator {
 
private static final long serialVersionUID = 3672555084179165255L;
-   
-   private double localValue;
+
private long count;
 
+   private double sum;
+
@Override
public void add(Double value) {
this.count++;
-   this.localValue += value;
+   this.sum += value;
}
 
public void add(double value) {
this.count++;
-   this.localValue += value;
+   this.sum += value;
}
 
public void add(long value) {
this.count++;
-   this.localValue += value;
+   this.sum += value;
}
 
public void add(int value) {
this.count++;
-   this.localValue += value;
+   this.sum += value;
}
 
@Override
public Double getLocalValue() {
if (this.count == 0) {
return 0.0;
}
-   return this.localValue / (double)this.count;
+   return this.sum / this.count;
}
 
@Override
public void resetLocal() {
this.count = 0;
-   this.localValue = 0;
+   this.sum = 0;
}
 
@Override
public void merge(Accumulator other) {
if (other instanceof AverageAccumulator) {
-   AverageAccumulator temp = (AverageAccumulator)other;
-   this.count += temp.count;
-   this.localValue += other.getLocalValue();
--- End diff --

Yes, and the test did not catch the bug since it was only merging the 
average of two single values (where the sum is the average).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2639: [FLINK-4586] [core] Broken AverageAccumulator

[StephanEwen]

Github user StephanEwen commented on a diff in the pull request:

https://github.com/apache/flink/pull/2639#discussion_r83643344
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/api/common/accumulators/AverageAccumulator.java
 ---
@@ -28,51 +28,52 @@
 public class AverageAccumulator implements SimpleAccumulator {
 
private static final long serialVersionUID = 3672555084179165255L;
-   
-   private double localValue;
+
private long count;
 
+   private double sum;
+
@Override
public void add(Double value) {
this.count++;
-   this.localValue += value;
+   this.sum += value;
}
 
public void add(double value) {
this.count++;
-   this.localValue += value;
+   this.sum += value;
}
 
public void add(long value) {
this.count++;
-   this.localValue += value;
+   this.sum += value;
}
 
public void add(int value) {
this.count++;
-   this.localValue += value;
+   this.sum += value;
}
 
@Override
public Double getLocalValue() {
if (this.count == 0) {
return 0.0;
}
-   return this.localValue / (double)this.count;
+   return this.sum / this.count;
}
 
@Override
public void resetLocal() {
this.count = 0;
-   this.localValue = 0;
+   this.sum = 0;
}
 
@Override
public void merge(Accumulator other) {
if (other instanceof AverageAccumulator) {
-   AverageAccumulator temp = (AverageAccumulator)other;
-   this.count += temp.count;
-   this.localValue += other.getLocalValue();
--- End diff --

I guess this was the buggy line?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request #2639: [FLINK-4586] [core] Broken AverageAccumulator

[greghogan]

GitHub user greghogan opened a pull request:

https://github.com/apache/flink/pull/2639

[FLINK-4586] [core] Broken AverageAccumulator



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/greghogan/flink 4586_broken_averageaccumulator

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/2639.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2639


commit 58d79b40ed837542e727c5f4f3a410ccb5f9ff24
Author: Greg Hogan 
Date:   2016-10-14T20:18:52Z

[FLINK-4586] [core] Broken AverageAccumulator




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---