zstan commented on code in PR #13742: URL: https://github.com/apache/ignite/pull/13742#discussion_r4374317595
########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/SqlValueUtils.java: ########## @@ -0,0 +1,136 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.processors.query.calcite.util; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import org.jetbrains.annotations.Nullable; + +/** Content equality and hashing for SQL keys. */ +public final class SqlValueUtils { + /** */ + private SqlValueUtils() { + // No-op. + } + + /** Compares ROWs, ARRAYs and MAPs recursively, using scalar equality at the leaves. */ + // TODO: Replace calls to this method with Functions.deepEquals when upgrading to Calcite 1.43.0. + public static boolean deepEquals(@Nullable Object left, @Nullable Object right) { Review Comment: best practise for such a things is to create an issue for calcite bump and after mention such an issue here. Also check org.apache.ignite.testframework.junits.common.GridCommonAbstractTest#assertEqualsCollectionsIgnoringOrder impl ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/SqlValueUtils.java: ########## @@ -0,0 +1,136 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.processors.query.calcite.util; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import org.jetbrains.annotations.Nullable; + +/** Content equality and hashing for SQL keys. */ +public final class SqlValueUtils { + /** */ + private SqlValueUtils() { + // No-op. + } + + /** Compares ROWs, ARRAYs and MAPs recursively, using scalar equality at the leaves. */ + // TODO: Replace calls to this method with Functions.deepEquals when upgrading to Calcite 1.43.0. + public static boolean deepEquals(@Nullable Object left, @Nullable Object right) { + if (left == right) + return true; + + if (left == null || right == null) + return false; + + List<?> leftList = asList(left); + List<?> rightList = asList(right); + + if (leftList != null && rightList != null) { + if (leftList.size() != rightList.size()) + return false; + + for (int i = 0; i < leftList.size(); i++) { + if (!deepEquals(leftList.get(i), rightList.get(i))) + return false; + } + + return true; + } + + if (leftList != null || rightList != null) + return false; + + if (left instanceof Map && right instanceof Map) + return mapsEqual((Map<?, ?>)left, (Map<?, ?>)right); + + if (left.getClass().isArray() && right.getClass().isArray()) + return Arrays.deepEquals(new Object[] {left}, new Object[] {right}); + + return left.equals(right); + } + + /** Maps are unordered; each entry must match exactly one entry, including when keys contain ROWs. */ + private static boolean mapsEqual(Map<?, ?> left, Map<?, ?> right) { + if (left.size() != right.size()) + return false; + + List<Map.Entry<?, ?>> unmatched = new ArrayList<>(right.entrySet()); + + for (Map.Entry<?, ?> entry : left.entrySet()) { + boolean found = false; + + for (int i = 0; i < unmatched.size(); i++) { + Map.Entry<?, ?> other = unmatched.get(i); + + if (deepEquals(entry.getKey(), other.getKey()) && deepEquals(entry.getValue(), other.getValue())) { + unmatched.remove(i); + found = true; + + break; + } + } + + if (!found) + return false; + } + + return true; + } + + /** Hash consistent with {@link #deepEquals}. */ + // TODO: Replace calls with Functions.deepHashCode when upgrading to Calcite 1.43.0. + public static int deepHashCode(@Nullable Object val) { + if (val == null) + return 0x789d; + + List<?> list = asList(val); + + if (list != null) { + int hash = 1; + + for (Object item : list) + hash = 31 * hash + deepHashCode(item); + + return hash; + } + + if (val instanceof Map) { + int hash = 0; + + for (Map.Entry<?, ?> entry : ((Map<?, ?>)val).entrySet()) + hash += deepHashCode(entry.getKey()) ^ deepHashCode(entry.getValue()); + + return hash; + } + + if (val.getClass().isArray()) + return Arrays.deepHashCode(new Object[] {val}); + + return val.hashCode(); + } + + /** Views the internal ROW or ARRAY representation as a sequence. */ + private static @Nullable List<?> asList(Object val) { Review Comment: this func can be applied only for concreete cases, not for Iterator (for example) probably it\`s better to append asertion here ? or rename some how with appropriate java doc ? ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ExpressionFactoryImpl.java: ########## @@ -257,9 +258,22 @@ else if (c2 != HIGHEST_VALUE) /** */ @SuppressWarnings("rawtypes") private static int compare(Object o1, Object o2, int nullComparison) { + if (o1 == o2) + return 0; + else if (o1 == null) + return nullComparison; + else if (o2 == null) + return -nullComparison; + if (BinaryUtils.isBinaryObjectImpl(o1)) return compareBinary(o1, o2, nullComparison); + if (o1 instanceof List && o2 instanceof List) + return Functions.compareLists((List<?>)o1, (List<?>)o2); Review Comment: not optimal implementation as for me, firstly - it compares the collection items and after - size, i\`d prefer to compare: sizes - items - sizes. ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/trait/DistributionFunction.java: ########## @@ -261,8 +262,8 @@ private static final class HashDistribution extends DistributionFunction { assert F.isEmpty(assignment) || assignment.size() == 1; } - AffinityAdapter<Row> aff = new AffinityAdapter<>(affSrvc.affinity(CU.UNDEFINED_CACHE_ID), k.toIntArray(), - ctx.rowHandler()); + AffinityAdapter<Row> aff = new AffinityAdapter<>(val -> U.safeAbs(SqlValueUtils.deepHashCode(val)), Review Comment: affSrvc - is not used in such a case, 1. is it correct also suggested with back compatibility ? 2. can we remove it from interface in such a case ? -- 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]
