fix: Define equality behavior for EvaluationContext implementations #1756
Conversation
Summary of ChangesHello @Vishalup29, 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 addresses a reported issue where Highlights
Using Gemini Code AssistThe 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
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 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
|
Vishalup29
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR adds consistent equals and hashCode implementations to ImmutableContext and LayeredEvaluationContext, which is a great improvement for SDK users. The logic to base equality on the resolved map from asMap() is correct and aligns with user expectations.
My review focuses on two main areas:
- Performance: The current implementations of
equalsandhashCodecallasMap()on every invocation. This can be inefficient, particularly forLayeredEvaluationContext. I've suggested caching strategies for both classes to mitigate potential performance bottlenecks. - Testing: One of the new tests can be improved to more directly validate the
equalscontract between different context types.
Additionally, I noticed that ImmutableContext still has a Lombok @EqualsAndHashCode annotation. Since you've provided a manual implementation, this annotation is now misleading and should be removed to improve code clarity, though I was unable to comment on that line directly as it was not part of the diff.
| public boolean equals(Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (!(o instanceof EvaluationContext)) { | ||
| return false; | ||
| } | ||
|
|
||
| EvaluationContext that = (EvaluationContext) o; | ||
| return this.asMap().equals(that.asMap()); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return asMap().hashCode(); | ||
| } |
There was a problem hiding this comment.
The logic here is correct, but it has a significant performance implication. The asMap() method on LayeredEvaluationContext is expensive, as it constructs a new map by merging all context layers every time it's called. Calling this in equals() and hashCode() can lead to performance bottlenecks, especially if these objects are used in collections.
I recommend caching the resolved map. The cache can be invalidated in putHookContext(), similar to how keySet is handled.
Here's a potential refactoring:
- Introduce a
private Map<String, Value> cachedMap;field. - Create a private helper method to get or build the map.
- Update
asMap(),equals(), andhashCode()to use the helper. - Invalidate
cachedMapinputHookContext().
Example:
// Add this field
private Map<String, Value> cachedMap;
// New helper method
private Map<String, Value> getResolvedMap() {
if (cachedMap == null) {
// logic from asMap() to build the map
HashMap<String, Value> map = new HashMap<>();
// ... build map ...
cachedMap = map;
}
return cachedMap;
}
@Override
public Map<String, Value> asMap() {
// Return a copy to prevent mutation of the cached map
return new HashMap<>(getResolvedMap());
}
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof EvaluationContext)) {
return false;
}
EvaluationContext that = (EvaluationContext) o;
return this.getResolvedMap().equals(that.asMap());
}
@Override
public int hashCode() {
return getResolvedMap().hashCode();
}
void putHookContext(EvaluationContext context) {
// ... existing logic
this.cachedMap = null; // Invalidate cache
this.keySet = null;
}This change would greatly improve the performance of equals and hashCode.
| public int hashCode() { | ||
| return asMap().hashCode(); | ||
| } |
There was a problem hiding this comment.
This implementation is correct, but asMap() may be an expensive call if this object is used frequently in hash-based collections. Since ImmutableContext is immutable, you could cache the hash code after its first computation to improve performance. This is a common optimization for immutable objects.
For example, you could introduce a private volatile Integer cachedHashCode; field and use lazy initialization in a thread-safe way:
private volatile Integer cachedHashCode;
@Override
public int hashCode() {
if (cachedHashCode == null) {
synchronized (this) {
if (cachedHashCode == null) {
cachedHashCode = asMap().hashCode();
}
}
}
return cachedHashCode;
}| // Compare resolved attributes, not the context objects | ||
| assertEquals(base.asMap(), layered.asMap()); |
There was a problem hiding this comment.
This test correctly sets up a scenario for mixed-context equality, but it only verifies that the asMap() representations are equal. To properly test the new equals implementation, you should directly compare the context objects. The equals method is designed to perform this map-based comparison internally. You should also assert that the hash codes are equal, to fully validate the contract between equals and hashCode.
// Compare resolved attributes via the equals method, which should compare the underlying maps.
assertEquals(base, layered);
assertEquals(layered, base);
assertEquals(base.hashCode(), layered.hashCode());
Vishalup29
force-pushed
the
issue_1754
branch
from
b826418 to
2b7ec29
Compare
…xt and add tests. Signed-off-by: vishalup29 <vishalupadhyay977@gmail.com>
Vishalup29
force-pushed
the
issue_1754
branch
from
2b7ec29 to
aae0dfa
Compare
|
|
@Vishalup29 Gemini's comments here are accurate, I think. Lazily building and caching this map is probably a good idea, instead of doing it again with every call... Though I think there's memory trade offs here as well... The comment about the Lombok impl is also right, I think that needs to be removed. cc @chrfwow because I'd like your insights here as well. What do you think? I think either way we'll generate a heap object doing 'asmap' during 'hashcode', so we might as well cache it in case we need it again, right? I think this is optimal as long as we do it lazily? |
|
Yes, we should definetly cache the map when we create it. I have to say I don't really like that the first invocation of |
Yes, I feel exactly the same way. It will be expensive, and allocate memory once, but I think it's still worth having. At least the use case we know of it was just used in test assertions - specifically to validate/assert in a test that a context was set... perhaps this will be the most common use-case. |
3 participants
Description
This PR adds consistent
equalsandhashCodeimplementations to the SDK’sEvaluationContextimplementations (ImmutableContextandLayeredEvaluationContext).A user reported in Issue #1754 that comparing context instances did not behave as expected,
especially after the introduction of
LayeredEvaluationContext. Because the SDK does notguarantee object identity, equality should be defined based on the resolved attribute map
returned by
asMap().This PR implements that behavior and adds regression tests to ensure we maintain consistent
equality semantics going forward.
Changes
equals(Object)andhashCode()to:ImmutableContextLayeredEvaluationContextasMap()representations.ImmutableContextinstances.LayeredEvaluationContextinstances.ImmutableContextandLayeredEvaluationContext.Related Issue
Fixes #1754
Notes
asMap()rather than structural depth or source layering.