From a628ef7e11369f9cd0abb303166591dc07cd760e Mon Sep 17 00:00:00 2001 From: ffccites <99155080+PDGGK@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:46:25 +1000 Subject: [PATCH] [common] Compare maps of different representations without throwing InternalRowUtils.equals picked the GenericMap fast path by testing data1, then cast data2 to GenericMap with no test of its own: if (data1 instanceof GenericMap) { map1 = (GenericMap) data1; map2 = (GenericMap) data2; // BinaryMap -> ClassCastException So comparing a GenericMap against a BinaryMap throws, while the reverse ordering returns true. The conversion in the else branch exists precisely because one MapType is represented by GenericMap, BinaryMap or ColumnarMap interchangeably, so the operand-1 gate contradicts the branch it guards. InternalRowUtils.hash already treats the two as interchangeable and returns the same value for both, so equals throwing for one ordering is an inconsistency inside a single class rather than a deliberate restriction. Each operand is now converted on its own, keeping the fast path when both are already GenericMap. The other casts in this method -- (InternalRow) data2, (InternalArray) data2 -- are interface casts and are not affected: every representation implements them. Test asserts both orderings and the hash agreement. Only the generic-first ordering changes; the other two are green before and after, which is what pins the intent rather than just the fix. --- .../apache/paimon/utils/InternalRowUtils.java | 29 +++++++--------- .../paimon/utils/InternalRowUtilsTest.java | 34 +++++++++++++++++++ 2 files changed, 46 insertions(+), 17 deletions(-) diff --git a/paimon-common/src/main/java/org/apache/paimon/utils/InternalRowUtils.java b/paimon-common/src/main/java/org/apache/paimon/utils/InternalRowUtils.java index 4c329797f39e..8fb0b8f8050d 100644 --- a/paimon-common/src/main/java/org/apache/paimon/utils/InternalRowUtils.java +++ b/paimon-common/src/main/java/org/apache/paimon/utils/InternalRowUtils.java @@ -91,23 +91,12 @@ public static boolean equals(Object data1, Object data2, DataType dataType) { return false; } MapType mapType = (MapType) dataType; - GenericMap map1; - GenericMap map2; - if (data1 instanceof GenericMap) { - map1 = (GenericMap) data1; - map2 = (GenericMap) data2; - } else { - map1 = - copyToGenericMap( - (InternalMap) data1, - mapType.getKeyType(), - mapType.getValueType()); - map2 = - copyToGenericMap( - (InternalMap) data2, - mapType.getKeyType(), - mapType.getValueType()); - } + // Decide per operand. One MapType is represented by GenericMap, BinaryMap or + // ColumnarMap interchangeably -- which is why the conversion below exists at all -- + // so gating data2's cast on data1's concrete class threw ClassCastException + // whenever the two sides happened to use different representations. + GenericMap map1 = toGenericMap((InternalMap) data1, mapType); + GenericMap map2 = toGenericMap((InternalMap) data2, mapType); InternalArray keyArray1 = map1.keyArray(); InternalArray keyArray2 = map2.keyArray(); InternalArray valueArray1 = map1.valueArray(); @@ -284,6 +273,12 @@ private static InternalMap copyMap(InternalMap map, DataType keyType, DataType v return copyToGenericMap(map, keyType, valueType); } + private static GenericMap toGenericMap(InternalMap map, MapType mapType) { + return map instanceof GenericMap + ? (GenericMap) map + : copyToGenericMap(map, mapType.getKeyType(), mapType.getValueType()); + } + private static GenericMap copyToGenericMap( InternalMap map, DataType keyType, DataType valueType) { Map javaMap = new HashMap<>(); diff --git a/paimon-common/src/test/java/org/apache/paimon/utils/InternalRowUtilsTest.java b/paimon-common/src/test/java/org/apache/paimon/utils/InternalRowUtilsTest.java index dec1ab4ad66f..fb14c024acc3 100644 --- a/paimon-common/src/test/java/org/apache/paimon/utils/InternalRowUtilsTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/utils/InternalRowUtilsTest.java @@ -18,6 +18,9 @@ package org.apache.paimon.utils; +import org.apache.paimon.data.BinaryArray; +import org.apache.paimon.data.BinaryArrayWriter; +import org.apache.paimon.data.BinaryMap; import org.apache.paimon.data.BinaryRow; import org.apache.paimon.data.BinaryString; import org.apache.paimon.data.BinaryVector; @@ -33,6 +36,7 @@ import org.apache.paimon.datagen.RandomGeneratorVisitor; import org.apache.paimon.datagen.RowDataGenerator; import org.apache.paimon.options.Options; +import org.apache.paimon.types.DataType; import org.apache.paimon.types.DataTypeRoot; import org.apache.paimon.types.DataTypes; import org.apache.paimon.types.RowType; @@ -299,4 +303,34 @@ public void testEqualsAndHashCodeNegativeCase() { rowWithMap2.setField(0, new GenericMap(map2)); assertThat(InternalRowUtils.equals(rowWithMap1, rowWithMap2, rowType2)).isFalse(); } + + @Test + public void testEqualsAcrossMapImplementations() { + DataType mapType = DataTypes.MAP(DataTypes.STRING(), DataTypes.INT()); + + Map entries = new HashMap<>(); + entries.put(BinaryString.fromString("a"), 1); + GenericMap generic = new GenericMap(entries); + + BinaryArray keys = new BinaryArray(); + BinaryArrayWriter keyWriter = new BinaryArrayWriter(keys, 1, 8); + keyWriter.writeString(0, BinaryString.fromString("a")); + keyWriter.complete(); + BinaryArray values = new BinaryArray(); + BinaryArrayWriter valueWriter = new BinaryArrayWriter(values, 1, 4); + valueWriter.writeInt(0, 1); + valueWriter.complete(); + BinaryMap binary = BinaryMap.valueOf(keys, values); + + // hash() already treats the two representations as interchangeable, so equals() throwing + // for one ordering is the inconsistency being fixed here. + assertThat(InternalRowUtils.hash(generic, mapType)) + .isEqualTo(InternalRowUtils.hash(binary, mapType)); + + // Both orderings must agree. Only the generic-first one changes: it used to pick the + // GenericMap fast path off data1 and then cast data2 to GenericMap unconditionally, + // throwing ClassCastException for a BinaryMap. + assertThat(InternalRowUtils.equals(generic, binary, mapType)).isTrue(); + assertThat(InternalRowUtils.equals(binary, generic, mapType)).isTrue(); + } }