RocMarshal commented on code in PR #26486:
URL: https://github.com/apache/flink/pull/26486#discussion_r2061975346
##########
flink-core/src/main/java/org/apache/flink/api/common/accumulators/Histogram.java:
##########
@@ -53,12 +53,7 @@ public TreeMap<Integer, Integer> getLocalValue() {
public void merge(Accumulator<Integer, TreeMap<Integer, Integer>> other) {
// Merge the values into this map
for (Map.Entry<Integer, Integer> entryFromOther :
other.getLocalValue().entrySet()) {
- Integer ownValue = this.treeMap.get(entryFromOther.getKey());
- if (ownValue == null) {
- this.treeMap.put(entryFromOther.getKey(),
entryFromOther.getValue());
- } else {
- this.treeMap.put(entryFromOther.getKey(),
entryFromOther.getValue() + ownValue);
- }
+ this.treeMap.merge(entryFromOther.getKey(),
entryFromOther.getValue(), Integer::sum);
Review Comment:
Thank you @davidradl very much for your comments.
I have tried a few test cases and hope they can address your concerns.
```java
class HistogramTest {
@Test
void testMerge() {
System.out.println("Check for Histogram: left value is null, right
value to merge is not null");
Histogram hleft1 = new Histogram();
Histogram hright1 = new Histogram();
hright1.add(1);
hleft1.merge(hright1);
System.out.println(hleft1);
System.out.println("Check for Histogram: left value is not null,
right value to merge is null");
Histogram hleft2 = new Histogram();
hleft2.add(1);
Histogram hright2 = new Histogram();
hleft2.merge(hright2);
System.out.println(hleft2);
System.out.println("Check for TreeMap: left value is null, right
value to merge is not null");
TreeMap<Integer, Integer> tmLeft1 = new TreeMap<>() {
{
put(1, null);
}
};
tmLeft1.merge(1, 1, Integer::sum);
System.out.println(tmLeft1);
System.out.println("Check for TreeMap: left value is not null, right
value to merge is null");
TreeMap<Integer, Integer> tmLeft2 = new TreeMap<>() {
{
put(1, 1);
}
};
try {
tmLeft2.merge(1, null, Integer::sum);
} catch (NullPointerException e) {
System.out.println("The null value will not be existed in
Histogram, so the case could be ignored here.");
}
System.out.println("Check for TreeMap When the entry pair is delete:
When remappingFunc is returning null");
TreeMap<Integer, Integer> tmLeft3 = new TreeMap<>() {
{
put(1, 1);
}
};
tmLeft3.merge(1, 1, (ignore1, ignore2) -> null);
System.out.println(tmLeft3);
}
}
```
The result:
```
Check for Histogram: left value is null, right value to merge is not null
{1=1}
Check for Histogram: left value is not null, right value to merge is null
{1=1}
Check for TreeMap: left value is null, right value to merge is not null
{1=1}
Check for TreeMap: left value is not null, right value to merge is null
{1=1}
Check for TreeMap When the entry pair is delete: When remappingFunc is
returning null
{}
```
In short, the merge method with Integer::sum will not return a null value
and will preserve the same semantics as the original code.
Therefore, in Histogram#merge, no entry will be removed.
I'm happy to add test cases for it if needed.
Please correct me if my understanding is wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
