feat: equivalent EvaluationContext impls are .equal - #1771
Conversation
Sorry, something went wrong.
Summary of ChangesHello @chrfwow, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the core EvaluationContext implementations, ImmutableContext and LayeredEvaluationContext, to provide robust and correct equals and hashCode behavior. The primary goal is to ensure that context objects are compared and hashed consistently based on their resolved attributes, even when dealing with complex layered or nested structures. This change is crucial for reliable context management and feature evaluation within the SDK. Highlights
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here. Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request refactors the ImmutableContext and LayeredEvaluationContext classes to improve performance and correctness, particularly concerning object equality and hashing. For ImmutableContext, the Lombok-generated equals and hashCode methods were replaced with custom implementations that lazily compute and cache the hash code based on the context's unmodifiable map. For LayeredEvaluationContext, a cachedMap was introduced to store the resolved attribute map, which is lazily computed and invalidated when underlying contexts change, optimizing repeated map access. Custom equals and hashCode methods were also added for LayeredEvaluationContext, relying on this cached map. The review comment suggests an optimization for LayeredEvaluationContext's map creation, recommending to call ensureKeySet() before initializing the HashMap for the cachedMap to pre-calculate the required capacity and prevent potential HashMap resizing, thus improving performance. New tests were added to validate these changes, including equality and hash code stability for both context types, and behavior of asObjectMap() for LayeredEvaluationContext.
Sorry, something went wrong.
| if (keySet != null && keySet.isEmpty()) { | ||
| return new HashMap<>(0); | ||
| cachedMap = Collections.emptyMap(); | ||
| return cachedMap; | ||
| } | ||
|
|
||
| HashMap<String, Value> map; | ||
| if (keySet != null) { | ||
| map = new HashMap<>(keySet.size()); | ||
| // use helper to size the map based on expected entries | ||
| map = HashMapUtils.forEntries(keySet.size()); | ||
| } else { | ||
| map = new HashMap<>(); | ||
| } |
There was a problem hiding this comment.
To improve performance and avoid potential HashMap resizing, it's better to compute the full set of keys before initializing the map. By calling ensureKeySet() upfront, you can guarantee that the HashMap is created with the correct initial capacity to hold all entries, which is more efficient than potentially creating a default-sized map that needs to be rehashed.
ensureKeySet();
if (this.keySet.isEmpty()) {
cachedMap = Collections.emptyMap();
return cachedMap;
}
// use helper to size the map based on expected entries
HashMap<String, Value> map = HashMapUtils.forEntries(this.keySet.size());
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 80.00000% with 9 lines in your changes missing coverage. Please review. @@ Coverage Diff @@
## main #1771 +/- ##
============================================
+ Coverage 92.53% 93.22% +0.69%
- Complexity 599 617 +18
============================================
Files 55 55
Lines 1406 1446 +40
Branches 154 161 +7
============================================
+ Hits 1301 1348 +47
+ Misses 64 54 -10
- Partials 41 44 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
This reverts commit 4cb39a4. Co-Authored-By: Vishalup29 <67001661+Vishalup29@users.noreply.github.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
|
Sorry, something went wrong.
This PR
Re-revert of #1768 with associated fixes: The object map of the LayeredEvaluationContext needs to be computed by calling the asObjectMap method of the layered contexts. Otherwise, nested EvaluationContexts will not get unwrapped (i.e. converted to HashMaps)
Related Issues
Fixes #1754
See also open-feature/java-sdk-contrib#1665