You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "AMashenkov (via GitHub)" <gi...@apache.org> on 2023/05/30 10:21:33 UTC
[GitHub] [ignite-3] AMashenkov opened a new pull request, #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
AMashenkov opened a new pull request, #2119:
URL: https://github.com/apache/ignite-3/pull/2119
https://issues.apache.org/jira/browse/IGNITE-19592
--
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
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1211513395
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/ColumnParams.java:
##########
@@ -86,4 +82,70 @@ public Integer precision() {
public Integer scale() {
return null;
}
+
+ /** Parameters builder. */
+ public static class Builder {
+ private ColumnParams params;
+
+ private Builder() {
+ params = new ColumnParams();
Review Comment:
I understand that other builders follow the same pattern, but hey, let's implement a proper builder pattern
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerWrapper.java:
##########
@@ -70,6 +74,22 @@ public CompletableFuture<Boolean> handle(DdlCommand cmd) {
.thenCompose(res -> catalogManager.dropTable(DdlToCatalogCommandConverter.convert((DropTableCommand) cmd))
.handle(handleModificationResult(((DropTableCommand) cmd).ifTableExists(), TableNotFoundException.class))
);
+ } else if (cmd instanceof AlterTableAddCommand) {
+ AlterTableAddCommand addCommand = (AlterTableAddCommand) cmd;
+
+ return ddlCommandFuture
+ .thenCompose(res -> catalogManager.addColumn(DdlToCatalogCommandConverter.convert(addCommand))
+ .handle(handleModificationResult(addCommand.ifTableExists(), TableNotFoundException.class))
+ .handle(handleModificationResult(addCommand.ifColumnNotExists(), ColumnAlreadyExistsException.class))
Review Comment:
I think we need to ignore addCommand.ifColumnNotExists flag
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -205,13 +215,83 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> addColumn(AlterTableAddColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ List<TableColumnDescriptor> columnDescriptors = new ArrayList<>();
+
+ for (ColumnParams col : params.columns()) {
+ if (table.column(col.name()) != null) {
+ throw new ColumnAlreadyExistsException(col.name());
+ }
+
+ columnDescriptors.add(CatalogUtils.fromParams(col));
+ }
+
+ return List.of(
+ new NewColumnsEntry(table.id(), columnDescriptors)
+ );
+ });
}
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> dropColumn(AlterTableDropColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ for (String columnName : params.columns()) {
+ if (table.column(columnName) == null) {
+ throw new ColumnNotFoundException(columnName);
+ }
+ if (table.isPrimaryKeyColumn(columnName)) {
+ throw new IllegalArgumentException("Can't drop primary key column: column=" + columnName);
Review Comment:
let's throw IgniteInternalException instead of IllegalArgumentException
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1212108259
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerWrapper.java:
##########
@@ -70,6 +73,21 @@ public CompletableFuture<Boolean> handle(DdlCommand cmd) {
.thenCompose(res -> catalogManager.dropTable(DdlToCatalogCommandConverter.convert((DropTableCommand) cmd))
.handle(handleModificationResult(((DropTableCommand) cmd).ifTableExists(), TableNotFoundException.class))
);
+ } else if (cmd instanceof AlterTableAddCommand) {
+ AlterTableAddCommand addCommand = (AlterTableAddCommand) cmd;
+
+ return ddlCommandFuture
+ .thenCompose(res -> catalogManager.addColumn(DdlToCatalogCommandConverter.convert(addCommand))
+ .handle(handleModificationResult(addCommand.ifTableExists(), TableNotFoundException.class))
+ );
+ } else if (cmd instanceof AlterTableDropCommand) {
+ AlterTableDropCommand dropCommand = (AlterTableDropCommand) cmd;
+
+ return ddlCommandFuture
+ .thenCompose(res -> catalogManager.dropColumn(DdlToCatalogCommandConverter.convert(dropCommand))
+ .handle(handleModificationResult(dropCommand.ifTableExists(), TableNotFoundException.class))
+ .handle(handleModificationResult(dropCommand.ifColumnExists(), ColumnNotFoundException.class))
Review Comment:
Should `ifColumnExists` also be ignored for `DROP COLUMN`?
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1211173130
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -205,13 +215,84 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> addColumn(AlterTableAddColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ List<TableColumnDescriptor> columnDescriptors = new ArrayList<>();
+
+ for (ColumnParams col : params.columns()) {
+ if (table.column(col.name()) != null) {
+ throw new ColumnAlreadyExistsException(col.name());
+ }
+
+ columnDescriptors.add(CatalogUtils.fromParams(col));
+ }
+
+ return List.of(
+ new NewColumnsEntry(table.id(), columnDescriptors)
+ );
+ });
}
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> dropColumn(AlterTableDropColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ } else if (params.columns().size() > 1 && params.ifColumnExists()) {
+ return failedFuture(new UnsupportedOperationException("Clause 'IF NOT EXISTS' is not supported when adding multiple columns."));
Review Comment:
current behavior for ADD COLUMN differs from DROP COLUMN, which seems strange to me. From my point of view it's better to keep the same for `ADD COLUMN` :roll_eyes:
--
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
[GitHub] [ignite-3] AMashenkov merged pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov merged PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1210535149
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -205,13 +215,84 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> addColumn(AlterTableAddColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ List<TableColumnDescriptor> columnDescriptors = new ArrayList<>();
+
+ for (ColumnParams col : params.columns()) {
+ if (table.column(col.name()) != null) {
+ throw new ColumnAlreadyExistsException(col.name());
Review Comment:
- From the user's point of view, I would expect that for a table with an existing column "column1" the following command "ADD COLUMN IF NOT EXISTS (column1 int, column2 int)" will silently create 'column2' :thinking: .
But as far as I understand the current behavior, we just ignore the exception. It seems to me that it's better to completely forbid 'IF NOT EXISTS' options for multiple columns if we can't support this (as was in previous PR).
- Why we need `ifColumnNotExists` field in `AlterTableAddColumnParams` if we don't use it?
- Also, current behavior for ADD COLUMN differs from DROP COLUMN, which seems strange to me.
I also don't quite understand - why we can't return an empty list to prevent catalog version change?
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1210560522
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -205,13 +215,84 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> addColumn(AlterTableAddColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ List<TableColumnDescriptor> columnDescriptors = new ArrayList<>();
+
+ for (ColumnParams col : params.columns()) {
+ if (table.column(col.name()) != null) {
+ throw new ColumnAlreadyExistsException(col.name());
+ }
+
+ columnDescriptors.add(CatalogUtils.fromParams(col));
+ }
+
+ return List.of(
+ new NewColumnsEntry(table.id(), columnDescriptors)
+ );
+ });
}
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> dropColumn(AlterTableDropColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ } else if (params.columns().size() > 1 && params.ifColumnExists()) {
+ return failedFuture(new UnsupportedOperationException("Clause 'IF NOT EXISTS' is not supported when adding multiple columns."));
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ Arrays.stream(schema.indexes())
+ .filter(index -> index.tableId() == table.id())
+ .flatMap(index -> index.columns().stream())
+ .filter(col -> params.columns().contains(col))
+ .findAny()
+ .ifPresent(columnName -> {
+ throw new IllegalArgumentException("Can't drop indexed column: column=" + columnName);
+ });
+
+ for (String columnName : params.columns()) {
+ if (table.column(columnName) == null) {
+ throw new ColumnNotFoundException(columnName);
+ }
+ if (table.isPrimaryKeyColumn(columnName)) {
Review Comment:
Since the primary key column is always indexed column - this condition will be hidden by the previous "indexed column" check. I suggest to move it upper.
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1210535149
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -205,13 +215,84 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> addColumn(AlterTableAddColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ List<TableColumnDescriptor> columnDescriptors = new ArrayList<>();
+
+ for (ColumnParams col : params.columns()) {
+ if (table.column(col.name()) != null) {
+ throw new ColumnAlreadyExistsException(col.name());
Review Comment:
- From the user's point of view, I would expect that for a table with an existing column "column1" the following command "ADD COLUMN IF NOT EXISTS (column1 int, column2 int)" will silently create 'column2' :thinking: .
But as far as I understand the current behavior, we just ignore the exception. It seems to me that it's better to completely forbid 'IF NOT EXISTS' options for multiple columns if we can't support this (as was in previous PR).
- Why we need `ifColumnNotExists` field in `AlterTableAddColumnParams` if we don't use it?
- Also, current behavior for ADD COLUMN differs from DROP COLUMN, which seems strange to me.
I also don't quite understand - why we can't return an empty list to prevent catalog version change?
E.g.
```
if (table.column(col.name()) != null) {
if (col.ifColumnNotExists()) {
continue;
}
throw new ColumnAlreadyExistsException(col.name());
}
...
if (columnDescriptors.isEmpty())
return emptyList();
```
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1210535149
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -205,13 +215,84 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> addColumn(AlterTableAddColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ List<TableColumnDescriptor> columnDescriptors = new ArrayList<>();
+
+ for (ColumnParams col : params.columns()) {
+ if (table.column(col.name()) != null) {
+ throw new ColumnAlreadyExistsException(col.name());
Review Comment:
- From the user's point of view, I would expect that for a table with an existing column "column1" the following command "ADD COLUMN IF NOT EXISTS (column1 int, column2 int)" will silently create 'column2'.
But as far as I understand the current behavior, we just ignore the exception. It seems to me that it's better to completely forbid 'IF NOT EXISTS' options for multiple columns if we can't support this (as was in previous PR).
- Why we need `ifColumnNotExists` field in `AlterTableAddColumnParams` if we don't use it?
- Also, current behavior for ADD COLUMN differs from DROP COLUMN, which seems strange to me.
I also don't quite understand - why we can't return an empty list here to prevent catalog version change?
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1210535149
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -205,13 +215,84 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> addColumn(AlterTableAddColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ List<TableColumnDescriptor> columnDescriptors = new ArrayList<>();
+
+ for (ColumnParams col : params.columns()) {
+ if (table.column(col.name()) != null) {
+ throw new ColumnAlreadyExistsException(col.name());
Review Comment:
- From the user's point of view, I would expect that for a table with an existing column "column1" the following command "ADD COLUMN IF NOT EXISTS (column1 int, column2 int)" will silently create 'column2' :thinking: .
But as far as I understand the current behavior, we just ignore the exception. It seems to me that it's better to completely forbid 'IF NOT EXISTS' options for multiple columns if we can't support this (as was in previous PR).
- Why we need `ifColumnNotExists` field in `AlterTableAddColumnParams` if we don't use it?
- Also, current behavior for ADD COLUMN differs from DROP COLUMN, which seems strange to me.
I also don't quite understand - why we can't return an empty list to prevent catalog version change?
E.g.
```
...
if (table.column(col.name()) != null) {
if (col.ifColumnNotExists()) {
continue;
}
throw new ColumnAlreadyExistsException(col.name());
}
...
if (columnDescriptors.isEmpty())
return emptyList();
```
--
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
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1211236617
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/HashIndexDescriptor.java:
##########
@@ -28,31 +26,20 @@
public class HashIndexDescriptor extends IndexDescriptor {
private static final long serialVersionUID = -6784028115063219759L;
- private final List<String> columns;
Review Comment:
changes to this class seems unrelated
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1210535149
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -205,13 +215,84 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> addColumn(AlterTableAddColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ List<TableColumnDescriptor> columnDescriptors = new ArrayList<>();
+
+ for (ColumnParams col : params.columns()) {
+ if (table.column(col.name()) != null) {
+ throw new ColumnAlreadyExistsException(col.name());
Review Comment:
- From the user's point of view, I would expect that for a table with an existing column "column1" the following command "ADD COLUMN IF NOT EXISTS (column1 int, column2 int)" will silently create 'column2' :thinking: .
But as far as I understand the current behavior, we just ignore the exception. It seems to me that it's better to completely forbid 'IF NOT EXISTS' options for multiple columns if we can't support this (as was in previous PR).
- Why we need `ifColumnNotExists` field in `AlterTableAddColumnParams` if we don't use it?
- Also, current behavior for ADD COLUMN differs from DROP COLUMN, which seems strange to me.
I also don't quite understand - why we can't return an empty list to prevent catalog version change?
E.g.
```
...
if (table.column(col.name()) != null) {
if (col.ifColumnNotExists()) {
continue;
}
throw new ColumnAlreadyExistsException(col.name());
}
...
if (columnDescriptors.isEmpty())
return emptyList();
```
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1210560522
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -205,13 +215,84 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> addColumn(AlterTableAddColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ List<TableColumnDescriptor> columnDescriptors = new ArrayList<>();
+
+ for (ColumnParams col : params.columns()) {
+ if (table.column(col.name()) != null) {
+ throw new ColumnAlreadyExistsException(col.name());
+ }
+
+ columnDescriptors.add(CatalogUtils.fromParams(col));
+ }
+
+ return List.of(
+ new NewColumnsEntry(table.id(), columnDescriptors)
+ );
+ });
}
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> dropColumn(AlterTableDropColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ } else if (params.columns().size() > 1 && params.ifColumnExists()) {
+ return failedFuture(new UnsupportedOperationException("Clause 'IF NOT EXISTS' is not supported when adding multiple columns."));
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ Arrays.stream(schema.indexes())
+ .filter(index -> index.tableId() == table.id())
+ .flatMap(index -> index.columns().stream())
+ .filter(col -> params.columns().contains(col))
+ .findAny()
+ .ifPresent(columnName -> {
+ throw new IllegalArgumentException("Can't drop indexed column: column=" + columnName);
+ });
+
+ for (String columnName : params.columns()) {
+ if (table.column(columnName) == null) {
+ throw new ColumnNotFoundException(columnName);
+ }
+ if (table.isPrimaryKeyColumn(columnName)) {
Review Comment:
Since the primary key column is always indexed column - this condition will be hidden by the previous "indexed column" check. I suggest to move this before "indexed column" check.
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1210558019
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -205,13 +215,84 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> addColumn(AlterTableAddColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ List<TableColumnDescriptor> columnDescriptors = new ArrayList<>();
+
+ for (ColumnParams col : params.columns()) {
+ if (table.column(col.name()) != null) {
+ throw new ColumnAlreadyExistsException(col.name());
+ }
+
+ columnDescriptors.add(CatalogUtils.fromParams(col));
+ }
+
+ return List.of(
+ new NewColumnsEntry(table.id(), columnDescriptors)
+ );
+ });
}
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> dropColumn(AlterTableDropColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ } else if (params.columns().size() > 1 && params.ifColumnExists()) {
+ return failedFuture(new UnsupportedOperationException("Clause 'IF NOT EXISTS' is not supported when adding multiple columns."));
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ Arrays.stream(schema.indexes())
+ .filter(index -> index.tableId() == table.id())
+ .flatMap(index -> index.columns().stream())
+ .filter(col -> params.columns().contains(col))
+ .findAny()
+ .ifPresent(columnName -> {
+ throw new IllegalArgumentException("Can't drop indexed column: column=" + columnName);
Review Comment:
It seems to me that in such a situation it would be interesting for a "simple user" :roll_eyes: to know which index uses this column :thinking:
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1210535149
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -205,13 +215,84 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> addColumn(AlterTableAddColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ List<TableColumnDescriptor> columnDescriptors = new ArrayList<>();
+
+ for (ColumnParams col : params.columns()) {
+ if (table.column(col.name()) != null) {
+ throw new ColumnAlreadyExistsException(col.name());
Review Comment:
- From the user's point of view, I would expect that for a table with an existing column "column1" the following command "ADD COLUMN IF NOT EXISTS (column1 int, column2 int)" will silently create 'column2'.
But as far as I understand the current behavior, we just ignore the exception. It seems to me that it's better to completely forbid 'IF NOT EXISTS' options for multiple columns if we can't support this (as was in previous PR).
- Why we need `ifColumnNotExists` field in `AlterTableAddColumnParams` if we don't use it?
- Also, current behavior for ADD COLUMN differs from DROP COLUMN, which seems strange to me.
I also don't quite understand - why we can't return an empty list to prevent catalog version change?
--
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
[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1210560522
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##########
@@ -205,13 +215,84 @@ public CompletableFuture<Void> dropTable(DropTableParams params) {
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> addColumn(AlterTableAddColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ List<TableColumnDescriptor> columnDescriptors = new ArrayList<>();
+
+ for (ColumnParams col : params.columns()) {
+ if (table.column(col.name()) != null) {
+ throw new ColumnAlreadyExistsException(col.name());
+ }
+
+ columnDescriptors.add(CatalogUtils.fromParams(col));
+ }
+
+ return List.of(
+ new NewColumnsEntry(table.id(), columnDescriptors)
+ );
+ });
}
/** {@inheritDoc} */
@Override
public CompletableFuture<Void> dropColumn(AlterTableDropColumnParams params) {
- return failedFuture(new UnsupportedOperationException("Not implemented yet."));
+ if (params.columns().isEmpty()) {
+ return completedFuture(null);
+ } else if (params.columns().size() > 1 && params.ifColumnExists()) {
+ return failedFuture(new UnsupportedOperationException("Clause 'IF NOT EXISTS' is not supported when adding multiple columns."));
+ }
+
+ return saveUpdate(catalog -> {
+ String schemaName = Objects.requireNonNullElse(params.schemaName(), CatalogService.PUBLIC);
+
+ SchemaDescriptor schema = Objects.requireNonNull(catalog.schema(schemaName), "No schema found: " + schemaName);
+
+ TableDescriptor table = schema.table(params.tableName());
+
+ if (table == null) {
+ throw new TableNotFoundException(schemaName, params.tableName());
+ }
+
+ Arrays.stream(schema.indexes())
+ .filter(index -> index.tableId() == table.id())
+ .flatMap(index -> index.columns().stream())
+ .filter(col -> params.columns().contains(col))
+ .findAny()
+ .ifPresent(columnName -> {
+ throw new IllegalArgumentException("Can't drop indexed column: column=" + columnName);
+ });
+
+ for (String columnName : params.columns()) {
+ if (table.column(columnName) == null) {
+ throw new ColumnNotFoundException(columnName);
+ }
+ if (table.isPrimaryKeyColumn(columnName)) {
Review Comment:
Since the primary key column is always indexed column - this condition will be hidden by the previous check. I suggest to move it upper.
--
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
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2119:
URL: https://github.com/apache/ignite-3/pull/2119#discussion_r1211237846
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/IndexDescriptor.java:
##########
@@ -17,6 +17,8 @@
package org.apache.ignite.internal.catalog.descriptors;
+import java.util.List;
Review Comment:
the same: index-related changes should be moved to IGNITE-19460
--
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