rpuch commented on code in PR #2493:
URL: https://github.com/apache/ignite-3/pull/2493#discussion_r1305249846
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -947,14 +836,45 @@ private static void
ensureNoTableOrIndexExistsWithSameName(CatalogSchemaDescript
}
private static void validateIndexColumns(CatalogTableDescriptor table,
AbstractCreateIndexCommandParams params) {
- for (String indexColumn : params.columns()) {
- if (table.columnDescriptor(indexColumn) == null) {
- throw new ColumnNotFoundException(indexColumn);
- }
- }
+ validateColumnsExistsInTable(table, params.columns());
if (params.unique() &&
!params.columns().containsAll(table.colocationColumns())) {
throw new IgniteException(Index.INVALID_INDEX_DEFINITION_ERR,
"Unique index must include all colocation columns");
}
}
+
+ private static void ensureColumnCanBeDropped(
+ CatalogSchemaDescriptor schema,
+ CatalogTableDescriptor table,
+ AlterTableDropColumnParams params
+ ) {
+ validateColumnsExistsInTable(table, params.columns());
+
+ List<String> inPrimaryKeyColumns = params.columns().stream()
+ .filter(table::isPrimaryKeyColumn)
+ .collect(toList());
+
+ if (!inPrimaryKeyColumns.isEmpty()) {
+ throw new IgniteException(Table.TABLE_DEFINITION_ERR, "Can't drop
primary key columns: " + inPrimaryKeyColumns);
+ }
+
+ Arrays.stream(schema.indexes())
+ .filter(index -> index.tableId() == table.id())
+ .forEach(index -> params.columns().stream()
+ .filter(index::hasColumn)
+ .findAny()
+ .ifPresent(columnName -> {
+ throw new SqlException(
Review Comment:
Special validation exception?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -947,14 +836,45 @@ private static void
ensureNoTableOrIndexExistsWithSameName(CatalogSchemaDescript
}
private static void validateIndexColumns(CatalogTableDescriptor table,
AbstractCreateIndexCommandParams params) {
- for (String indexColumn : params.columns()) {
- if (table.columnDescriptor(indexColumn) == null) {
- throw new ColumnNotFoundException(indexColumn);
- }
- }
+ validateColumnsExistsInTable(table, params.columns());
if (params.unique() &&
!params.columns().containsAll(table.colocationColumns())) {
throw new IgniteException(Index.INVALID_INDEX_DEFINITION_ERR,
"Unique index must include all colocation columns");
}
}
+
+ private static void ensureColumnCanBeDropped(
+ CatalogSchemaDescriptor schema,
+ CatalogTableDescriptor table,
+ AlterTableDropColumnParams params
+ ) {
+ validateColumnsExistsInTable(table, params.columns());
+
+ List<String> inPrimaryKeyColumns = params.columns().stream()
+ .filter(table::isPrimaryKeyColumn)
+ .collect(toList());
+
+ if (!inPrimaryKeyColumns.isEmpty()) {
+ throw new IgniteException(Table.TABLE_DEFINITION_ERR, "Can't drop
primary key columns: " + inPrimaryKeyColumns);
Review Comment:
Should not special validation exception be thrown here instead of a generic
`IgniteException`?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -947,14 +836,45 @@ private static void
ensureNoTableOrIndexExistsWithSameName(CatalogSchemaDescript
}
private static void validateIndexColumns(CatalogTableDescriptor table,
AbstractCreateIndexCommandParams params) {
- for (String indexColumn : params.columns()) {
- if (table.columnDescriptor(indexColumn) == null) {
- throw new ColumnNotFoundException(indexColumn);
- }
- }
+ validateColumnsExistsInTable(table, params.columns());
if (params.unique() &&
!params.columns().containsAll(table.colocationColumns())) {
throw new IgniteException(Index.INVALID_INDEX_DEFINITION_ERR,
"Unique index must include all colocation columns");
}
}
+
+ private static void ensureColumnCanBeDropped(
+ CatalogSchemaDescriptor schema,
+ CatalogTableDescriptor table,
+ AlterTableDropColumnParams params
+ ) {
+ validateColumnsExistsInTable(table, params.columns());
+
+ List<String> inPrimaryKeyColumns = params.columns().stream()
+ .filter(table::isPrimaryKeyColumn)
+ .collect(toList());
+
+ if (!inPrimaryKeyColumns.isEmpty()) {
+ throw new IgniteException(Table.TABLE_DEFINITION_ERR, "Can't drop
primary key columns: " + inPrimaryKeyColumns);
+ }
+
+ Arrays.stream(schema.indexes())
+ .filter(index -> index.tableId() == table.id())
+ .forEach(index -> params.columns().stream()
+ .filter(index::hasColumn)
+ .findAny()
+ .ifPresent(columnName -> {
+ throw new SqlException(
+ STMT_VALIDATION_ERR,
+ format("Can't drop indexed column:
[columnName={}, indexName={}]", columnName, index.name()));
+ }));
+ }
+
+ private static void validateColumnsExistsInTable(CatalogTableDescriptor
table, Collection<String> columns) {
Review Comment:
```suggestion
private static void validateColumnsExistInTable(CatalogTableDescriptor
table, Collection<String> columns) {
```
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -1938,19 +1622,81 @@ void testDropNotExistingIndex() {
assertThat(manager.dropIndex(DropIndexParams.builder().indexName(INDEX_NAME).build()),
willThrowFast(IndexNotFoundException.class));
}
- private void createSomeTable(String tableName) {
- CreateTableParams params = CreateTableParams.builder()
- .schemaName(SCHEMA_NAME)
- .tableName(tableName)
- .zone(ZONE_NAME)
- .columns(List.of(
-
ColumnParams.builder().name("key1").type(ColumnType.INT32).build(),
-
ColumnParams.builder().name("val1").type(ColumnType.INT32).build()
- ))
- .primaryKeyColumns(List.of("key1"))
- .build();
+ @Test
+ void testDropNotExistingTable() {
+ assertThat(manager.dropTable(dropTableParams(TABLE_NAME)),
willThrowFast(TableNotFoundException.class));
+ }
+
+ @Test
+ void testCreateTableWithAlreadyExistingName() {
+ assertThat(manager.createTable(simpleTable(TABLE_NAME)),
willCompleteSuccessfully());
- assertThat(manager.createTable(params), willCompleteSuccessfully());
+ assertThat(manager.createTable(simpleTable(TABLE_NAME)),
willThrowFast(TableAlreadyExistsException.class));
+ }
+
+ @Test
+ void testCreateTableWithSameNameAsExistingIndex() {
+ assertThat(manager.createTable(simpleTable(TABLE_NAME)),
willCompleteSuccessfully());
+ assertThat(manager.createIndex(simpleIndex()),
willCompleteSuccessfully());
+
+ assertThat(manager.createTable(simpleTable(INDEX_NAME)),
willThrowFast(IndexAlreadyExistsException.class));
+ }
+
+ @Test
+ void testDropColumnWithNotExistingTable() {
+ assertThat(manager.dropColumn(dropColumnParams("key")),
willThrowFast(TableNotFoundException.class));
+ }
+
+ @Test
+ void testDropColumnWithMissingTableColumns() {
+ assertThat(manager.createTable(simpleTable(TABLE_NAME)),
willCompleteSuccessfully());
+
+ assertThat(manager.dropColumn(dropColumnParams("fake")),
willThrowFast(ColumnNotFoundException.class));
+ }
+
+ @Test
+ void testDropColumnWithPrimaryKeyColumns() {
+ assertThat(manager.createTable(simpleTable(TABLE_NAME)),
willCompleteSuccessfully());
+
+ assertThat(
+ manager.dropColumn(dropColumnParams("ID")),
+ willThrowFast(IgniteException.class, "Can't drop primary key
columns: [ID]")
+ );
+ }
+
+ @Test
+ void testDropColumnWithIndexColumns() {
+ assertThat(manager.createTable(simpleTable(TABLE_NAME)),
willCompleteSuccessfully());
+ assertThat(manager.createIndex(simpleIndex()),
willCompleteSuccessfully());
+
+ assertThat(
+ manager.dropColumn(dropColumnParams("VAL")),
+ willThrowFast(IgniteException.class, String.format("Can't drop
indexed column: [columnName=VAL, indexName=%s]", INDEX_NAME))
+ );
+ }
+
+ @Test
+ void testAddColumnWithNotExistingTable() {
+ assertThat(manager.addColumn(addColumnParams(columnParams("key",
INT32))), willThrowFast(TableNotFoundException.class));
+ }
+
+ @Test
+ void testAddColumnWithExistsColumn() {
Review Comment:
```suggestion
void testAddColumnWithExistingName() {
```
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerSelfTest.java:
##########
@@ -1631,123 +1446,6 @@ void testGetTableIdOnDropIndexEvent() {
assertEquals(tableId, eventParameters.tableId());
}
- @Test
- void testCreateTableErrors() {
Review Comment:
Was this moved elsewhere?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogParamsValidationUtils.java:
##########
@@ -206,25 +295,68 @@ private static void
validateCommonCreateIndexParams(AbstractCreateIndexCommandPa
validateNameField(params.tableName(),
Index.INVALID_INDEX_DEFINITION_ERR, "Missing table name");
- if (CollectionUtils.nullOrEmpty(params.columns())) {
- throw new
CatalogValidationException(Index.INVALID_INDEX_DEFINITION_ERR, "Columns not
specified");
- }
-
- params.columns().stream()
- .filter(Predicate.not(new HashSet<>()::add))
- .findAny()
- .ifPresent(columnName -> {
- throw new CatalogValidationException(
- Index.INVALID_INDEX_DEFINITION_ERR,
- "Duplicate columns are present: {}",
- params.columns()
- );
- });
+ validateColumns(
+ params.columns(),
+ Index.INVALID_INDEX_DEFINITION_ERR,
+ "Columns not specified",
+ "Duplicate columns are present: {}"
+ );
}
private static void validateNameField(String name, int errorCode, String
errorMessage) {
if (StringUtils.nullOrBlank(name)) {
throw new CatalogValidationException(errorCode, errorMessage);
}
}
+
+ private static void validateCollectionIsNotEmpty(Collection<?> collection,
int errorCode, String errorMessage) {
+ if (CollectionUtils.nullOrEmpty(collection)) {
+ throw new CatalogValidationException(errorCode, errorMessage);
+ }
+ }
+
+ private static void validateColumns(
+ List<String> columns,
+ int errorCode,
+ String emptyColumnsErrorMessage,
+ String duplicateColumnsErrorMessageFormat
+ ) {
+ validateCollectionIsNotEmpty(columns, errorCode,
emptyColumnsErrorMessage);
+
+ List<String> duplicates = columns.stream()
+ .filter(Predicate.not(new HashSet<>()::add))
+ .collect(toList());
+
+ if (!duplicates.isEmpty()) {
+ throw new CatalogValidationException(errorCode,
duplicateColumnsErrorMessageFormat, duplicates);
+ }
+ }
+
+ private static void validateColumnsContainsInAnotherColumns(
+ List<String> columns,
Review Comment:
I suggest to rename `columns` to `columnsToCheck` to make it easier to see
what is being validated and what is the context of the validation
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableParams.java:
##########
@@ -113,8 +99,9 @@ public Builder primaryKeyColumns(List<String> pkCols) {
*
* @param colocationCols Colocation column names.
* @return {@code this}.
+ * @throws NullPointerException If the colocation column names is
{@code null} or one of its elements.
*/
- public Builder colocationColumns(@Nullable List<String>
colocationCols) {
+ public Builder colocationColumns(List<String> colocationCols) {
params.colocationCols = colocationCols;
Review Comment:
Let's add a defensive copy here as well
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogParamsValidationUtils.java:
##########
@@ -206,25 +295,68 @@ private static void
validateCommonCreateIndexParams(AbstractCreateIndexCommandPa
validateNameField(params.tableName(),
Index.INVALID_INDEX_DEFINITION_ERR, "Missing table name");
- if (CollectionUtils.nullOrEmpty(params.columns())) {
- throw new
CatalogValidationException(Index.INVALID_INDEX_DEFINITION_ERR, "Columns not
specified");
- }
-
- params.columns().stream()
- .filter(Predicate.not(new HashSet<>()::add))
- .findAny()
- .ifPresent(columnName -> {
- throw new CatalogValidationException(
- Index.INVALID_INDEX_DEFINITION_ERR,
- "Duplicate columns are present: {}",
- params.columns()
- );
- });
+ validateColumns(
+ params.columns(),
+ Index.INVALID_INDEX_DEFINITION_ERR,
+ "Columns not specified",
+ "Duplicate columns are present: {}"
+ );
}
private static void validateNameField(String name, int errorCode, String
errorMessage) {
if (StringUtils.nullOrBlank(name)) {
throw new CatalogValidationException(errorCode, errorMessage);
}
}
+
+ private static void validateCollectionIsNotEmpty(Collection<?> collection,
int errorCode, String errorMessage) {
+ if (CollectionUtils.nullOrEmpty(collection)) {
+ throw new CatalogValidationException(errorCode, errorMessage);
+ }
+ }
+
+ private static void validateColumns(
+ List<String> columns,
+ int errorCode,
+ String emptyColumnsErrorMessage,
+ String duplicateColumnsErrorMessageFormat
+ ) {
+ validateCollectionIsNotEmpty(columns, errorCode,
emptyColumnsErrorMessage);
+
+ List<String> duplicates = columns.stream()
+ .filter(Predicate.not(new HashSet<>()::add))
+ .collect(toList());
+
+ if (!duplicates.isEmpty()) {
+ throw new CatalogValidationException(errorCode,
duplicateColumnsErrorMessageFormat, duplicates);
+ }
+ }
+
+ private static void validateColumnsContainsInAnotherColumns(
+ List<String> columns,
+ List<String> anotherColumns,
+ int errorCode,
+ String errorMessageFormat
+ ) {
+ List<String> notContains = columns.stream()
Review Comment:
`notContained`
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogParamsValidationUtils.java:
##########
@@ -206,25 +295,68 @@ private static void
validateCommonCreateIndexParams(AbstractCreateIndexCommandPa
validateNameField(params.tableName(),
Index.INVALID_INDEX_DEFINITION_ERR, "Missing table name");
- if (CollectionUtils.nullOrEmpty(params.columns())) {
- throw new
CatalogValidationException(Index.INVALID_INDEX_DEFINITION_ERR, "Columns not
specified");
- }
-
- params.columns().stream()
- .filter(Predicate.not(new HashSet<>()::add))
- .findAny()
- .ifPresent(columnName -> {
- throw new CatalogValidationException(
- Index.INVALID_INDEX_DEFINITION_ERR,
- "Duplicate columns are present: {}",
- params.columns()
- );
- });
+ validateColumns(
+ params.columns(),
+ Index.INVALID_INDEX_DEFINITION_ERR,
+ "Columns not specified",
+ "Duplicate columns are present: {}"
+ );
}
private static void validateNameField(String name, int errorCode, String
errorMessage) {
if (StringUtils.nullOrBlank(name)) {
throw new CatalogValidationException(errorCode, errorMessage);
}
}
+
+ private static void validateCollectionIsNotEmpty(Collection<?> collection,
int errorCode, String errorMessage) {
+ if (CollectionUtils.nullOrEmpty(collection)) {
+ throw new CatalogValidationException(errorCode, errorMessage);
+ }
+ }
+
+ private static void validateColumns(
+ List<String> columns,
+ int errorCode,
+ String emptyColumnsErrorMessage,
+ String duplicateColumnsErrorMessageFormat
+ ) {
+ validateCollectionIsNotEmpty(columns, errorCode,
emptyColumnsErrorMessage);
+
+ List<String> duplicates = columns.stream()
+ .filter(Predicate.not(new HashSet<>()::add))
+ .collect(toList());
+
+ if (!duplicates.isEmpty()) {
+ throw new CatalogValidationException(errorCode,
duplicateColumnsErrorMessageFormat, duplicates);
+ }
+ }
+
+ private static void validateColumnsContainsInAnotherColumns(
Review Comment:
```suggestion
private static void validateColumnsAreContainedInAnotherColumns(
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/CreateTableParams.java:
##########
@@ -20,67 +20,51 @@
import java.util.List;
import org.jetbrains.annotations.Nullable;
-/**
- * CREATE TABLE statement.
- */
+/** CREATE TABLE statement. */
public class CreateTableParams extends AbstractTableCommandParams {
/** Creates parameters builder. */
public static Builder builder() {
return new Builder();
}
+ private CreateTableParams() {
+ // No-op.
+ }
+
/** Primary key columns. */
- @Nullable
private List<String> pkCols;
/** Colocation columns. */
- @Nullable
private List<String> colocationCols;
- /** Columns. */
- private List<ColumnParams> cols;
+ /** Columns, {@code null} if not set. */
+ private @Nullable List<ColumnParams> cols;
Review Comment:
When can it be null? Is it possible to create a table without any columns?
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerValidationTest.java:
##########
@@ -685,4 +854,17 @@ private static AlterZoneParams alterZoneParams(@Nullable
Integer autoAdjust, @Nu
.dataNodesAutoAdjustScaleDown(scaleDown)
.build();
}
+
+ private CreateTableParams simpleTableParamsWithoutPrimaryKeys(@Nullable
List<String> primaryKeys) {
Review Comment:
```suggestion
private CreateTableParams simpleTableParamsWithPrimaryKeys(@Nullable
List<String> primaryKeys) {
```
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogManagerValidationTest.java:
##########
@@ -685,4 +854,17 @@ private static AlterZoneParams alterZoneParams(@Nullable
Integer autoAdjust, @Nu
.dataNodesAutoAdjustScaleDown(scaleDown)
.build();
}
+
+ private CreateTableParams simpleTableParamsWithoutPrimaryKeys(@Nullable
List<String> primaryKeys) {
+ return createTableParams(TABLE_NAME, List.of(columnParams("key",
INT32), columnParams("val", INT64)), primaryKeys, null);
+ }
+
+ private CreateTableParams
simpleTableParamsWithoutColocationColumns(@Nullable List<String>
colocationColumns) {
Review Comment:
```suggestion
private CreateTableParams
simpleTableParamsWithColocationColumns(@Nullable List<String>
colocationColumns) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
