Skip to content

[hotfix][core] Optimize the merge logic lines calling in Histogram.java. #26486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+3 −8
Open

[hotfix][core] Optimize the merge logic lines calling in Histogram.java. #26486

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions flink-core/src/main/java/org/apache/flink/api/common/accumulators/Histogram.java
View file
Original file line number Diff line number Diff line change
@@ -35,7 +35,7 @@ public class Histogram implements Accumulator<Integer, TreeMap<Integer, Integer>

private static final long serialVersionUID = 1L;

private TreeMap<Integer, Integer> treeMap = new TreeMap<Integer, Integer>();
private TreeMap<Integer, Integer> treeMap = new TreeMap<>();

@Override
public void add(Integer value) {
@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you confirm there is an existing unit test for this code please.

Copy link
Contributor Author

@RocMarshal RocMarshal Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidradl Thanks for the comment.
I looked into it and didn’t find any existing test cases. In my limited read, this API is a built-in method in the Java JDK, so theoretically, it shouldn’t have any issues. If we need to add a new test case to verify whether the Histogram#merge method works correctly, I’d be happy to do that. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the javadoc for merge if entryFromOther.getValue() is null, then the entry is removed, is this possible?
Testing the cases where null is our own Value (because the record does not exist) and null for the other value would be useful I think, as well as the happy path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @davidradl very much for your comments.
I have tried a few test cases and hope they can address your concerns.

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.

}
}

@@ -75,7 +70,7 @@ public String toString() {
@Override
public Accumulator<Integer, TreeMap<Integer, Integer>> clone() {
Histogram result = new Histogram();
result.treeMap = new TreeMap<Integer, Integer>(treeMap);
result.treeMap = new TreeMap<>(treeMap);
return result;
}
}