This is an automated email from the ASF dual-hosted git repository. jooger 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 adf91949fc IGNITE-22421: Sql. Interval type. DDL statements should return a proper error (#3936) adf91949fc is described below commit adf91949fc10f11441f02bc9ac51cb27a0005c8c Author: Max Zhuravkov <shh...@gmail.com> AuthorDate: Thu Jun 20 13:00:13 2024 +0300 IGNITE-22421: Sql. Interval type. DDL statements should return a proper error (#3936) --- .../commands/AlterTableAddColumnCommand.java | 1 + .../internal/catalog/commands/CatalogUtils.java | 12 ++++ .../catalog/commands/CreateTableCommand.java | 2 + .../ignite/internal/catalog/CatalogTableTest.java | 8 ++- ...AlterTableAlterColumnCommandValidationTest.java | 64 ++++++++++++++++++++- .../commands/CreateTableCommandValidationTest.java | 59 +++++++++++++++++++ .../internal/sql/engine/ItCreateTableDdlTest.java | 30 ++++++++++ .../prepare/ddl/DdlSqlToCommandConverter.java | 13 +++++ .../prepare/ddl/DdlSqlToCommandConverterTest.java | 67 ++++++++++++++++++++++ 9 files changed, 252 insertions(+), 4 deletions(-) diff --git a/modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AlterTableAddColumnCommand.java b/modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AlterTableAddColumnCommand.java index d1cc894a3d..43de989d84 100644 --- a/modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AlterTableAddColumnCommand.java +++ b/modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/AlterTableAddColumnCommand.java @@ -105,6 +105,7 @@ public class AlterTableAddColumnCommand extends AbstractTableCommand { throw new CatalogValidationException(format("Column with name '{}' specified more than once", column.name())); } + CatalogUtils.ensureTypeCanBeStored(column.name(), column.type()); CatalogUtils.ensureNonFunctionalDefault(column.name(), column.defaultValueDefinition()); } } diff --git a/modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java b/modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java index 26cb9ed617..8a788341cb 100644 --- a/modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java +++ b/modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CatalogUtils.java @@ -597,4 +597,16 @@ public class CatalogUtils { throw new CatalogValidationException( format("Default of unsupported kind: [col={}, defaultType={}]", columnName, defaultValue.type)); } + + /** + * Check if provided column type can be persisted, or fail otherwise. + */ + // TODO: https://issues.apache.org/jira/browse/IGNITE-15200 + // Remove this after interval type support is added. + static void ensureTypeCanBeStored(String columnName, ColumnType columnType) { + if (columnType == ColumnType.PERIOD || columnType == ColumnType.DURATION) { + throw new CatalogValidationException( + format("Column of type '{}' cannot be persisted [col={}].", columnType, columnName)); + } + } } diff --git a/modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java b/modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java index 63f0b5f732..2322ef5cdd 100644 --- a/modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java +++ b/modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableCommand.java @@ -178,6 +178,8 @@ public class CreateTableCommand extends AbstractTableCommand { for (ColumnParams column : columns) { boolean partOfPk = primaryKey.columns().contains(column.name()); + + CatalogUtils.ensureTypeCanBeStored(column.name(), column.type()); if (partOfPk) { CatalogUtils.ensureSupportedDefault(column.name(), column.type(), column.defaultValueDefinition()); } else { diff --git a/modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogTableTest.java b/modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogTableTest.java index 78f91b81c9..403811b7e0 100644 --- a/modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogTableTest.java +++ b/modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogTableTest.java @@ -39,9 +39,11 @@ import static org.apache.ignite.internal.testframework.matchers.CompletableFutur import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully; import static org.apache.ignite.internal.util.CompletableFutures.falseCompletedFuture; import static org.apache.ignite.sql.ColumnType.DECIMAL; +import static org.apache.ignite.sql.ColumnType.DURATION; import static org.apache.ignite.sql.ColumnType.INT32; import static org.apache.ignite.sql.ColumnType.INT64; import static org.apache.ignite.sql.ColumnType.NULL; +import static org.apache.ignite.sql.ColumnType.PERIOD; import static org.apache.ignite.sql.ColumnType.STRING; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.empty; @@ -800,7 +802,7 @@ public class CatalogTableTest extends BaseCatalogManagerTest { * Changing precision is not supported for all types other than DECIMAL. */ @ParameterizedTest - @EnumSource(value = ColumnType.class, names = {"NULL", "DECIMAL"}, mode = Mode.EXCLUDE) + @EnumSource(value = ColumnType.class, names = {"NULL", "DECIMAL", "PERIOD", "DURATION"}, mode = Mode.EXCLUDE) public void testAlterColumnTypeAnyPrecisionChangeIsRejected(ColumnType type) { ColumnParams pkCol = columnParams("ID", INT32); ColumnParams colWithPrecision; @@ -1008,10 +1010,12 @@ public class CatalogTableTest extends BaseCatalogManagerTest { * All other transitions are forbidden because they lead to incompatible schemas. */ @ParameterizedTest(name = "set data type {0}") - @EnumSource(value = ColumnType.class, names = "NULL", mode = Mode.EXCLUDE) + @EnumSource(value = ColumnType.class, names = {"NULL", "PERIOD", "DURATION"}, mode = Mode.EXCLUDE) public void testAlterColumnType(ColumnType target) { EnumSet<ColumnType> types = EnumSet.allOf(ColumnType.class); types.remove(NULL); + types.remove(PERIOD); + types.remove(DURATION); List<ColumnParams> testColumns = types.stream() .map(t -> initializeColumnWithDefaults(t, columnParamsBuilder("COL_" + t, t))) diff --git a/modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/AlterTableAlterColumnCommandValidationTest.java b/modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/AlterTableAlterColumnCommandValidationTest.java index 8f8b472bcc..65d7884020 100644 --- a/modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/AlterTableAlterColumnCommandValidationTest.java +++ b/modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/AlterTableAlterColumnCommandValidationTest.java @@ -19,7 +19,10 @@ package org.apache.ignite.internal.catalog.commands; import static org.apache.ignite.internal.catalog.CatalogTestUtils.initializeColumnWithDefaults; import static org.apache.ignite.internal.lang.IgniteStringFormatter.format; +import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows; import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause; +import static org.apache.ignite.sql.ColumnType.DURATION; +import static org.apache.ignite.sql.ColumnType.PERIOD; import java.util.ArrayList; import java.util.List; @@ -306,8 +309,10 @@ public class AlterTableAlterColumnCommandValidationTest extends AbstractCommandV ); } + // TODO: https://issues.apache.org/jira/browse/IGNITE-15200 + // Include DURATION and PERIOD types after these types are supported. @ParameterizedTest - @EnumSource(mode = Mode.EXCLUDE, value = ColumnType.class, names = {"DECIMAL", "NULL"}) + @EnumSource(mode = Mode.EXCLUDE, value = ColumnType.class, names = {"DECIMAL", "NULL", "DURATION", "PERIOD"}) void precisionCannotBeChangedIfTypeIsNotDecimal(ColumnType type) { String tableName = "TEST"; String columnName = "VAL"; @@ -421,8 +426,10 @@ public class AlterTableAlterColumnCommandValidationTest extends AbstractCommandV ); } + // TODO: https://issues.apache.org/jira/browse/IGNITE-15200 + // Include DURATION and PERIOD types after these types are supported. @ParameterizedTest - @EnumSource(mode = Mode.EXCLUDE, value = ColumnType.class, names = {"STRING", "BYTE_ARRAY", "NULL"}) + @EnumSource(mode = Mode.EXCLUDE, value = ColumnType.class, names = {"STRING", "BYTE_ARRAY", "NULL", "DURATION", "PERIOD"}) void lengthCannotBeChangedForNonVariableTypes(ColumnType type) { String tableName = "TEST"; String columnName = "VAL"; @@ -671,10 +678,63 @@ public class AlterTableAlterColumnCommandValidationTest extends AbstractCommandV ); } + // TODO: https://issues.apache.org/jira/browse/IGNITE-15200 + // Remove this after interval type support is added. + @ParameterizedTest + @EnumSource(value = ColumnType.class, names = {"PERIOD", "DURATION"}, mode = Mode.INCLUDE) + void rejectIntervalTypes(ColumnType columnType) { + String error = format("Column of type '{}' cannot be persisted [col=P]", columnType); + + { + ColumnParams val = ColumnParams.builder() + .name("P") + .type(columnType) + .precision(2) + .nullable(false) + .build(); + + AlterTableAddColumnCommandBuilder builder = AlterTableAddColumnCommand.builder() + .tableName("T") + .schemaName(SCHEMA_NAME) + .columns(List.of(val)); + + assertThrows( + CatalogValidationException.class, + builder::build, + error + ); + } + + { + ColumnParams val = ColumnParams.builder() + .name("P") + .type(columnType) + .precision(2) + .nullable(true) + .build(); + + AlterTableAddColumnCommandBuilder builder = AlterTableAddColumnCommand.builder() + .tableName("T") + .schemaName(SCHEMA_NAME) + .columns(List.of(val)); + + assertThrows( + CatalogValidationException.class, + builder::build, + error + ); + } + } + private static Stream<Arguments> invalidTypeConversionPairs() { List<Arguments> arguments = new ArrayList<>(); for (ColumnType from : ColumnType.values()) { for (ColumnType to : ColumnType.values()) { + // TODO: https://issues.apache.org/jira/browse/IGNITE-15200 + // Remove this after interval type support is added. + if (from == DURATION || to == DURATION || from == PERIOD || to == PERIOD) { + continue; + } if (from != to && !CatalogUtils.isSupportedColumnTypeChange(from, to) && from != ColumnType.NULL) { arguments.add(Arguments.of(from, to)); } diff --git a/modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CreateTableCommandValidationTest.java b/modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CreateTableCommandValidationTest.java index 8c2e93ec0d..f8a2498330 100644 --- a/modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CreateTableCommandValidationTest.java +++ b/modules/catalog/src/test/java/org/apache/ignite/internal/catalog/commands/CreateTableCommandValidationTest.java @@ -18,6 +18,7 @@ package org.apache.ignite.internal.catalog.commands; import static org.apache.ignite.internal.lang.IgniteStringFormatter.format; +import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows; import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause; import static org.apache.ignite.sql.ColumnType.INT32; import static org.apache.ignite.sql.ColumnType.UUID; @@ -29,9 +30,12 @@ import org.apache.ignite.internal.catalog.Catalog; import org.apache.ignite.internal.catalog.CatalogCommand; import org.apache.ignite.internal.catalog.CatalogValidationException; import org.apache.ignite.internal.catalog.descriptors.CatalogColumnCollation; +import org.apache.ignite.sql.ColumnType; 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; +import org.junit.jupiter.params.provider.EnumSource.Mode; import org.junit.jupiter.params.provider.MethodSource; /** @@ -480,4 +484,59 @@ public class CreateTableCommandValidationTest extends AbstractCommandValidationT command.get(newCatalog); }); } + + // TODO: https://issues.apache.org/jira/browse/IGNITE-15200 + // Remove this after interval type support is added. + @ParameterizedTest + @EnumSource(value = ColumnType.class, names = {"PERIOD", "DURATION"}, mode = Mode.INCLUDE) + void rejectIntervalTypes(ColumnType columnType) { + ColumnParams id = ColumnParams.builder() + .name("ID") + .type(INT32) + .build(); + + String error = format("Column of type '{}' cannot be persisted [col=P]", columnType); + + { + ColumnParams val = ColumnParams.builder() + .name("P") + .type(columnType) + .precision(2) + .nullable(false) + .build(); + + CreateTableCommandBuilder builder = CreateTableCommand.builder() + .tableName("T") + .schemaName(SCHEMA_NAME) + .columns(List.of(id, val)) + .primaryKey(primaryKey("ID")); + + assertThrows( + CatalogValidationException.class, + builder::build, + error + ); + } + + { + ColumnParams val = ColumnParams.builder() + .name("P") + .type(columnType) + .precision(2) + .nullable(true) + .build(); + + CreateTableCommandBuilder builder = CreateTableCommand.builder() + .tableName("T") + .schemaName(SCHEMA_NAME) + .columns(List.of(id, val)) + .primaryKey(primaryKey("ID")); + + assertThrows( + CatalogValidationException.class, + builder::build, + error + ); + } + } } diff --git a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItCreateTableDdlTest.java b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItCreateTableDdlTest.java index 82191c3b7c..7781bcbb70 100644 --- a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItCreateTableDdlTest.java +++ b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItCreateTableDdlTest.java @@ -446,6 +446,36 @@ public class ItCreateTableDdlTest extends BaseSqlIntegrationTest { sql("DROP TABLE \"table\"\"Test\"\"\""); } + // TODO: https://issues.apache.org/jira/browse/IGNITE-15200 + // Remove this after interval type support is added. + @Test + public void testCreateTableDoesNotAllowIntervals() { + assertThrowsSqlException( + STMT_VALIDATION_ERR, + "Type INTERVAL YEAR cannot be used in a column definition [column=ID]", + () -> sql("CREATE TABLE test(id INTERVAL YEAR PRIMARY KEY, val INT)") + ); + + assertThrowsSqlException( + STMT_VALIDATION_ERR, + "Type INTERVAL YEAR cannot be used in a column definition [column=P]", + () -> sql("CREATE TABLE test(id INTEGER PRIMARY KEY, p INTERVAL YEAR)") + ); + } + + // TODO: https://issues.apache.org/jira/browse/IGNITE-15200 + // Remove this after interval type support is added. + @Test + public void testAlterTableDoesNotAllowIntervals() { + sql("CREATE TABLE test(id INTEGER PRIMARY KEY, val INTEGER)"); + + assertThrowsSqlException( + STMT_VALIDATION_ERR, + "Type INTERVAL YEAR cannot be used in a column definition [column=P]", + () -> sql("ALTER TABLE TEST ADD COLUMN p INTERVAL YEAR") + ); + } + private static CatalogZoneDescriptor getDefaultZone(IgniteImpl node) { CatalogManager catalogManager = node.catalogManager(); Catalog catalog = catalogManager.catalog(catalogManager.activeCatalogVersion(node.clock().nowLong())); diff --git a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java index aa5e213d8a..22ab2f73d3 100644 --- a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java +++ b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java @@ -78,6 +78,7 @@ import org.apache.calcite.sql.ddl.SqlDdlNodes; import org.apache.calcite.sql.parser.SqlParserPos; import org.apache.calcite.sql.parser.SqlParserUtil; import org.apache.calcite.sql.type.SqlTypeName; +import org.apache.calcite.sql.type.SqlTypeUtil; import org.apache.calcite.util.DateString; import org.apache.calcite.util.TimeString; import org.apache.calcite.util.TimestampString; @@ -382,6 +383,18 @@ public class DdlSqlToCommandConverter { String name = col.name.getSimple(); RelDataType relType = planner.convert(col.dataType, nullable); + + // TODO: https://issues.apache.org/jira/browse/IGNITE-15200 + // Remove this after interval type support is added. + if (SqlTypeUtil.isInterval(relType)) { + String error = format( + "Type {} cannot be used in a column definition [column={}].", + relType.getSqlTypeName().getSpaceName(), + name + ); + throw new SqlException(STMT_VALIDATION_ERR, error); + } + ColumnTypeParams typeParams = new ColumnTypeParams(relType); return ColumnParams.builder() diff --git a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverterTest.java b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverterTest.java index 2ff25506e7..1167fb509d 100644 --- a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverterTest.java +++ b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverterTest.java @@ -96,6 +96,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestFactory; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Mockito; @@ -302,6 +303,7 @@ public class DdlSqlToCommandConverterTest extends AbstractDdlSqlToCommandConvert } @TestFactory + @Disabled("https://issues.apache.org/jira/browse/IGNITE-15200") public Stream<DynamicTest> numericDefaultWithIntervalTypes() { List<DynamicTest> testItems = new ArrayList<>(); PlanningContext ctx = createContext(); @@ -338,6 +340,7 @@ public class DdlSqlToCommandConverterTest extends AbstractDdlSqlToCommandConvert } @TestFactory + @Disabled("https://issues.apache.org/jira/browse/IGNITE-15200") public Stream<DynamicTest> nonIntervalDefaultsWithIntervalTypes() { List<DynamicTest> testItems = new ArrayList<>(); PlanningContext ctx = createContext(); @@ -667,6 +670,70 @@ public class DdlSqlToCommandConverterTest extends AbstractDdlSqlToCommandConvert assertThat(ex.getMessage(), containsString("String cannot be empty")); } + // TODO: https://issues.apache.org/jira/browse/IGNITE-15200 + // Remove this after interval type support is added. + @ParameterizedTest + @MethodSource("intervalTypeNames") + public void testCreateTableDoNotAllowIntervalTypes(SqlTypeName sqlTypeName) throws SqlParseException { + String typeName = intervalSqlName(sqlTypeName); + String error = format("Type {} cannot be used in a column definition [column=P].", sqlTypeName.getSpaceName()); + + { + var node = parse(format("CREATE TABLE t (id INTEGER PRIMARY KEY, p INTERVAL {})", typeName)); + assertThat(node, instanceOf(SqlDdl.class)); + + assertThrowsSqlException(STMT_VALIDATION_ERR, error, + () -> converter.convert((SqlDdl) node, createContext())); + } + + { + var node = parse(format("CREATE TABLE t (id INTEGER PRIMARY KEY, p INTERVAL {} NOT NULL)", typeName)); + assertThat(node, instanceOf(SqlDdl.class)); + + assertThrowsSqlException(STMT_VALIDATION_ERR, error, + () -> converter.convert((SqlDdl) node, createContext())); + } + } + + // TODO: https://issues.apache.org/jira/browse/IGNITE-15200 + // Remove this after interval type support is added. + @ParameterizedTest + @MethodSource("intervalTypeNames") + public void testAlterTableNotAllowIntervalTypes(SqlTypeName sqlTypeName) throws SqlParseException { + String typeName = intervalSqlName(sqlTypeName); + String error = format("Type {} cannot be used in a column definition [column=P].", sqlTypeName.getSpaceName()); + + { + var node = parse(format("ALTER TABLE t ADD COLUMN p INTERVAL {}", typeName)); + assertThat(node, instanceOf(SqlDdl.class)); + + assertThrowsSqlException(STMT_VALIDATION_ERR, error, + () -> converter.convert((SqlDdl) node, createContext())); + } + + { + var node = parse(format("ALTER TABLE t ADD COLUMN p INTERVAL {} NOT NULL", typeName)); + assertThat(node, instanceOf(SqlDdl.class)); + + assertThrowsSqlException(STMT_VALIDATION_ERR, error, + () -> converter.convert((SqlDdl) node, createContext())); + } + } + + private static Set<SqlTypeName> intervalTypeNames() { + return INTERVAL_TYPES; + } + + private static String intervalSqlName(SqlTypeName intervalType) { + String typeName; + if (intervalType.getStartUnit() != intervalType.getEndUnit()) { + typeName = intervalType.getStartUnit().name() + " TO " + intervalType.getEndUnit().name(); + } else { + typeName = intervalType.getStartUnit().name(); + } + return typeName; + } + private static Matcher<CatalogTableColumnDescriptor> columnThat(String description, Function<CatalogTableColumnDescriptor, Boolean> checker) { return new CustomMatcher<>(description) {