From ee9e1bfddf27a7868b650ea6cc04a6cf74eebea8 Mon Sep 17 00:00:00 2001 From: aokolnychyi Date: Wed, 6 Dec 2023 11:41:41 -0800 Subject: [PATCH 1/2] Core: Fix equality in StructLikeMap --- .../java/org/apache/iceberg/TestHelpers.java | 46 +++++++++++++++++++ .../apache/iceberg/util/StructLikeMap.java | 21 +++------ .../iceberg/util/TestStructLikeMap.java | 30 ++++++++++++ 3 files changed, 83 insertions(+), 14 deletions(-) diff --git a/api/src/test/java/org/apache/iceberg/TestHelpers.java b/api/src/test/java/org/apache/iceberg/TestHelpers.java index cefd765a91b5..fcb528caba96 100644 --- a/api/src/test/java/org/apache/iceberg/TestHelpers.java +++ b/api/src/test/java/org/apache/iceberg/TestHelpers.java @@ -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 get(int pos, Class javaClass) { + return javaClass.cast(values[pos]); + } + + @Override + public 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; diff --git a/core/src/main/java/org/apache/iceberg/util/StructLikeMap.java b/core/src/main/java/org/apache/iceberg/util/StructLikeMap.java index 58bd03041331..2bb5fa1c9d40 100644 --- a/core/src/main/java/org/apache/iceberg/util/StructLikeMap.java +++ b/core/src/main/java/org/apache/iceberg/util/StructLikeMap.java @@ -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; @@ -128,9 +127,9 @@ public Set> entrySet() { private static class StructLikeEntry implements Entry { - private Map.Entry inner; + private final Entry inner; - private StructLikeEntry(Map.Entry inner) { + private StructLikeEntry(Entry inner) { this.inner = inner; } @@ -146,25 +145,19 @@ public R getValue() { @Override public int hashCode() { - int hashCode = getKey().hashCode(); - 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) o; - return Objects.equals(getKey(), that.getKey()) - && Objects.equals(getValue(), that.getValue()); } + + StructLikeEntry that = (StructLikeEntry) o; + return inner.equals(that.inner); } @Override diff --git a/core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java b/core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java index db4176ce231e..bd67d3cf0a95 100644 --- a/core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java +++ b/core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java @@ -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; @@ -147,4 +149,32 @@ public void testKeysWithNulls() { assertThat(map.remove(record3)).isEqualTo("aaa"); } + + @Test + public void testEqualsAndHashCode() { + Map map1 = StructLikeMap.create(STRUCT_TYPE); + map1.put(CustomRow.of(1, null), "aaa"); + map1.put(CustomRow.of(2, null), "bbb"); + + Map 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 map1 = StructLikeMap.create(STRUCT_TYPE); + map1.put(CustomRow.of(1, null), "aaa"); + map1.put(CustomRow.of(2, null), "bbb"); + + Map 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()); + } } From eefef22a5aeb047afe9ba067f9b5328d5e843883 Mon Sep 17 00:00:00 2001 From: aokolnychyi Date: Wed, 6 Dec 2023 12:15:08 -0800 Subject: [PATCH 2/2] Review --- .../org/apache/iceberg/util/TestStructLikeMap.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java b/core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java index bd67d3cf0a95..f18c48eaa344 100644 --- a/core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java +++ b/core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java @@ -153,10 +153,14 @@ public void testKeysWithNulls() { @Test public void testEqualsAndHashCode() { Map map1 = StructLikeMap.create(STRUCT_TYPE); + Map map2 = StructLikeMap.create(STRUCT_TYPE); + + assertThat(map1).isEqualTo(map2); + assertThat(map1.hashCode()).isEqualTo(map2.hashCode()); + map1.put(CustomRow.of(1, null), "aaa"); map1.put(CustomRow.of(2, null), "bbb"); - Map map2 = StructLikeMap.create(STRUCT_TYPE); map2.put(Row.of(1, null), "aaa"); map2.put(Row.of(2, null), "bbb"); @@ -167,10 +171,14 @@ public void testEqualsAndHashCode() { @Test public void testKeyAndEntrySetEquality() { Map map1 = StructLikeMap.create(STRUCT_TYPE); + Map map2 = StructLikeMap.create(STRUCT_TYPE); + + assertThat(map1.keySet()).isEqualTo(map2.keySet()); + assertThat(map1.entrySet()).isEqualTo(map2.entrySet()); + map1.put(CustomRow.of(1, null), "aaa"); map1.put(CustomRow.of(2, null), "bbb"); - Map map2 = StructLikeMap.create(STRUCT_TYPE); map2.put(Row.of(1, null), "aaa"); map2.put(Row.of(2, null), "bbb");