[GitHub] [ignite-3] xtern commented on a diff in pull request #2119: IGNITE-19592 Implement ADD/DROP COLUMN DDL commands with using Catalog

["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