amogh-jahagirdar commented on code in PR #4953:
URL: https://github.com/apache/iceberg/pull/4953#discussion_r889123826
##########
api/src/main/java/org/apache/iceberg/util/CharSequenceSet.java:
##########
@@ -172,7 +172,7 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
- return Objects.hash(wrapperSet);
Review Comment:
1.) Yeah I was thinking about this. It would change the value of the
hashcode compared to using hash, because before we're doing an array allocation
and then hashing that, now we're doing the hash on the object itself. There's
an example of this
https://www.baeldung.com/java-objects-hash-vs-objects-hashcode .
However it still would retain the property that any equal objects would have
the same hashcode within the same JVM, which is sufficient. Across different
JVMs (in a distributed context) we aren't guaranteed consistent hashcode for
the same object. But this is an issue regardless of the method that's used
https://martin.kleppmann.com/2012/06/18/java-hashcode-unsafe-for-distributed-systems.html
2.) That's a good point, although I was kind of against suppressing mostly
because it seemed really unnecessary. In the case of more fields, I think it
makes sense to just tackle that then because presumably it would be part of a
bigger change anyways. I'm not sure about if for these classes new fields would
ever be added but of course, hard to predict that
Let me know what you think !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
