This is an automated email from the ASF dual-hosted git repository. zstan pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/ignite-3.git
The following commit(s) were added to refs/heads/main by this push: new b8175146dd IGNITE-22158: Sql. incorrect result of NATURAL JOIN for BIGINTEGER column on condition (#3791) b8175146dd is described below commit b8175146ddb0f357228b59bffeef5c6f01631861 Author: Max Zhuravkov <shh...@gmail.com> AuthorDate: Mon May 27 14:05:52 2024 +0300 IGNITE-22158: Sql. incorrect result of NATURAL JOIN for BIGINTEGER column on condition (#3791) --- .../ignite/internal/sql/engine/ItJoinTest.java | 38 +++++++++ .../sql/join/natural/natural_join.test | 81 ------------------ ...ral_join.test_ignore => test_natural_join.test} | 41 +++------ .../natural/test_natural_join_different_types.test | 50 +++++++++++ .../sql/engine/prepare/IgniteSqlValidator.java | 47 +++++++++++ .../internal/sql/engine/util/IgniteResource.java | 4 + .../ignite/internal/sql/engine/util/TypeUtils.java | 24 ++++++ .../calcite/runtime/CalciteResource.properties | 2 + .../engine/planner/JoinWithUsingPlannerTest.java | 96 ++++++++++++++++++++++ 9 files changed, 273 insertions(+), 110 deletions(-) diff --git a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItJoinTest.java b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItJoinTest.java index 191ef5b901..f61e5873ff 100644 --- a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItJoinTest.java +++ b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItJoinTest.java @@ -17,13 +17,17 @@ package org.apache.ignite.internal.sql.engine; +import static org.apache.ignite.internal.sql.engine.util.SqlTestUtils.assertThrowsSqlException; + import java.util.Arrays; import java.util.List; import java.util.stream.Stream; import org.apache.ignite.internal.sql.BaseSqlIntegrationTest; import org.apache.ignite.internal.sql.engine.util.QueryChecker; import org.apache.ignite.internal.testframework.WithSystemProperty; +import org.apache.ignite.lang.ErrorGroups.Sql; import org.junit.jupiter.api.BeforeAll; +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.EnumSource; @@ -1073,4 +1077,38 @@ public class ItJoinTest extends BaseSqlIntegrationTest { return types; } + + @Test + // TODO this test case can be removed after https://issues.apache.org/jira/browse/IGNITE-22295 + public void testNaturalJoinTypeMismatch() { + try { + sql("CREATE TABLE t1_ij (i INTEGER PRIMARY KEY, j INTEGER);"); + sql("CREATE TABLE t2_ij (i INTEGER PRIMARY KEY, j BIGINT);"); + + var expectedMessage = "Column N#1 matched using NATURAL keyword or USING clause " + + "has incompatible types in this context: 'INTEGER' to 'BIGINT'"; + + assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, expectedMessage, () -> sql("SELECT * FROM t1_ij NATURAL JOIN t2_ij")); + } finally { + sql("DROP TABLE t1_ij"); + sql("DROP TABLE t2_ij"); + } + } + + @Test + // TODO this test case can be removed after https://issues.apache.org/jira/browse/IGNITE-22295 + public void testUsingJoinTypeMismatch() { + try { + sql("CREATE TABLE t1_ij (i INTEGER PRIMARY KEY, j INTEGER);"); + sql("CREATE TABLE t2_ij (i INTEGER PRIMARY KEY, j BIGINT);"); + + var expectedMessage = "Column N#1 matched using NATURAL keyword or USING clause " + + "has incompatible types in this context: 'INTEGER' to 'BIGINT'"; + + assertThrowsSqlException(Sql.STMT_VALIDATION_ERR, expectedMessage, () -> sql("SELECT * FROM t1_ij JOIN t2_ij USING (i)")); + } finally { + sql("DROP TABLE t1_ij"); + sql("DROP TABLE t2_ij"); + } + } } diff --git a/modules/sql-engine/src/integrationTest/sql/join/natural/natural_join.test b/modules/sql-engine/src/integrationTest/sql/join/natural/natural_join.test deleted file mode 100644 index a88207bd22..0000000000 --- a/modules/sql-engine/src/integrationTest/sql/join/natural/natural_join.test +++ /dev/null @@ -1,81 +0,0 @@ -# name: test/sql/join/natural/natural_join.test -# description: Test natural joins -# group: [natural] - -statement ok -PRAGMA enable_verification - -# create tables -statement ok -CREATE TABLE t1 (a INTEGER, b INTEGER) - -statement ok -INSERT INTO t1 VALUES (1, 2) - -# https://issues.apache.org/jira/browse/IGNITE-22158 -# Let's amend both type for t2 table to BIGINT after resolve the ticket. -statement ok -CREATE TABLE t2 (a INTEGER, c INTEGER) - -statement ok -INSERT INTO t2 VALUES (1, 3), (2, 4) - -# NATURAL join with one column -query III -SELECT * FROM t1 NATURAL JOIN t2 ----- -1 2 3 - -query III -SELECT t1.a, t1.b, t2.c FROM t1 NATURAL JOIN t2 ----- -1 2 3 - -query III -SELECT t1.a, t1.b, t2.c FROM t1 NATURAL JOIN t2 ORDER BY t2.a ----- -1 2 3 - -# natural join with multiple matching columns -statement ok -CREATE TABLE t3 (a INTEGER, b INTEGER, c INTEGER) - -statement ok -INSERT INTO t3 VALUES (1, 2, 3) - -query III -SELECT * FROM t1 NATURAL JOIN t3 ----- -1 2 3 - -# common columns moved to the first place, column order: a, c, b -query III -SELECT * FROM t3 NATURAL JOIN t2 ----- -1 3 2 - -# when there no matching columns are present natural join behaves like a cross join -skipif ignite3 -# https://issues.apache.org/jira/browse/IGNITE-18668 -query I -select * from (values (1)) tbl(a) natural join (values (1), (2)) tbl2(b) order by 1, 2 ----- -1 1 -1 2 - -# natural join with subqueries -query I -select * from (select 42) tbl(a) natural join (select 42) tbl2(a) ----- -42 - -# uncorrelated scalar subquery -query I -select (select * from (select 42) tbl(a) natural join (select 42) tbl2(a)) ----- -42 - -# error: duplicate table alias on both sides -statement error -select (select * from (select 42) tbl(a) natural join (select 42) tbl(a)) - diff --git a/modules/sql-engine/src/integrationTest/sql/join/natural/natural_join.test_ignore b/modules/sql-engine/src/integrationTest/sql/join/natural/test_natural_join.test similarity index 73% rename from modules/sql-engine/src/integrationTest/sql/join/natural/natural_join.test_ignore rename to modules/sql-engine/src/integrationTest/sql/join/natural/test_natural_join.test index 8893cdf222..fa3d4f5502 100644 --- a/modules/sql-engine/src/integrationTest/sql/join/natural/natural_join.test_ignore +++ b/modules/sql-engine/src/integrationTest/sql/join/natural/test_natural_join.test @@ -1,8 +1,6 @@ # name: test/sql/join/natural/natural_join.test # description: Test natural joins # group: [natural] -# Ignore https://issues.apache.org/jira/browse/IGNITE-15573 -# Ignore https://issues.apache.org/jira/browse/IGNITE-16046 statement ok PRAGMA enable_verification @@ -48,24 +46,28 @@ SELECT * FROM t1 NATURAL JOIN t3 ---- 1 2 3 -query III -SELECT a, b, c FROM t3 NATURAL JOIN t2 ----- -1 2 3 - # common columns moved to the first place, column order: a, c, b query III SELECT * FROM t3 NATURAL JOIN t2 ---- 1 3 2 +skipif ignite3 +# https://issues.apache.org/jira/browse/IGNITE-22307 +query III +SELECT a, b, c FROM t3 NATURAL JOIN t2 +---- +1 2 3 + # natural join chain query III SELECT * FROM t1 NATURAL JOIN t2 NATURAL JOIN t3 ---- 1 2 3 -# natural join chain on not matching columns behaves like join on true +# when there no matching columns are present natural join behaves like a cross join +skipif ignite3 +# https://issues.apache.org/jira/browse/IGNITE-18668 query I select * from (values (1)) tbl(a) natural join (values (1), (2)) tbl2(b) order by 1, 2 ---- @@ -95,28 +97,9 @@ select (select * from (select 42) tbl(a) natural join (select 42) tbl2(a)) statement error select (select * from (select 42) tbl(a) natural join (select 42) tbl(a)) -statement ok -DROP TABLE t1 - -statement ok -CREATE TABLE t0(c0 DATE, c1 DATE DEFAULT('0.5868720116119102'), c2 INT1, PRIMARY KEY(c1, c2, c0)); - -statement ok -CREATE TABLE t1(c0 DATETIME, c1 DATE DEFAULT(TIMESTAMP '1970-01-11 02:37:59'), PRIMARY KEY(c0)); - -statement ok -CREATE VIEW v0(c0) AS SELECT false FROM t1, t0 HAVING 1689380428; - -statement ok -SELECT COUNT(t1.rowid) FROM t1, v0 NATURAL RIGHT JOIN t0; - -statement ok -SELECT COUNT(t1.rowid) FROM t1, v0 RIGHT JOIN t0 ON v0.c0=t0.c0; - -statement error -SELECT COUNT(t1.rowid) FROM t1, v0 RIGHT JOIN t0 ON t1.c1=t0.c1 AND v0.c0=t0.c0; - +skipif ignite3 # column name appears more than once on left side of the natural join +# https://issues.apache.org/jira/browse/IGNITE-22306 statement error select * from (values (1)) t1(i) join (values (1)) t2(i) on (t1.i=t2.i) natural join (values (1)) t3(i); diff --git a/modules/sql-engine/src/integrationTest/sql/join/natural/test_natural_join_different_types.test b/modules/sql-engine/src/integrationTest/sql/join/natural/test_natural_join_different_types.test new file mode 100644 index 0000000000..27734cd79c --- /dev/null +++ b/modules/sql-engine/src/integrationTest/sql/join/natural/test_natural_join_different_types.test @@ -0,0 +1,50 @@ +# name: test/join/natural/test_natural_join_different_types.test +# description: Test natural joins. Common column types differ +# group: [natural] + +statement ok +CREATE TABLE t1_ij (i INTEGER, j INTEGER); + +statement ok +INSERT INTO t1_ij VALUES (1, 1), (2, 2); + +# Check against a NOT NULL type + +statement ok +CREATE TABLE t1_ij_not_null (i INTEGER NOT NULL, j INTEGER); + +statement ok +INSERT INTO t1_ij_not_null VALUES (1, 1), (2, 2); + +statement ok +INSERT INTO t1_ij VALUES (NULL, NULL); + +query II rowsort +SELECT * FROM t1_ij JOIN t1_ij_not_null USING (i, j) +---- +1 1 +2 2 + +statement ok +CREATE TABLE t1_ij_i_bigint (i BIGINT, j INTEGER); + +# Ignite validation + +# Test should not produce an error after https://issues.apache.org/jira/browse/IGNITE-22295 is fixed +statement error: Column N#0 matched using NATURAL keyword or USING clause has incompatible types in this context: 'INTEGER' to 'BIGINT' +SELECT * FROM t1_ij NATURAL JOIN t1_ij_i_bigint + +# Test should not produce an error after https://issues.apache.org/jira/browse/IGNITE-22295 is fixed +statement error: Column N#0 matched using NATURAL keyword or USING clause has incompatible types in this context: 'INTEGER' to 'BIGINT' +SELECT * FROM t1_ij JOIN t1_ij_i_bigint USING (i, j) + +# Calcite's validation + +statement ok +CREATE TABLE t1_ij_i_date (i DATE, j INTEGER); + +statement error: Column 'I' matched using NATURAL keyword or USING clause has incompatible types in this context: 'INTEGER' to 'DATE' +SELECT * FROM t1_ij NATURAL JOIN t1_ij_i_date + +statement error: Column 'I' matched using NATURAL keyword or USING clause has incompatible types in this context: 'INTEGER' to 'DATE' +SELECT* FROM t1_ij NATURAL JOIN t1_ij_i_date USING (i) diff --git a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java index e19b737ccc..c2b66f287d 100644 --- a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java +++ b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java @@ -744,6 +744,16 @@ public class IgniteSqlValidator extends SqlValidatorImpl { } } + @Override + protected void validateJoin(SqlJoin join, SqlValidatorScope scope) { + super.validateJoin(join, scope); + + if (join.isNatural() || join.getConditionType() == JoinConditionType.USING) { + // TODO Remove this method after https://issues.apache.org/jira/browse/IGNITE-22295 + validateJoinCondition(join); + } + } + /** {@inheritDoc} */ @Override protected SqlNode performUnconditionalRewrites(SqlNode node, boolean underFrom) { @@ -809,6 +819,43 @@ public class IgniteSqlValidator extends SqlValidatorImpl { } } + /** Rewrite NATURAL join condition into a predicate. */ + private void validateJoinCondition(SqlJoin join) { + SqlValidatorNamespace leftNs = getNamespace(join.getLeft()); + requireNonNull(leftNs, "leftNs"); + + SqlValidatorNamespace rightNs = getNamespace(join.getRight()); + requireNonNull(leftNs, "rightNs"); + + List<String> joinColumnList = SqlValidatorUtil.deriveNaturalJoinColumnList( + getCatalogReader().nameMatcher(), + leftNs.getRowType(), + rightNs.getRowType()); + + // Natural join between relations with a disjoint set of common columns + if (joinColumnList.isEmpty()) { + return; + } + + for (int i = 0; i < joinColumnList.size(); i++) { + String col = joinColumnList.get(i); + RelDataTypeField leftField = leftNs.getRowType().getField(col, true, false); + RelDataTypeField rightField = rightNs.getRowType().getField(col, true, false); + + assert leftField != null; + assert rightField != null; + + RelDataType leftType = leftField.getType(); + RelDataType rightType = rightField.getType(); + + if (!TypeUtils.typesRepresentTheSameColumnTypes(leftType, rightType)) { + throw newValidationError(join, IgniteResource.INSTANCE.naturalOrUsingColumnNotCompatible( + i, leftType.toString(), rightType.toString()) + ); + } + } + } + private void validateAggregateFunction(SqlCall call, SqlAggFunction aggFunction) { if (!SqlKind.AGGREGATE.contains(aggFunction.kind)) { throw newValidationError(call, diff --git a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java index 3191a65e69..405c48a197 100644 --- a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java +++ b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java @@ -84,6 +84,10 @@ public interface IgniteResource { @Resources.BaseMessage("Length for type {0} must be at least 1") Resources.ExInst<SqlValidatorException> invalidStringLength(String typeName); + @Resources.BaseMessage("Column N#{0} matched using NATURAL keyword or USING clause " + + "has incompatible types in this context: ''{1}'' to ''{2}''") + Resources.ExInst<SqlValidatorException> naturalOrUsingColumnNotCompatible(int num, String type1, String type2); + /** Constructs a signature string to use in error messages. */ static String makeSignature(SqlCallBinding binding, RelDataType... operandTypes) { return makeSignature(binding, Arrays.asList(operandTypes)); diff --git a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/TypeUtils.java b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/TypeUtils.java index 3e41578427..0877a04ae2 100644 --- a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/TypeUtils.java +++ b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/TypeUtils.java @@ -754,4 +754,28 @@ public class TypeUtils { return bldr; } + + /** + * Checks whether or not the given types represent the same column types. + * + * @param lhs Left type. + * @param rhs Right type. + * @return {@code true} if types represent the same {@link ColumnType} after conversion. + */ + // TODO this method can be removed after https://issues.apache.org/jira/browse/IGNITE-22295 + public static boolean typesRepresentTheSameColumnTypes(RelDataType lhs, RelDataType rhs) { + // IgniteCustomType: check for custom data type, otherwise this expression can fail when type is converted into column type. + if (isCustomType(lhs) && isCustomType(rhs) || SqlTypeUtil.isAtomic(lhs) && SqlTypeUtil.isAtomic(rhs)) { + ColumnType col1 = columnType(lhs); + ColumnType col2 = columnType(rhs); + + return col1 == col2; + } else { + return false; + } + } + + private static boolean isCustomType(RelDataType type) { + return type instanceof IgniteCustomType; + } } diff --git a/modules/sql-engine/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties b/modules/sql-engine/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties index 2671960e62..d3a1ceccde 100644 --- a/modules/sql-engine/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties +++ b/modules/sql-engine/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties @@ -19,3 +19,5 @@ # This message should be the same as or very similar to the one produced by # org.apache.ignite.internal.schema.Column::nullConstraintViolationMessage ColumnNotNullable=Column ''{0}'' does not allow NULLs +# TODO Remove this after https://issues.apache.org/jira/browse/IGNITE-22295 +NaturalOrUsingColumnNotCompatible=Column ''{0}'' matched using NATURAL keyword or USING clause has incompatible types in this context: ''{1}'' to ''{2}'' diff --git a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/JoinWithUsingPlannerTest.java b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/JoinWithUsingPlannerTest.java index 8306cf1e09..c38f9ec78d 100644 --- a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/JoinWithUsingPlannerTest.java +++ b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/JoinWithUsingPlannerTest.java @@ -17,14 +17,32 @@ package org.apache.ignite.internal.sql.engine.planner; +import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows; +import static org.junit.jupiter.api.Assertions.assertEquals; + import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.runtime.CalciteContextException; +import org.apache.calcite.sql.type.SqlTypeName; import org.apache.ignite.internal.sql.engine.framework.TestBuilders; import org.apache.ignite.internal.sql.engine.schema.IgniteSchema; import org.apache.ignite.internal.sql.engine.trait.IgniteDistributions; +import org.apache.ignite.internal.sql.engine.util.Commons; +import org.apache.ignite.internal.sql.engine.util.TypeUtils; +import org.apache.ignite.internal.type.NativeType; import org.apache.ignite.internal.type.NativeTypes; +import org.apache.ignite.sql.ColumnType; import org.junit.jupiter.api.BeforeAll; 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; /** * Join with USING syntax tests. @@ -155,4 +173,82 @@ public class JoinWithUsingPlannerTest extends AbstractPlannerTest { // assertPlan("SELECT *, T2._KEY FROM T1 NATURAL JOIN T2", schemas, // hasColumns("DEPTID", "NAME", "EMPID", "PARENTID", "_KEY")); } + + @ParameterizedTest + @MethodSource("nativeTypesMatrix") + public void testNaturalOrUsingJoinWithDifferentTypes(boolean natural, TypeArg type1, TypeArg type2) throws Exception { + + NativeType left = type1.nativeType; + NativeType right = type2.nativeType; + + IgniteSchema publicSchema = createSchema("PUBLIC", + TestBuilders.table().name("T1") + .addColumn("A", left) + .addColumn("B", NativeTypes.INT32) + .distribution(IgniteDistributions.random()) + .build(), + TestBuilders.table().name("T2") + .addColumn("C", NativeTypes.STRING) + .addColumn("A", right) + .distribution(IgniteDistributions.random()) + .build() + ); + + String query = natural + ? "SELECT * FROM T1 NATURAL JOIN T2" + : "SELECT * FROM T1 JOIN T2 USING (A)"; + + if (left.mismatch(right)) { + assertThrows(CalciteContextException.class, + () -> physicalPlan(query, publicSchema), + "NATURAL keyword or USING clause has incompatible types" + ); + } else { + RelDataType rowType = physicalPlan(query, publicSchema).getRowType(); + assertEquals(List.of("A", "B", "C"), rowType.getFieldNames()); + + SqlTypeName joinColType = rowType.getFieldList().get(0).getType().getSqlTypeName(); + SqlTypeName expectedNativeType = TypeUtils.native2relationalType(Commons.typeFactory(), left).getSqlTypeName(); + + assertEquals(joinColType, expectedNativeType); + } + } + + private static Stream<Arguments> nativeTypesMatrix() { + Set<ColumnType> unsupportedTypes = Set.of( + ColumnType.NULL, + ColumnType.NUMBER, + // TODO Exclude interval types after https://issues.apache.org/jira/browse/IGNITE-15200 + ColumnType.PERIOD, + ColumnType.DURATION, + // TODO Exclude BitMask type after https://issues.apache.org/jira/browse/IGNITE-18431 + ColumnType.BITMASK + ); + + List<TypeArg> types1 = Arrays.stream(ColumnType.values()) + .filter(c -> !unsupportedTypes.contains(c)) + .map(c -> TypeUtils.columnType2NativeType(c, 5, 2, 5)) + .map(TypeArg::new) + .collect(Collectors.toList()); + + List<Boolean> naturalJoinCondition = List.of(true, false); + + return types1.stream().flatMap(t1 -> naturalJoinCondition.stream() + .flatMap(b -> types1.stream().map(t2 -> Arguments.of(b, t1, t2)) + )); + } + + static class TypeArg { + + final NativeType nativeType; + + TypeArg(NativeType nativeType) { + this.nativeType = nativeType; + } + + @Override + public String toString() { + return nativeType.spec().name(); + } + } }