Skip to content

API: Fix equals and hashCode in CharSequenceSet#9245

Merged
aokolnychyi merged 1 commit intoapache:mainfrom
aokolnychyi:fix-charsequence-set-equality-and-hash-code
Dec 16, 2023
Merged

API: Fix equals and hashCode in CharSequenceSet#9245
aokolnychyi merged 1 commit intoapache:mainfrom
aokolnychyi:fix-charsequence-set-equality-and-hash-code

Conversation

@aokolnychyi
Copy link
Contributor

@aokolnychyi aokolnychyi commented Dec 7, 2023 •
edited
Loading

The equals and hashCode behaviors in CharSequenceSet contradict the Set API, which prohibits wrapping these sets into unmodifiable wrappers in CharSequenceMap#keySet.

/**
 * Compares the specified object with this set for equality.  Returns
 * {@code true} if the specified object is also a set, the two sets
 * have the same size, and every member of the specified set is
 * contained in this set (or equivalently, every member of this set is
 * contained in the specified set).  This definition ensures that the
 * equals method works properly across different implementations of the
 * set interface.
 *
 * @param o object to be compared for equality with this set
 * @return {@code true} if the specified object is equal to this set
 */
boolean equals(Object o);

/**
 * Returns the hash code value for this set.  The hash code of a set is
 * defined to be the sum of the hash codes of the elements in the set,
 * where the hash code of a {@code null} element is defined to be zero.
 * This ensures that {@code s1.equals(s2)} implies that
 * {@code s1.hashCode()==s2.hashCode()} for any two sets {@code s1}
 * and {@code s2}, as required by the general contract of
 * {@link Object#hashCode}.
 *
 * @return the hash code value for this set
 * @see Object#equals(Object)
 * @see Set#equals(Object)
 */
int hashCode();

@github-actions github-actions bot added the API label Dec 7, 2023
aokolnychyi
aokolnychyi commented Dec 7, 2023
View reviewed changes
api/src/test/java/org/apache/iceberg/util/TestCharSequenceSet.java
set2.add(new StringBuffer("v2"));
set2.add("v3");

Set<CharSequence> set3 = Collections.unmodifiableSet(set2);
Copy link
Contributor Author

@aokolnychyi aokolnychyi Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am primarily interested in wrapping CharSequenceSet into UnmodifiableSet in CharSequenceMap.

@aokolnychyi aokolnychyi force-pushed the fix-charsequence-set-equality-and-hash-code branch from f914a5f to b9b9561 Compare December 8, 2023 01:42
aokolnychyi
aokolnychyi commented Dec 8, 2023
View reviewed changes
api/src/main/java/org/apache/iceberg/util/CharSequenceSet.java
@Override
public boolean equals(Object o) {
if (this == o) {
public boolean equals(Object other) {
Copy link
Contributor Author

@aokolnychyi aokolnychyi Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See Set#equals() and AbstractSet#equals() for background.

aokolnychyi
aokolnychyi commented Dec 8, 2023
View reviewed changes
api/src/main/java/org/apache/iceberg/util/CharSequenceSet.java
return wrapperSet.equals(that.wrapperSet);
try {
return containsAll(that);
} catch (ClassCastException | NullPointerException unused) {
Copy link
Contributor Author

@aokolnychyi aokolnychyi Dec 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this class does not throw these exceptions now, it may in the future. These exceptions are valid outcomes defined by the Set API. Therefore, I am respecting that to be safe.

aokolnychyi
aokolnychyi commented Dec 8, 2023
View reviewed changes
api/src/main/java/org/apache/iceberg/util/CharSequenceSet.java
@Override
public int hashCode() {
return Objects.hashCode(wrapperSet);
return wrapperSet.stream().mapToInt(CharSequenceWrapper::hashCode).sum();
Copy link
Contributor Author

@aokolnychyi aokolnychyi Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Set API requires the hash code to be a sum of hash codes of all elements. See Set#hashCode().

aokolnychyi
aokolnychyi commented Dec 8, 2023
View reviewed changes
api/src/test/java/org/apache/iceberg/util/TestCharSequenceSet.java

Set<CharSequence> set3 = Collections.unmodifiableSet(set2);

Set<CharSequenceWrapper> set4 =
Copy link
Contributor Author

@aokolnychyi aokolnychyi Dec 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one works too now.

@aokolnychyi aokolnychyi changed the title API: Fix equals and hashCode behavior in CharSequenceSet API: Fix equals and hashCode in CharSequenceSet Dec 8, 2023
szehon-ho
szehon-ho approved these changes Dec 15, 2023
View reviewed changes
Copy link
Member

@szehon-ho szehon-ho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Another option is to extend java AbstractSet, wondering if we considered it?

@aokolnychyi
Copy link
Contributor Author

aokolnychyi commented Dec 16, 2023

@szehon-ho, I started by extending AbstractSet. Unfortunately, hashCode does not work. It is implemented using the iterator that in our case returns unwrapped elements and breaks the contract.

@aokolnychyi aokolnychyi merged commit d6a4ca7 into apache:main Dec 16, 2023
@aokolnychyi
Copy link
Contributor Author

aokolnychyi commented Dec 16, 2023

Thanks, @szehon-ho!

@ajantha-bhat
Copy link
Member

ajantha-bhat commented Dec 21, 2023

I think this PR has produced error prone warnings.

image

lisirrx pushed a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024
@aokolnychyi @lisirrx
API: Fix equals and hashCode in CharSequenceSet (apache#9245)
cc03d0b
geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024
@aokolnychyi @geruh
API: Fix equals and hashCode in CharSequenceSet (apache#9245)
50ae660
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
@aokolnychyi @devangjhabakh
API: Fix equals and hashCode in CharSequenceSet (apache#9245)
b7c513c
@aokolnychyi aokolnychyi mentioned this pull request Jan 10, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@szehon-ho szehon-ho szehon-ho approved these changes

Assignees

No one assigned

Labels

API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aokolnychyi @ajantha-bhat @szehon-ho