-
Notifications
You must be signed in to change notification settings - Fork 112
Fix ResolvedKeySpacePath.equals and hashCode #3591
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.
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
base: main
Are you sure you want to change the base?
Conversation
Required tests on PathValue too
.../java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java
Outdated
Show resolved
Hide resolved
...main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java
Outdated
Show resolved
Hide resolved
.../java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java
Show resolved
Hide resolved
...rc/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java
Show resolved
Hide resolved
...a/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java
Show resolved
Hide resolved
...a/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java
Outdated
Show resolved
Hide resolved
...a/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java
Show resolved
Hide resolved
This will be useful when comparing paths, because they can validate the parent, and don't care about the directiory's other children.
// Handle ANY_VALUE specially - typeOf does not support ANY_VALUE | ||
boolean isAnyValue = (o1 == ANY_VALUE || o2 == ANY_VALUE); | ||
if (isAnyValue) { | ||
return Objects.equals(o1, o2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a defensive coding we can add equals
and hashCode
to AnyValue
, though all reasonable implementations should use the static constant...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but AnyValue
is a private-nested class, so KeySpaceDirectory
is the only way to construct one, so you really shouldn't be accessing it other than via the constant.
getDirectory().getName(), | ||
getDirectory().getValue(), | ||
getValue(), | ||
KeySpaceDirectory.valueHashCode(getValue()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be the same as getDirectory().getValue()
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if the directory is a constant. If the getDirectory().getValue()
is ANY_VALUE
, this would be any value of that type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case there may be a discrepancy where equals
(line 281) compares:
- keyspacePath.value
KeyPathDirectory.equalsIgnoringHierarchy
->- name
- KeyType
- directory.value
Whereas hashCode
compares:
- name
- keyType
- keyspacePath.value
So it looks as if the hashCode
could benefit from the extra hash for directory.value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, that is probably true.
It's not catastrophic to have hashCode
compare less than equals
, especially given that in standard use cases, if everything else is equal, the directory value should be too.
That being said, it would probably be confusing to anyone else coming upon this, so at least it should have a comment.
This fixes
ResolvedKeySpace.equals
andhashCode
so that they follow the spec and are consistent.Notably there were a few issues:
KeyType.BYTES
PathValue
, while equals looked at the value)PathValue
did not implementequals
/hashCode
remainder
was not included.Fixes: #3594