You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "zstan (via GitHub)" <gi...@apache.org> on 2024/09/12 08:59:25 UTC
[PR] IGNITE-23141 Sql. Add execution test to verify numeric type coercion in binary comparison and arithmetic [ignite-3]
zstan opened a new pull request, #4382:
URL: https://github.com/apache/ignite-3/pull/4382
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23141 Sql. Add execution test to verify numeric type coercion in binary comparison and arithmetic [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #4382:
URL: https://github.com/apache/ignite-3/pull/4382#discussion_r1764440768
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/coercion/BaseTypeCheckExecutionTest.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * 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.exec.coercion;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+import java.math.BigDecimal;
+import java.math.RoundingMode;
+import org.apache.ignite.internal.sql.engine.framework.DataProvider;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders;
+import org.apache.ignite.internal.sql.engine.framework.TestCluster;
+import org.apache.ignite.internal.sql.engine.planner.datatypes.utils.TypePair;
+import org.apache.ignite.internal.sql.engine.util.CursorUtils;
+import org.apache.ignite.internal.sql.engine.util.SqlTestUtils;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.apache.ignite.internal.type.DecimalNativeType;
+import org.apache.ignite.internal.type.NativeType;
+import org.apache.ignite.internal.type.NativeTypes;
+import org.apache.ignite.internal.util.Pair;
+import org.apache.ignite.sql.ColumnMetadata;
+import org.apache.ignite.sql.ColumnType;
+import org.apache.ignite.sql.ResultSetMetadata;
+import org.hamcrest.Matcher;
+
+/** Base class for check execution results of numeric operations. */
+class BaseTypeCheckExecutionTest extends BaseIgniteAbstractTest {
+ private static Pair<Object, Object> generateDifferentValues(TypePair typePair) {
Review Comment:
could you please add javadocs
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23141 Sql. Add execution test to verify numeric type coercion in binary comparison and arithmetic [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #4382:
URL: https://github.com/apache/ignite-3/pull/4382#discussion_r1758677572
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/coercion/BaseTypeCheckExecutionTest.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * 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.exec.coercion;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.await;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+import java.math.BigDecimal;
+import java.math.RoundingMode;
+import org.apache.ignite.internal.sql.engine.framework.DataProvider;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders;
+import org.apache.ignite.internal.sql.engine.framework.TestCluster;
+import org.apache.ignite.internal.sql.engine.planner.datatypes.utils.TypePair;
+import org.apache.ignite.internal.sql.engine.util.SqlTestUtils;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.apache.ignite.internal.type.DecimalNativeType;
+import org.apache.ignite.internal.type.NativeType;
+import org.apache.ignite.internal.type.NativeTypes;
+import org.apache.ignite.internal.util.Pair;
+import org.apache.ignite.sql.ColumnType;
+import org.hamcrest.Matcher;
+
+/** Base class for check execution results of numeric operations. */
+class BaseTypeCheckExecutionTest extends BaseIgniteAbstractTest {
+ static Pair<Object, Object> generateDifferentValues(TypePair typePair) {
+ Object objFirst = SqlTestUtils.generateValueByType(typePair.first());
+ assert objFirst != null;
+ Object objSecond;
+ do {
+ objSecond = SqlTestUtils.generateValueByType(typePair.second());
+
+ assert objSecond != null;
+ } while (objFirst.toString().equals(objSecond.toString()));
+
+ return new Pair<>(objFirst, objSecond);
+ }
+
+ static Object generateConstantValueByType(NativeType type) {
+ String numericBase = "9";
+ ColumnType type0 = type.spec().asColumnType();
+ switch (type0) {
+ case INT8:
+ return Byte.valueOf(numericBase);
+ case INT16:
+ return Short.valueOf(numericBase);
+ case INT32:
+ return Integer.valueOf(numericBase);
+ case INT64:
+ return Long.valueOf(numericBase);
+ case FLOAT:
+ return Float.valueOf(numericBase);
+ case DOUBLE:
+ return Double.valueOf(numericBase);
+ case DECIMAL:
+ int scale = ((DecimalNativeType) type).scale();
+ int precision = ((DecimalNativeType) type).precision();
+ assert precision >= scale : "unexpected precision\\scale";
+
+ BigDecimal bd = new BigDecimal(numericBase);
+ return bd.setScale(scale, RoundingMode.UNNECESSARY);
+ default:
+ throw new AssertionError("Unexpected type: " + type0);
+ }
+ }
+
+ static ClusterWrapper testCluster(TypePair typePair, DataProvider<Object[]> dataProvider) {
+ TestCluster cluster = TestBuilders.cluster().nodes("N1")
+ .addTable().name("T")
+ .addKeyColumn("id", NativeTypes.INT32)
+ .addColumn("C1", typePair.first())
+ .addColumn("C2", typePair.second())
+ .end()
+ .dataProvider("N1", "T", TestBuilders.tableScan(dataProvider))
+ .build();
+
+ return new ClusterWrapper(cluster);
+ }
+
+ static class ClusterWrapper implements AutoCloseable {
+ private TestCluster cluster;
+
+ ClusterWrapper(TestCluster cluster) {
+ this.cluster = cluster;
+
+ cluster.start();
+ }
+
+ void process(String sql, Matcher<Object> resultMatcher) {
+ var gatewayNode = cluster.node("N1");
+ var plan = gatewayNode.prepare(sql);
+
+ for (var row : await(gatewayNode.executePlan(plan).requestNextAsync(10_000)).items()) {
Review Comment:
CursorUtils#getAllFromCursor
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23141 Sql. Add execution test to verify numeric type coercion in binary comparison and arithmetic [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #4382:
URL: https://github.com/apache/ignite-3/pull/4382#discussion_r1760545299
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/coercion/BaseTypeCheckExecutionTest.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * 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.exec.coercion;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.await;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+import java.math.BigDecimal;
+import java.math.RoundingMode;
+import org.apache.ignite.internal.sql.engine.framework.DataProvider;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders;
+import org.apache.ignite.internal.sql.engine.framework.TestCluster;
+import org.apache.ignite.internal.sql.engine.planner.datatypes.utils.TypePair;
+import org.apache.ignite.internal.sql.engine.util.SqlTestUtils;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.apache.ignite.internal.type.DecimalNativeType;
+import org.apache.ignite.internal.type.NativeType;
+import org.apache.ignite.internal.type.NativeTypes;
+import org.apache.ignite.internal.util.Pair;
+import org.apache.ignite.sql.ColumnType;
+import org.hamcrest.Matcher;
+
+/** Base class for check execution results of numeric operations. */
+class BaseTypeCheckExecutionTest extends BaseIgniteAbstractTest {
+ static Pair<Object, Object> generateDifferentValues(TypePair typePair) {
+ Object objFirst = SqlTestUtils.generateValueByType(typePair.first());
+ assert objFirst != null;
+ Object objSecond;
+ do {
+ objSecond = SqlTestUtils.generateValueByType(typePair.second());
+
+ assert objSecond != null;
+ } while (objFirst.toString().equals(objSecond.toString()));
+
+ return new Pair<>(objFirst, objSecond);
+ }
+
+ static Object generateConstantValueByType(NativeType type) {
+ String numericBase = "9";
+ ColumnType type0 = type.spec().asColumnType();
+ switch (type0) {
+ case INT8:
+ return Byte.valueOf(numericBase);
+ case INT16:
+ return Short.valueOf(numericBase);
+ case INT32:
+ return Integer.valueOf(numericBase);
+ case INT64:
+ return Long.valueOf(numericBase);
+ case FLOAT:
+ return Float.valueOf(numericBase);
+ case DOUBLE:
+ return Double.valueOf(numericBase);
+ case DECIMAL:
+ int scale = ((DecimalNativeType) type).scale();
+ int precision = ((DecimalNativeType) type).precision();
+ assert precision >= scale : "unexpected precision\\scale";
+
+ BigDecimal bd = new BigDecimal(numericBase);
+ return bd.setScale(scale, RoundingMode.UNNECESSARY);
+ default:
+ throw new AssertionError("Unexpected type: " + type0);
+ }
+ }
+
+ static ClusterWrapper testCluster(TypePair typePair, DataProvider<Object[]> dataProvider) {
+ TestCluster cluster = TestBuilders.cluster().nodes("N1")
+ .addTable().name("T")
+ .addKeyColumn("id", NativeTypes.INT32)
+ .addColumn("C1", typePair.first())
+ .addColumn("C2", typePair.second())
+ .end()
+ .dataProvider("N1", "T", TestBuilders.tableScan(dataProvider))
+ .build();
+
+ return new ClusterWrapper(cluster);
+ }
+
+ static class ClusterWrapper implements AutoCloseable {
+ private TestCluster cluster;
+
+ ClusterWrapper(TestCluster cluster) {
+ this.cluster = cluster;
+
+ cluster.start();
+ }
+
+ void process(String sql, Matcher<Object> resultMatcher) {
+ var gatewayNode = cluster.node("N1");
+ var plan = gatewayNode.prepare(sql);
+
+ for (var row : await(gatewayNode.executePlan(plan).requestNextAsync(10_000)).items()) {
Review Comment:
done
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23141 Sql. Add execution test to verify numeric type coercion in binary comparison and arithmetic [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #4382:
URL: https://github.com/apache/ignite-3/pull/4382#discussion_r1760897222
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/coercion/NumericComparisonExecutionTest.java:
##########
@@ -0,0 +1,993 @@
+/*
+ * 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.exec.coercion;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.math.BigDecimal;
+import java.util.Arrays;
+import java.util.EnumMap;
+import java.util.EnumSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.stream.Stream;
+import org.apache.ignite.internal.sql.engine.framework.DataProvider;
+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.util.SqlTestUtils;
+import org.apache.ignite.internal.type.DecimalNativeType;
+import org.apache.ignite.internal.type.NativeType;
+import org.apache.ignite.internal.type.NativeTypes;
+import org.apache.ignite.internal.util.Pair;
+import org.apache.ignite.sql.ColumnMetadata;
+import org.hamcrest.BaseMatcher;
+import org.hamcrest.Description;
+import org.hamcrest.Matcher;
+import org.junit.jupiter.api.Assumptions;
+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;
+
+/** Check execution and return results for numeric operations. */
+public class NumericComparisonExecutionTest extends BaseTypeCheckExecutionTest {
+ private static final Set<NativeType> FRACTIONAL_TYPES = Set.of(NativeTypes.DOUBLE, NativeTypes.FLOAT);
Review Comment:
```suggestion
private static final Set<NativeType> APPROXIMATE_NUMERIC_TYPES = Set.of(NativeTypes.DOUBLE, NativeTypes.FLOAT);
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/coercion/NumericComparisonExecutionTest.java:
##########
@@ -0,0 +1,993 @@
+/*
+ * 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.exec.coercion;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.math.BigDecimal;
+import java.util.Arrays;
+import java.util.EnumMap;
+import java.util.EnumSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.stream.Stream;
+import org.apache.ignite.internal.sql.engine.framework.DataProvider;
+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.util.SqlTestUtils;
+import org.apache.ignite.internal.type.DecimalNativeType;
+import org.apache.ignite.internal.type.NativeType;
+import org.apache.ignite.internal.type.NativeTypes;
+import org.apache.ignite.internal.util.Pair;
+import org.apache.ignite.sql.ColumnMetadata;
+import org.hamcrest.BaseMatcher;
+import org.hamcrest.Description;
+import org.hamcrest.Matcher;
+import org.junit.jupiter.api.Assumptions;
+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;
+
+/** Check execution and return results for numeric operations. */
+public class NumericComparisonExecutionTest extends BaseTypeCheckExecutionTest {
+ private static final Set<NativeType> FRACTIONAL_TYPES = Set.of(NativeTypes.DOUBLE, NativeTypes.FLOAT);
+
+ private static final Map<NumericPair, ClassInfoHolder> NUMERIC_OP_TYPES_SUM = new EnumMap<>(NumericPair.class);
+ private static final Map<NumericPair, ClassInfoHolder> NUMERIC_OP_TYPES_SUBTRACT = new EnumMap<>(NumericPair.class);
+ private static final Map<NumericPair, ClassInfoHolder> NUMERIC_OP_TYPES_MULT = new EnumMap<>(NumericPair.class);
+ private static final Map<NumericPair, ClassInfoHolder> NUMERIC_OP_TYPES_DIV = new EnumMap<>(NumericPair.class);
+ private static final Map<NumericPair, ClassInfoHolder> NUMERIC_OP_TYPES_MODULO = new EnumMap<>(NumericPair.class);
+
+ enum Operation {
+ SUM, SUBTRACT, MULT, DIV, MODULO
+ }
+
+ private static final Map<Operation, Map<NumericPair, ClassInfoHolder>> TYPES_MAPPING = Map.of(
+ Operation.SUM, NUMERIC_OP_TYPES_SUM,
+ Operation.SUBTRACT, NUMERIC_OP_TYPES_SUBTRACT,
+ Operation.MULT, NUMERIC_OP_TYPES_MULT,
+ Operation.DIV, NUMERIC_OP_TYPES_DIV,
+ Operation.MODULO, NUMERIC_OP_TYPES_MODULO
+ );
+
+
+ @ParameterizedTest
+ @MethodSource("comparisonWithEqArgs")
+ public void comparisonEq(TypePair typePair, String sql, Matcher<Object> resultMatcher) throws Exception {
+ try (ClusterWrapper testCluster = testCluster(typePair, eqDataProvider(typePair))) {
+ testCluster.process(sql, resultMatcher);
+ }
+ }
+
+ @ParameterizedTest
+ @MethodSource("comparisonNotEqArgs")
+ public void comparisonNotEq(TypePair typePair, String sql, Matcher<Object> resultMatcher) throws Exception {
+ try (ClusterWrapper testCluster = testCluster(typePair, nonEqDataProvider(typePair))) {
+ testCluster.process(sql, resultMatcher);
+ }
+ }
+
+ @ParameterizedTest
+ @MethodSource("sumArgs")
+ public void sumOp(TypePair typePair, String sql, Matcher<Object> resultMatcher) throws Exception {
+ try (ClusterWrapper testCluster = testCluster(typePair, eqDataProvider(typePair))) {
+ testCluster.process(sql, resultMatcher);
+ }
+ }
+
+ @ParameterizedTest
+ @MethodSource("subtractArgs")
+ public void subtractOp(TypePair typePair, String sql, Matcher<Object> resultMatcher) throws Exception {
+ try (ClusterWrapper testCluster = testCluster(typePair, eqDataProvider(typePair))) {
+ testCluster.process(sql, resultMatcher);
+ }
+ }
+
+ @ParameterizedTest
+ @MethodSource("divArgs")
+ public void divOp(TypePair typePair, String sql, Matcher<Object> resultMatcher) throws Exception {
+ Assumptions.assumeFalse(typePair.first() instanceof DecimalNativeType || typePair.second() instanceof DecimalNativeType,
+ "need to be fixed after: https://issues.apache.org/jira/browse/IGNITE-23171");
+
+ try (ClusterWrapper testCluster = testCluster(typePair, binOpDataProvider(typePair))) {
+ testCluster.process(sql, resultMatcher);
+ }
+ }
+
+ @ParameterizedTest
+ @MethodSource("multArgs")
+ public void multOp(TypePair typePair, String sql, Matcher<Object> resultMatcher) throws Exception {
+ try (ClusterWrapper testCluster = testCluster(typePair, eqDataProvider(typePair))) {
+ testCluster.process(sql, resultMatcher);
+ }
+ }
+
+ @ParameterizedTest
+ @MethodSource("moduloArgs")
+ public void moduloOp(TypePair typePair, String sql, Matcher<Object> resultMatcher) throws Exception {
+ // modulo undefined for fractional types
+ if (FRACTIONAL_TYPES.contains(typePair.first()) || FRACTIONAL_TYPES.contains(typePair.second())) {
+ return;
+ }
+
+ try (ClusterWrapper testCluster = testCluster(typePair, eqDataProvider(typePair))) {
+ testCluster.process(sql, resultMatcher);
+ }
+ }
+
+ /**
+ * This test ensures that object mapping doesn't miss any type pair from {@link NumericPair}.
+ */
+ @Test
+ void argsIncludesAllTypePairs() {
+ for (Map<NumericPair, ClassInfoHolder> ent : TYPES_MAPPING.values()) {
+ EnumSet<NumericPair> remainingPairs = EnumSet.allOf(NumericPair.class);
+
+ remainingPairs.removeAll(ent.keySet());
+
+ assertTrue(remainingPairs.isEmpty(), () ->
+ "Not all types are enlisted, remaining: " + remainingPairs);
+ }
+ }
+
+ private static DataProvider<Object[]> eqDataProvider(TypePair typePair) {
+ Object val1;
+ Object val2;
+
+ if (typePair.first().equals(typePair.second())) {
+ val1 = generateConstantValueByType(typePair.first());
+ val2 = val1;
+ } else {
+ val1 = generateConstantValueByType(typePair.first());
+ val2 = generateConstantValueByType(typePair.second());
+ }
+
+ return DataProvider.fromRow(new Object[]{0, val1, val2}, 1);
+ }
+
+ private static DataProvider<Object[]> nonEqDataProvider(TypePair typePair) {
+ Pair<Object, Object> objPair = generateDifferentValues(typePair);
+ Object val1 = objPair.getFirst();
+ Object val2 = objPair.getSecond();
+
+ return DataProvider.fromRow(new Object[]{0, val1, val2}, 1);
+ }
+
+ private static DataProvider<Object[]> binOpDataProvider(TypePair typePair) {
+ Object val1 = SqlTestUtils.generateValueByType(typePair.first());
+ Object val2 = SqlTestUtils.generateValueByType(typePair.second());
+
+ return DataProvider.fromRow(new Object[]{0, val1, val2}, 1);
+ }
+
+ private static Stream<Arguments> comparisonWithEqArgs() {
+ Stream<Arguments> s1 = Arrays.stream(NumericPair.values()).map(a -> forTypePair(a, "SELECT c1 = c2 FROM t").ok());
+ Stream<Arguments> s2 = Arrays.stream(NumericPair.values()).map(a -> forTypePair(a, "SELECT c1 <= c2 FROM t").ok());
+ Stream<Arguments> s3 = Arrays.stream(NumericPair.values()).map(a -> forTypePair(a, "SELECT c1 >= c2 FROM t").ok());
+ Stream<Arguments> s4 = Arrays.stream(NumericPair.values()).map(a -> forTypePair(a, "SELECT c1 < c2 FROM t").fail());
+ Stream<Arguments> s5 = Arrays.stream(NumericPair.values()).map(a -> forTypePair(a, "SELECT c1 > c2 FROM t").fail());
+
+ return Stream.of(s1, s2, s3, s4, s5).flatMap(Function.identity());
+ }
+
+ private static Stream<Arguments> comparisonNotEqArgs() {
+ return Arrays.stream(NumericPair.values()).map(a -> forTypePair(a, "SELECT c1 = c2 FROM t").fail());
+ }
+
+ private static Stream<Arguments> sumArgs() {
+ return Arrays.stream(NumericPair.values()).map(a -> forTypePair(a, "SELECT c1 + c2 FROM t").type(a, Operation.SUM));
+ }
+
+ private static Stream<Arguments> subtractArgs() {
+ return Arrays.stream(NumericPair.values()).map(a -> forTypePair(a, "SELECT c1 - c2 FROM t").type(a, Operation.SUBTRACT));
+ }
+
+ private static Stream<Arguments> multArgs() {
+ return Arrays.stream(NumericPair.values()).map(a -> forTypePair(a, "SELECT c1 * c2 FROM t").type(a, Operation.MULT));
+ }
+
+ private static Stream<Arguments> divArgs() {
+ return Arrays.stream(NumericPair.values()).map(a -> forTypePair(a, "SELECT c1 / c2 FROM t").type(a, Operation.DIV));
+ }
+
+ private static Stream<Arguments> moduloArgs() {
+ return Arrays.stream(NumericPair.values()).map(a -> forTypePair(a, "SELECT c1 % c2 FROM t").type(a, Operation.MODULO));
+ }
+
+ private static ExecutionResultBuilder forTypePair(TypePair typePair, String exp) {
+ return new ExecutionResultBuilder(typePair, exp);
+ }
+
+ static class ExecutionResultBuilder {
+ private final TypePair pair;
+ private Matcher<?> opMatcher;
+ private final String expression;
+
+ private ExecutionResultBuilder(TypePair pair, String exp) {
+ this.pair = pair;
+ expression = exp;
+ }
+
+ Arguments fail() {
+ opMatcher = ofBoolType(false);
+
+ return Arguments.of(pair, expression, opMatcher);
+ }
+
+ Arguments ok() {
+ opMatcher = ofBoolType(true);
+
+ return Arguments.of(pair, expression, opMatcher);
+ }
+
+ Arguments type(NumericPair type, Operation op) {
+ opMatcher = ofType(type, op);
+
+ return Arguments.of(pair, expression, opMatcher);
+ }
+ }
+
+ private static Matcher<Object> ofBoolType(Boolean compResult) {
+ return new BaseMatcher<>() {
+ Object actual;
+
+ @Override
+ public boolean matches(Object actual) {
+ assert actual != null;
+ Pair<Object, ColumnMetadata> pair = (Pair<Object, ColumnMetadata>) actual;
+ this.actual = pair.getFirst();
+ return compResult.equals(this.actual);
+ }
+
+ @Override
+ public void describeTo(Description description) {
+ description.appendText(format("Expected : '{}' but found '{}'", compResult, actual));
+ }
+ };
+ }
+
+ private static Matcher<Object> ofType(NumericPair typesPair, Operation op) {
+ return new BaseMatcher<>() {
+ Object actual;
+ ClassInfoHolder innerClassInfo;
+ int precision = 0;
+ int scale = 0;
+ ColumnMetadata colMeta;
+
+ @Override
+ public boolean matches(Object actual) {
+ assert actual != null;
+ Pair<Object, ColumnMetadata> pair = (Pair<Object, ColumnMetadata>) actual;
+ this.actual = pair.getFirst();
+ colMeta = pair.getSecond();
+
+ boolean checkPrecisionScale = false;
+
+ if (this.actual instanceof BigDecimal) {
+ precision = ((BigDecimal) this.actual).precision();
+ scale = ((BigDecimal) this.actual).scale();
+ checkPrecisionScale = true;
+ }
+
+ innerClassInfo = TYPES_MAPPING.get(op).get(typesPair);
+
+ boolean precCheck = checkPrecisionScale ? colMeta.precision() >= precision && colMeta.scale() >= scale : true;
+
+ // negative scale in meta for operations with integer and real\double types.
+ return precCheck && innerClassInfo.clazz.isInstance(this.actual)
+ && innerClassInfo.precision == precision
+ && innerClassInfo.scale == scale;
+ }
+
+ @Override
+ public void describeTo(Description description) {
+ description.appendText(format("Expected : '{}' but found '{}, precision: {}, scale: {}', column meta: {}",
+ innerClassInfo, actual.getClass(), precision, scale, colMeta));
+ }
+ };
+ }
+
+ static {
Review Comment:
it would be nice to write test cases in the same style as it is for planner tests
```
private static Stream<Arguments> sumArgs() {
return Stream.of(
forTypePair(NumericPair.TINYINT_TINYINT)
.resultWillBe(ofType(NativeType.INT8))),
...
);
}
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/coercion/NumericComparisonExecutionTest.java:
##########
@@ -0,0 +1,993 @@
+/*
+ * 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.exec.coercion;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.math.BigDecimal;
+import java.util.Arrays;
+import java.util.EnumMap;
+import java.util.EnumSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.stream.Stream;
+import org.apache.ignite.internal.sql.engine.framework.DataProvider;
+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.util.SqlTestUtils;
+import org.apache.ignite.internal.type.DecimalNativeType;
+import org.apache.ignite.internal.type.NativeType;
+import org.apache.ignite.internal.type.NativeTypes;
+import org.apache.ignite.internal.util.Pair;
+import org.apache.ignite.sql.ColumnMetadata;
+import org.hamcrest.BaseMatcher;
+import org.hamcrest.Description;
+import org.hamcrest.Matcher;
+import org.junit.jupiter.api.Assumptions;
+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;
+
+/** Check execution and return results for numeric operations. */
+public class NumericComparisonExecutionTest extends BaseTypeCheckExecutionTest {
Review Comment:
let's split this class on two: one for binary arithmetic and another for comparison. Also, we need to cover the same comparison operators as in planner tests counterpart
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23141 Sql. Add execution test to verify numeric type coercion in binary comparison and arithmetic [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #4382:
URL: https://github.com/apache/ignite-3/pull/4382#discussion_r1764557114
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/coercion/NumericArithmeticsExecutionTest.java:
##########
@@ -0,0 +1,808 @@
+/*
+ * 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.exec.coercion;
+
+import static org.apache.ignite.internal.sql.engine.planner.datatypes.BaseTypeCoercionTest.checkIncludesAllNumericTypePairs;
+import static org.apache.ignite.internal.sql.engine.planner.datatypes.BaseTypeCoercionTest.forTypePair;
+
+import java.util.Set;
+import java.util.stream.Stream;
+import org.apache.ignite.internal.sql.engine.framework.DataProvider;
+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.util.SqlTestUtils;
+import org.apache.ignite.internal.type.DecimalNativeType;
+import org.apache.ignite.internal.type.NativeType;
+import org.apache.ignite.internal.type.NativeTypes;
+import org.hamcrest.Matcher;
+import org.junit.jupiter.api.Assumptions;
+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;
+
+/** Check execution and return results for numeric arithmetics. */
+public class NumericArithmeticsExecutionTest extends BaseTypeCheckExecutionTest {
+ private static final Set<NativeType> APPROXIMATE_NUMERIC_TYPES = Set.of(NativeTypes.DOUBLE, NativeTypes.FLOAT);
+
+ @ParameterizedTest
+ @MethodSource("sumArgs")
+ public void sumOp(TypePair typePair, Matcher<Object> resultMatcher) throws Exception {
+ String sql = "SELECT c1 + c2 FROM t";
+ try (ClusterWrapper testCluster = testCluster(typePair, eqDataProvider(typePair))) {
+ testCluster.process(sql, resultMatcher);
+ }
+ }
+
+ @ParameterizedTest
+ @MethodSource("subtractArgs")
+ public void subtractOp(TypePair typePair, Matcher<Object> resultMatcher) throws Exception {
+ String sql = "SELECT c1 - c2 FROM t";
+ try (ClusterWrapper testCluster = testCluster(typePair, eqDataProvider(typePair))) {
+ testCluster.process(sql, resultMatcher);
+ }
+ }
+
+ @ParameterizedTest
+ @MethodSource("multArgs")
+ public void multOp(TypePair typePair, Matcher<Object> resultMatcher) throws Exception {
+ String sql = "SELECT c1 * c2 FROM t";
+ try (ClusterWrapper testCluster = testCluster(typePair, eqDataProvider(typePair))) {
+ testCluster.process(sql, resultMatcher);
+ }
+ }
+
+ @ParameterizedTest
+ @MethodSource("divArgs")
+ public void divOp(TypePair typePair, Matcher<Object> resultMatcher) throws Exception {
+ String sql = "SELECT c1 / c2 FROM t";
+ Assumptions.assumeFalse(typePair.first() instanceof DecimalNativeType || typePair.second() instanceof DecimalNativeType,
+ "need to be fixed after: https://issues.apache.org/jira/browse/IGNITE-23171");
+
+ try (ClusterWrapper testCluster = testCluster(typePair, binOpDataProvider(typePair))) {
+ testCluster.process(sql, resultMatcher);
+ }
+ }
+
+ @ParameterizedTest
+ @MethodSource("moduloArgs")
+ public void moduloOp(TypePair typePair, Matcher<Object> resultMatcher) throws Exception {
+ // modulo is undefined for fractional types
+ if (APPROXIMATE_NUMERIC_TYPES.contains(typePair.first()) || APPROXIMATE_NUMERIC_TYPES.contains(typePair.second())) {
+ return;
+ }
+
+ String sql = "SELECT c1 % c2 FROM t";
+
+ try (ClusterWrapper testCluster = testCluster(typePair, eqDataProvider(typePair))) {
+ testCluster.process(sql, resultMatcher);
+ }
+ }
+
+ @Test
+ public void checkAllTypePairs() {
+ checkIncludesAllNumericTypePairs(sumArgs());
+ checkIncludesAllNumericTypePairs(subtractArgs());
+ checkIncludesAllNumericTypePairs(multArgs());
+ checkIncludesAllNumericTypePairs(divArgs());
+ checkIncludesAllNumericTypePairs(moduloArgs());
+ }
+
+ private static DataProvider<Object[]> binOpDataProvider(TypePair typePair) {
+ Object val1 = SqlTestUtils.generateValueByType(typePair.first());
+ Object val2 = SqlTestUtils.generateValueByType(typePair.second());
+
+ return DataProvider.fromRow(new Object[]{0, val1, val2}, 1);
+ }
+
+ private static Stream<Arguments> sumArgs() {
+ return Stream.of(
+ forTypePair(NumericPair.TINYINT_TINYINT).resultWillBe(NativeTypes.INT8),
+ forTypePair(NumericPair.TINYINT_SMALLINT).resultWillBe(NativeTypes.INT16),
+ forTypePair(NumericPair.TINYINT_INT).resultWillBe(NativeTypes.INT32),
+ forTypePair(NumericPair.TINYINT_BIGINT).resultWillBe(NativeTypes.INT64),
+ forTypePair(NumericPair.TINYINT_DECIMAL_1_0).resultWillBe(Types.DECIMAL_2_0),
+ forTypePair(NumericPair.TINYINT_DECIMAL_2_1).resultWillBe(Types.DECIMAL_3_1),
+ forTypePair(NumericPair.TINYINT_DECIMAL_4_3).resultWillBe(Types.DECIMAL_5_3),
+ forTypePair(NumericPair.TINYINT_DECIMAL_2_0).resultWillBe(Types.DECIMAL_2_0),
+ forTypePair(NumericPair.TINYINT_DECIMAL_3_1).resultWillBe(Types.DECIMAL_3_1),
+ forTypePair(NumericPair.TINYINT_DECIMAL_5_3).resultWillBe(Types.DECIMAL_5_3),
+ forTypePair(NumericPair.TINYINT_DECIMAL_5_0).resultWillBe(Types.DECIMAL_2_0),
+ forTypePair(NumericPair.TINYINT_DECIMAL_6_1).resultWillBe(Types.DECIMAL_3_1),
+ forTypePair(NumericPair.TINYINT_DECIMAL_8_3).resultWillBe(Types.DECIMAL_5_3),
Review Comment:
it looks suspiciously somthing + dec(8.3) = dec(5.3). I will still suggest using generate values with real precision and scale, otherwise, we are checking not we want.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23141 Sql. Add execution test to verify numeric type coercion in binary comparison and arithmetic [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on PR #4382:
URL: https://github.com/apache/ignite-3/pull/4382#issuecomment-2355488945
@korlov42 , @ygerzhedovich gyus, i cut it into two (arithmetic and comparison) tests and change comparison results for more convenient usage in linkage with planner tests. So i prefer to merge it as is, after completion [1] additional issue need to be filled for planner results re-usage in executions tests.
[1] https://issues.apache.org/jira/browse/IGNITE-23192
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org