korlov42 commented on code in PR #4045:
URL: https://github.com/apache/ignite-3/pull/4045#discussion_r1672098248
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/AbstractPlannerTest.java:
##########
@@ -675,7 +676,16 @@ protected void checkSplitAndSerialization(IgniteRel rel,
IgniteSchema publicSche
checkSplitAndSerialization(rel, Collections.singleton(publicSchema));
}
+ // Set of Relational operators that do not support serialization and
shouldn't be sent between cluster nodes.
+ private static final Set<Class> unsupportSerializationOperators = Set.of(
+ IgniteKeyValueModify.class
Review Comment:
IgniteKeyValueGet as well
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/datatypes/BaseTypeCoercionTest.java:
##########
@@ -51,6 +53,14 @@ static Stream<Arguments> allNumericPairs() {
return Arrays.stream(NumericPair.values()).map(Arguments::of);
}
+ static void checkIncludesAllTypePairs(Stream<Arguments> args) {
Review Comment:
it's better to revert this change. BaseTypeCoercionTest is base class for
all type pairs, not only numeric ones.
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/datatypes/UpdateSourcesCoercionTest.java:
##########
@@ -0,0 +1,687 @@
+/*
+ * 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.sql.engine.planner.datatypes;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import java.util.EnumMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Stream;
+import org.apache.calcite.rex.RexNode;
+import
org.apache.ignite.internal.sql.engine.planner.datatypes.utils.NumericPair;
+import org.apache.ignite.internal.sql.engine.planner.datatypes.utils.TypePair;
+import org.apache.ignite.internal.sql.engine.planner.datatypes.utils.Types;
+import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
+import org.apache.ignite.internal.sql.engine.rel.IgniteTableModify;
+import org.apache.ignite.internal.sql.engine.schema.IgniteSchema;
+import org.apache.ignite.internal.sql.engine.util.SqlTestUtils;
+import org.apache.ignite.internal.type.NativeTypeSpec;
+import org.apache.ignite.internal.type.NativeTypes;
+import org.hamcrest.BaseMatcher;
+import org.hamcrest.Description;
+import org.hamcrest.Matcher;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+/**
+ * A set of test to verify behavior of type coercion for UPDATE operations,
when values belongs to the NUMERIC type family.
+ *
+ * <p>This tests aim to help to understand in which cases implicit cast will
be added to which values.
+ */
+public class UpdateSourcesCoercionTest extends BaseTypeCoercionTest {
+
+ @ParameterizedTest
+ @MethodSource("args1")
+ public void update1(
+ TypePair pair,
+ Matcher<RexNode> operandMatcher
+ ) throws Exception {
+ // TODO: remove during implement IGNITE-22283
+ if (pair.first().spec() == NativeTypeSpec.NUMBER ||
pair.second().spec() == NativeTypeSpec.NUMBER) {
+ return;
+ }
+
+ IgniteSchema schema = createSchemaWithTwoColumnTable(pair.first(),
pair.second());
+
+ Object val =
SqlTestUtils.generateValueByType(pair.second().spec().asColumnType());
+
+ assertPlan("UPDATE T SET c1=" + val, schema,
modifyOperandMatcher(operandMatcher)::matches, List.of());
+ }
+
+ @ParameterizedTest
+ @MethodSource("argsDyn")
+ public void updateDynamicParameters(
+ TypePair pair,
+ Matcher<RexNode> operandMatcher
+ ) throws Exception {
+ // TODO: remove during implement IGNITE-22283
+ if (pair.first().spec() == NativeTypeSpec.NUMBER ||
pair.second().spec() == NativeTypeSpec.NUMBER) {
+ return;
+ }
+
+ IgniteSchema schema = createSchemaWithTwoColumnTable(pair.first(),
pair.second());
+
+ Object val =
SqlTestUtils.generateValueByType(pair.second().spec().asColumnType());
+
+ assertPlan("UPDATE T SET c1=?", schema,
modifyOperandMatcher(operandMatcher)::matches, List.of(val));
+ }
+
+ @ParameterizedTest
+ @MethodSource("args2")
+ public void update2(
+ TypePair pair,
+ Matcher<RexNode> operandMatcher
+ ) throws Exception {
+ // TODO: remove during implement IGNITE-22283
+ if (pair.first().spec() == NativeTypeSpec.NUMBER ||
pair.second().spec() == NativeTypeSpec.NUMBER) {
+ return;
+ }
+
+ IgniteSchema schema = createSchemaWithTwoColumnTable(pair.first(),
pair.second());
+
+ assertPlan("UPDATE T SET C1=C2", schema,
modifyOperandMatcher(operandMatcher)::matches, List.of());
+ }
+
+ /**
+ * This test ensures that {@link #args1()}, {@link #args2()} and {@link
#argsDyn()} doesn't miss any type pair from
+ * {@link NumericPair}.
+ */
+ @Test
+ void argsIncludesAllTypePairs() {
+ checkIncludesAllTypePairs(args1());
+ checkIncludesAllTypePairs(args2());
+ checkIncludesAllTypePairs(argsDyn());
+ }
+
+ private static Matcher<IgniteRel> modifyOperandMatcher(Matcher<RexNode>
matcher) {
+ return new BaseMatcher<>() {
+ @Override
+ public boolean matches(Object actual) {
+ RexNode expression =
Objects.requireNonNull(((IgniteTableModify)
actual).getSourceExpressionList()).get(0);
+
+ assertThat(expression, matcher);
+
+ return true;
+ }
+
+ @Override
+ public void describeTo(Description description) {
+
+ }
+ };
+ }
+
+
+ private static Stream<Arguments> args1() {
Review Comment:
the same, let's give it meaningful name
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/datatypes/UpdateSourcesCoercionTest.java:
##########
@@ -0,0 +1,687 @@
+/*
+ * 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.sql.engine.planner.datatypes;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import java.util.EnumMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Stream;
+import org.apache.calcite.rex.RexNode;
+import
org.apache.ignite.internal.sql.engine.planner.datatypes.utils.NumericPair;
+import org.apache.ignite.internal.sql.engine.planner.datatypes.utils.TypePair;
+import org.apache.ignite.internal.sql.engine.planner.datatypes.utils.Types;
+import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
+import org.apache.ignite.internal.sql.engine.rel.IgniteTableModify;
+import org.apache.ignite.internal.sql.engine.schema.IgniteSchema;
+import org.apache.ignite.internal.sql.engine.util.SqlTestUtils;
+import org.apache.ignite.internal.type.NativeTypeSpec;
+import org.apache.ignite.internal.type.NativeTypes;
+import org.hamcrest.BaseMatcher;
+import org.hamcrest.Description;
+import org.hamcrest.Matcher;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+/**
+ * A set of test to verify behavior of type coercion for UPDATE operations,
when values belongs to the NUMERIC type family.
+ *
+ * <p>This tests aim to help to understand in which cases implicit cast will
be added to which values.
+ */
+public class UpdateSourcesCoercionTest extends BaseTypeCoercionTest {
+
+ @ParameterizedTest
+ @MethodSource("args1")
+ public void update1(
Review Comment:
let's give it meaningful name. The same for `update2` method
##########
modules/sql-engine/src/testFixtures/java/org/apache/ignite/internal/sql/engine/util/SqlTestUtils.java:
##########
@@ -351,4 +355,50 @@ public static RexNode
generateLiteralOrValueExpr(ColumnType type, Object value)
throw new IllegalArgumentException("Unexpected type: " + type);
}
}
+
+ /** Convert {@link ColumnType} to {@link SqlTypeName}. */
+ public static SqlTypeName columnType2SqlTypeName(ColumnType columnType) {
+ switch (columnType) {
+ case NULL:
+ return SqlTypeName.NULL;
+ case BOOLEAN:
+ return SqlTypeName.BOOLEAN;
+ case INT8:
+ return SqlTypeName.TINYINT;
+ case INT16:
+ return SqlTypeName.SMALLINT;
+ case INT32:
+ return SqlTypeName.INTEGER;
+ case INT64:
+ return SqlTypeName.BIGINT;
+ case FLOAT:
+ return SqlTypeName.REAL;
+ case DOUBLE:
+ return SqlTypeName.DOUBLE;
+ case DECIMAL:
+ case NUMBER:
+ return SqlTypeName.DECIMAL;
+ case DATE:
+ return SqlTypeName.DATE;
+ case TIME:
+ return SqlTypeName.TIME;
+ case DATETIME:
+ return SqlTypeName.TIMESTAMP_WITH_LOCAL_TIME_ZONE;
+ case TIMESTAMP:
+ return SqlTypeName.TIMESTAMP;
Review Comment:
vice versa
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/datatypes/InsertSourcesCoercionTest.java:
##########
@@ -0,0 +1,665 @@
+/*
+ * 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.sql.engine.planner.datatypes;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import java.util.EnumMap;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Stream;
+import org.apache.calcite.rex.RexNode;
+import
org.apache.ignite.internal.sql.engine.planner.datatypes.utils.NumericPair;
+import org.apache.ignite.internal.sql.engine.planner.datatypes.utils.TypePair;
+import org.apache.ignite.internal.sql.engine.planner.datatypes.utils.Types;
+import org.apache.ignite.internal.sql.engine.rel.IgniteKeyValueModify;
+import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
+import org.apache.ignite.internal.sql.engine.schema.IgniteSchema;
+import org.apache.ignite.internal.sql.engine.util.SqlTestUtils;
+import org.apache.ignite.internal.type.NativeTypeSpec;
+import org.apache.ignite.internal.type.NativeTypes;
+import org.hamcrest.BaseMatcher;
+import org.hamcrest.Description;
+import org.hamcrest.Matcher;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+/**
+ * A set of test to verify behavior of type coercion for INSERT operations,
when values belongs to the NUMERIC type family.
+ *
+ * <p>This tests aim to help to understand in which cases implicit cast will
be added to which values.
+ */
+public class InsertSourcesCoercionTest extends BaseTypeCoercionTest {
Review Comment:
let's add `Numeric-` prefix to conform to other test classes inside the
package. The same for other classes
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
