mihaibudiu commented on code in PR #5305: URL: https://github.com/apache/calcite/pull/5305#discussion_r4210650009
########## core/src/test/resources/sql/row-equality.iq: ########## @@ -612,4 +612,401 @@ ORDER BY c; !ok +# Test cases for https://issues.apache.org/jira/browse/CALCITE-7836 +# [CALCITE-7836] Array functions return incorrect results for ROW elements +# Construct equal rows from separate input columns so constant folding Review Comment: this is an implementation detail, it does not have to be in the test ########## core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java: ########## @@ -166,6 +166,57 @@ private static void checkArrayRowComparison(Object specialRow, Object normalRow, assertThat(SqlFunctions.sortArray(new ArrayList<>(rows), false), is(list(max, min, null))); } + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-7836";>[CALCITE-7836] + * Array functions return incorrect results for ROW elements</a>. */ + @Test void testArrayFunctionsWithRows() { + final Object[][] rows = { + {1, "Foo"}, + {null, "Foo"}, + {new Object[] {1, "Foo"}, 10}, + {list(new Object[] {1, "Foo"}, new Object[] {2, null}), 10}, + {Collections.singletonMap("key", new Object[] {1, null}), 10}, + {-31}, // Deep hash code is zero. + {-30} // Deep hash code collides with the replacement for zero. + }; + for (Object[] row : rows) { + // Copy the outer row; the nested containers are checked separately below. + checkArrayFunctionsWithRows(row, row.clone()); + } + checkArrayFunctionsWithRows(new Object[] {new Object[] {1, null}, 10}, Review Comment: Can you add a comment saying that these results were checked using a custom python script which ran the queries against spark? We've had incorrect expected results checked in in the past. I would do this for every function in this file. ########## linq4j/src/main/java/org/apache/calcite/linq4j/function/Functions.java: ########## @@ -527,115 +527,116 @@ private static class DeepEqualityComparer @Override public int hashCode(@Nullable Object t) { return deepHashCode(t); } + } - private static boolean deepEquals(@Nullable Object v1, @Nullable Object v2) { - if (v1 == v2) { - return true; - } - if (v1 == null || v2 == null) { - return false; - } - // Normalize both to List: an ARRAY of ROW is a List of Object[], - // and each element is normalized in turn. - final @Nullable List<?> list1 = asListOrNull(v1); - final @Nullable List<?> list2 = asListOrNull(v2); - if (list1 != null && list2 != null) { - final int n = list1.size(); - if (n != list2.size()) { - return false; - } - for (int i = 0; i < n; i++) { - if (!deepEquals(list1.get(i), list2.get(i))) { - return false; - } - } - return true; - } - if (list1 != null || list2 != null) { - return false; - } - if (v1 instanceof Map && v2 instanceof Map) { - return mapDeepEquals((Map<?, ?>) v1, (Map<?, ?>) v2); - } - if (v1.getClass().isArray() && v2.getClass().isArray()) { - // Primitive arrays (e.g. byte[] for BINARY values). - return Arrays.deepEquals(new Object[] {v1}, new Object[] {v2}); - } - return v1.equals(v2); + /** Compares values recursively; see {@link #deepComparer()}. */ + public static boolean deepEquals(@Nullable Object v1, @Nullable Object v2) { + if (v1 == v2) { + return true; } - - /** Compares two maps as unordered sets of entries, comparing keys and - * values deeply. - * - * <p>Java {@link Map#equals} is already order-independent, but it looks a - * key up by that key's own hashCode and equals, which matches a struct key - * only by reference; hence the scan. */ - private static boolean mapDeepEquals(Map<?, ?> m1, Map<?, ?> m2) { - if (m1.size() != m2.size()) { + if (v1 == null || v2 == null) { + return false; + } + // Normalize both to List: an ARRAY of ROW is a List of Object[], + // and each element is normalized in turn. + final @Nullable List<?> list1 = asListOrNull(v1); + final @Nullable List<?> list2 = asListOrNull(v2); + if (list1 != null && list2 != null) { + final int n = list1.size(); + if (n != list2.size()) { return false; } - // Remove on match, so that keys that are deep-equal but distinct to - // Java, as two Object[] with the same contents are, pair up one to one. - final List<Map.Entry<?, ?>> unmatched = new ArrayList<>(m2.entrySet()); - for (Map.Entry<?, ?> e1 : m1.entrySet()) { - boolean found = false; - for (int i = 0; i < unmatched.size(); i++) { - final Map.Entry<?, ?> e2 = unmatched.get(i); - if (deepEquals(e1.getKey(), e2.getKey()) - && deepEquals(e1.getValue(), e2.getValue())) { - unmatched.remove(i); - found = true; - break; - } - } - if (!found) { + for (int i = 0; i < n; i++) { + if (!deepEquals(list1.get(i), list2.get(i))) { return false; } } return true; } + if (list1 != null || list2 != null) { + return false; + } + if (v1 instanceof Map && v2 instanceof Map) { + return mapDeepEquals((Map<?, ?>) v1, (Map<?, ?>) v2); + } + if (v1.getClass().isArray() && v2.getClass().isArray()) { + // Primitive arrays (e.g. byte[] for BINARY values). + return Arrays.deepEquals(new Object[] {v1}, new Object[] {v2}); + } + return v1.equals(v2); + } - /** Computes a hash code that is equal for values that - * {@link #deepEquals} considers equal; in particular, an - * {@code Object[]} and a {@link List} with equal elements hash alike. */ - private static int deepHashCode(@Nullable Object o) { - if (o == null) { - return 0x789d; - } - final @Nullable List<?> list = asListOrNull(o); - if (list != null) { - int h = 1; - for (Object element : list) { - h = 31 * h + deepHashCode(element); - } - return h; - } - if (o instanceof Map) { - // Sum of per-entry hashes, as Map.hashCode does, so that the hash - // ignores entry order just as mapDeepEquals does. - int h = 0; - for (Map.Entry<?, ?> e : ((Map<?, ?>) o).entrySet()) { - h += deepHashCode(e.getKey()) ^ deepHashCode(e.getValue()); + /** Compares two maps as unordered sets of entries, comparing keys and + * values deeply. + * + * <p>Java {@link Map#equals} is already order-independent, but it looks a + * key up by that key's own hashCode and equals, which matches a struct key + * only by reference; hence the scan. */ + private static boolean mapDeepEquals(Map<?, ?> m1, Map<?, ?> m2) { + if (m1.size() != m2.size()) { + return false; + } + // Remove on match, so that keys that are deep-equal but distinct to + // Java, as two Object[] with the same contents are, pair up one to one. + final List<Map.Entry<?, ?>> unmatched = new ArrayList<>(m2.entrySet()); + for (Map.Entry<?, ?> e1 : m1.entrySet()) { + boolean found = false; + for (int i = 0; i < unmatched.size(); i++) { Review Comment: Maybe you can file a ticket for this. ########## core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java: ########## @@ -7333,17 +7339,82 @@ public static List arrayExcept(List list1, List list2) { /** Support the ARRAY_INTERSECT function. */ public static List arrayIntersect(List list1, List list2) { - final Set result = new LinkedHashSet<>(list1); - result.retainAll(list2); - return new ArrayList<>(result); + final Set<@Nullable Object> result = toDeepEqualitySet(list1); + result.retainAll(toDeepEqualitySet(list2)); + return unwrapDeepEqualitySet(result); } /** Support the ARRAY_UNION function. */ public static List arrayUnion(List list1, List list2) { - final Set result = new LinkedHashSet<>(); - result.addAll(list1); - result.addAll(list2); - return new ArrayList<>(result); + final Set<@Nullable Object> result = toDeepEqualitySet(list1); + for (Object element : list2) { + result.add(wrapForDeepEquality(element)); + } + return unwrapDeepEqualitySet(result); + } + + /** Creates a set using deep equality, preserving element encounter order. + * This preserves the ordering required by ARRAY_DISTINCT. */ + private static Set<@Nullable Object> toDeepEqualitySet(List<?> list) { + final Set<@Nullable Object> set = new LinkedHashSet<>(); + for (Object value : list) { + set.add(wrapForDeepEquality(value)); + } + return set; + } + + /** Wraps composite values, which may contain ROW values, but leaves scalar + * values and null unchanged. */ + private static @Nullable Object wrapForDeepEquality(@Nullable Object value) { + if (value == null) { + return null; + } else if (value instanceof List || value instanceof Map || value.getClass().isArray()) { + return new DeepEqualityWrapper(value); + } + return value; + } + + /** Creates a list of the set's elements in iteration order, replacing + * deep-equality wrappers with their original values. */ + private static List<@Nullable Object> unwrapDeepEqualitySet(Set<@Nullable Object> set) { + final List<@Nullable Object> values = new ArrayList<>(set.size()); + for (Object element : set) { + values.add(element instanceof DeepEqualityWrapper + ? ((DeepEqualityWrapper) element).value : element); + } + return values; + } + + /** A wrapper providing deep equality and hashing for composite values. + * The value must not change while the wrapper is in use. */ Review Comment: I would say "assumes values are immutable". ########## core/src/test/resources/sql/row-equality.iq: ########## @@ -612,4 +612,401 @@ ORDER BY c; !ok +# Test cases for https://issues.apache.org/jira/browse/CALCITE-7836 +# [CALCITE-7836] Array functions return incorrect results for ROW elements +# Construct equal rows from separate input columns so constant folding Review Comment: same request as above: if these were validated, write this information here. -- 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]
