[hotfix][core] Optimize the merge logic lines calling in Histogram.java. #26486
Conversation
|
hi, @lvyanquan Could you. help take a look ? thx a lot ! |
| } else { | ||
| this.treeMap.put(entryFromOther.getKey(), entryFromOther.getValue() + ownValue); | ||
| } | ||
| this.treeMap.merge(entryFromOther.getKey(), entryFromOther.getValue(), Integer::sum); |
There was a problem hiding this comment.
can you confirm there is an existing unit test for this code please.
There was a problem hiding this comment.
@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. :)
There was a problem hiding this comment.
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.
What is the purpose of the change
[hotfix][core] Optimize the merge logic lines calling in Histogram.java.
Brief change log
[hotfix][core] Optimize the merge logic lines calling in Histogram.java.
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation