Skip to content
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

Core: Fix equality in StructLikeMap #9236

Merged
merged 2 commits into from
Dec 6, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions api/src/test/java/org/apache/iceberg/TestHelpers.java
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,52 @@ public int hashCode() {
}
}

// similar to Row but has its own hashCode() and equals() implementations
// it is useful for testing custom collections that rely on wrappers
public static class CustomRow implements StructLike {
public static CustomRow of(Object... values) {
return new CustomRow(values);
}

private final Object[] values;

private CustomRow(Object... values) {
this.values = values;
}

@Override
public int size() {
return values.length;
}

@Override
public <T> T get(int pos, Class<T> javaClass) {
return javaClass.cast(values[pos]);
}

@Override
public <T> void set(int pos, T value) {
values[pos] = value;
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
} else if (other == null || getClass() != other.getClass()) {
return false;
}

CustomRow that = (CustomRow) other;
return Arrays.equals(values, that.values);
}

@Override
public int hashCode() {
return 17 * Arrays.hashCode(values);
}
}

public static class TestFieldSummary implements ManifestFile.PartitionFieldSummary {
private final boolean containsNull;
private final Boolean containsNaN;
Expand Down
21 changes: 7 additions & 14 deletions core/src/main/java/org/apache/iceberg/util/StructLikeMap.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import java.util.AbstractMap;
import java.util.Collection;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.Function;
import org.apache.iceberg.StructLike;
Expand Down Expand Up @@ -128,9 +127,9 @@ public Set<Entry<StructLike, T>> entrySet() {

private static class StructLikeEntry<R> implements Entry<StructLike, R> {

private Map.Entry<StructLikeWrapper, R> inner;
private final Entry<StructLikeWrapper, R> inner;

private StructLikeEntry(Map.Entry<StructLikeWrapper, R> inner) {
private StructLikeEntry(Entry<StructLikeWrapper, R> inner) {
this.inner = inner;
}

Expand All @@ -146,25 +145,19 @@ public R getValue() {

@Override
public int hashCode() {
int hashCode = getKey().hashCode();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was also throwing NPE cause key could be null. Not anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, getKey().hashCode() meant we used the hash of the actual element, not the wrapper.

if (getValue() != null) {
hashCode ^= getValue().hashCode();
}
return hashCode;
return inner.hashCode();
}

@Override
@SuppressWarnings("unchecked")
public boolean equals(Object o) {
if (this == o) {
return true;
} else if (!(o instanceof StructLikeEntry)) {
} else if (o == null || getClass() != o.getClass()) {
return false;
} else {
StructLikeEntry that = (StructLikeEntry<R>) o;
return Objects.equals(getKey(), that.getKey())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We used getKey() which unwrapped the object and we were loosing proper equality.

&& Objects.equals(getValue(), that.getValue());
}

StructLikeEntry<?> that = (StructLikeEntry<?>) o;
return inner.equals(that.inner);
}

@Override
Expand Down
30 changes: 30 additions & 0 deletions core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.StructLike;
import org.apache.iceberg.TestHelpers.CustomRow;
import org.apache.iceberg.TestHelpers.Row;
import org.apache.iceberg.data.GenericRecord;
import org.apache.iceberg.data.Record;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -147,4 +149,32 @@ public void testKeysWithNulls() {

assertThat(map.remove(record3)).isEqualTo("aaa");
}

@Test
public void testEqualsAndHashCode() {
Copy link
Member

Choose a reason for hiding this comment

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

Can we do a quick addition of a test for empty map equality? I am pretty sure that is fine but just to be complete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll add that.

Map<StructLike, String> map1 = StructLikeMap.create(STRUCT_TYPE);
map1.put(CustomRow.of(1, null), "aaa");
map1.put(CustomRow.of(2, null), "bbb");

Map<StructLike, String> map2 = StructLikeMap.create(STRUCT_TYPE);
map2.put(Row.of(1, null), "aaa");
map2.put(Row.of(2, null), "bbb");

assertThat(map1).isEqualTo(map2);
assertThat(map1.hashCode()).isEqualTo(map2.hashCode());
}

@Test
public void testKeyAndEntrySetEquality() {
Map<StructLike, String> map1 = StructLikeMap.create(STRUCT_TYPE);
map1.put(CustomRow.of(1, null), "aaa");
map1.put(CustomRow.of(2, null), "bbb");

Map<StructLike, String> map2 = StructLikeMap.create(STRUCT_TYPE);
map2.put(Row.of(1, null), "aaa");
map2.put(Row.of(2, null), "bbb");

assertThat(map1.keySet()).isEqualTo(map2.keySet());
assertThat(map1.entrySet()).isEqualTo(map2.entrySet());
}
}
Loading