From 6dd52005478c5cf03870f79abe82851f40e95787 Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Mon, 4 Mar 2024 14:55:46 +0900 Subject: [PATCH 1/2] Support FIRST and AFTER clause when adding a new column in engine --- .../antlr4/io/trino/grammar/sql/SqlBase.g4 | 3 +- .../io/trino/execution/AddColumnTask.java | 25 ++++- .../main/java/io/trino/metadata/Metadata.java | 3 +- .../io/trino/metadata/MetadataManager.java | 5 +- .../tracing/TracingConnectorMetadata.java | 10 ++ .../io/trino/tracing/TracingMetadata.java | 5 +- .../execution/BaseDataDefinitionTaskTest.java | 23 ++++- .../io/trino/execution/TestAddColumnTask.java | 95 ++++++++++++++++--- .../trino/metadata/AbstractMockMetadata.java | 3 +- .../main/java/io/trino/sql/SqlFormatter.java | 9 ++ .../java/io/trino/sql/parser/AstBuilder.java | 19 ++++ .../java/io/trino/sql/tree/AddColumn.java | 23 ++++- .../io/trino/sql/tree/ColumnPosition.java | 24 +++++ .../java/io/trino/sql/TestSqlFormatter.java | 43 +++++++++ .../io/trino/sql/parser/TestSqlParser.java | 47 ++++++++- .../sql/parser/TestStatementBuilder.java | 3 + .../trino/spi/connector/ColumnPosition.java | 24 +++++ .../spi/connector/ConnectorMetadata.java | 15 ++- docs/src/main/sphinx/sql/alter-table.md | 13 +++ .../ClassLoaderSafeConnectorMetadata.java | 9 ++ .../io/trino/plugin/kudu/KuduMetadata.java | 15 ++- .../trino/plugin/mongodb/MongoMetadata.java | 9 +- 22 files changed, 384 insertions(+), 41 deletions(-) create mode 100644 core/trino-parser/src/main/java/io/trino/sql/tree/ColumnPosition.java create mode 100644 core/trino-spi/src/main/java/io/trino/spi/connector/ColumnPosition.java diff --git a/core/trino-grammar/src/main/antlr4/io/trino/grammar/sql/SqlBase.g4 b/core/trino-grammar/src/main/antlr4/io/trino/grammar/sql/SqlBase.g4 index 3bc36d93c3ad..d716ee538376 100644 --- a/core/trino-grammar/src/main/antlr4/io/trino/grammar/sql/SqlBase.g4 +++ b/core/trino-grammar/src/main/antlr4/io/trino/grammar/sql/SqlBase.g4 @@ -80,7 +80,8 @@ statement | ALTER TABLE (IF EXISTS)? from=qualifiedName RENAME TO to=qualifiedName #renameTable | ALTER TABLE (IF EXISTS)? tableName=qualifiedName - ADD COLUMN (IF NOT EXISTS)? column=columnDefinition #addColumn + ADD COLUMN (IF NOT EXISTS)? column=columnDefinition + (FIRST | LAST | AFTER after=identifier)? #addColumn | ALTER TABLE (IF EXISTS)? tableName=qualifiedName RENAME COLUMN (IF EXISTS)? from=qualifiedName TO to=identifier #renameColumn | ALTER TABLE (IF EXISTS)? tableName=qualifiedName diff --git a/core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java b/core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java index 4cf78c373d33..77e18a6a9500 100644 --- a/core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java +++ b/core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java @@ -34,6 +34,7 @@ import io.trino.sql.PlannerContext; import io.trino.sql.tree.AddColumn; import io.trino.sql.tree.ColumnDefinition; +import io.trino.sql.tree.ColumnPosition; import io.trino.sql.tree.Expression; import io.trino.sql.tree.Identifier; @@ -110,6 +111,7 @@ public ListenableFuture execute( ColumnDefinition element = statement.getColumn(); Identifier columnName = element.getName().getOriginalParts().get(0); + ColumnPosition position = statement.getPosition().orElse(new ColumnPosition.Last()); Type type; try { type = plannerContext.getTypeManager().getType(toTypeSignature(element.getType())); @@ -133,6 +135,9 @@ public ListenableFuture execute( if (!element.isNullable() && !plannerContext.getMetadata().getConnectorCapabilities(session, catalogHandle).contains(NOT_NULL_COLUMN_CONSTRAINT)) { throw semanticException(NOT_SUPPORTED, element, "Catalog '%s' does not support NOT NULL for column '%s'", catalogHandle, columnName); } + if (position instanceof ColumnPosition.After after && !columns.containsKey(after.column().getValue().toLowerCase(ENGLISH))) { + throw semanticException(COLUMN_NOT_FOUND, statement, "Column '%s' does not", after.column().getValue()); + } Map columnProperties = columnPropertyManager.getProperties( catalogHandle.getCatalogName().toString(), @@ -152,7 +157,12 @@ public ListenableFuture execute( .setProperties(columnProperties) .build(); - plannerContext.getMetadata().addColumn(session, tableHandle, qualifiedTableName.asCatalogSchemaTableName(), column); + plannerContext.getMetadata().addColumn( + session, + tableHandle, + qualifiedTableName.asCatalogSchemaTableName(), + column, + toConnectorColumnPosition(position)); } else { accessControl.checkCanAlterColumn(session.toSecurityContext(), qualifiedTableName); @@ -160,6 +170,10 @@ public ListenableFuture execute( if (!columns.containsKey(columnName.getValue().toLowerCase(ENGLISH))) { throw semanticException(COLUMN_NOT_FOUND, statement, "Column '%s' does not exist", columnName); } + if (!(position instanceof ColumnPosition.Last)) { + // TODO https://github.com/trinodb/trino/issues/24513 Support FIRST and AFTER options + throw semanticException(NOT_SUPPORTED, statement, "Specifying column position is not supported for nested columns"); + } List parentPath = statement.getColumn().getName().getOriginalParts().subList(0, statement.getColumn().getName().getOriginalParts().size() - 1).stream() .map(identifier -> identifier.getValue().toLowerCase(ENGLISH)) @@ -234,4 +248,13 @@ private Type getSupportedType(Session session, CatalogHandle catalogHandle, Map< .getSupportedType(session, catalogHandle, tableProperties, type) .orElse(type); } + + private static io.trino.spi.connector.ColumnPosition toConnectorColumnPosition(ColumnPosition columnPosition) + { + return switch (columnPosition) { + case ColumnPosition.First _ -> new io.trino.spi.connector.ColumnPosition.First(); + case ColumnPosition.After after -> new io.trino.spi.connector.ColumnPosition.After(after.column().getValue().toLowerCase(ENGLISH)); + case ColumnPosition.Last _ -> new io.trino.spi.connector.ColumnPosition.Last(); + }; + } } diff --git a/core/trino-main/src/main/java/io/trino/metadata/Metadata.java b/core/trino-main/src/main/java/io/trino/metadata/Metadata.java index 2c71eb193ab4..0401cd7482ec 100644 --- a/core/trino-main/src/main/java/io/trino/metadata/Metadata.java +++ b/core/trino-main/src/main/java/io/trino/metadata/Metadata.java @@ -27,6 +27,7 @@ import io.trino.spi.connector.CatalogSchemaTableName; import io.trino.spi.connector.ColumnHandle; import io.trino.spi.connector.ColumnMetadata; +import io.trino.spi.connector.ColumnPosition; import io.trino.spi.connector.ConnectorCapabilities; import io.trino.spi.connector.ConnectorOutputMetadata; import io.trino.spi.connector.ConnectorTableMetadata; @@ -276,7 +277,7 @@ Optional getTableHandleForExecute( /** * Add the specified column to the table. */ - void addColumn(Session session, TableHandle tableHandle, CatalogSchemaTableName table, ColumnMetadata column); + void addColumn(Session session, TableHandle tableHandle, CatalogSchemaTableName table, ColumnMetadata column, ColumnPosition position); /** * Add the specified field to the column. diff --git a/core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java b/core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java index f437ae0bd5f9..4a9a6f94c906 100644 --- a/core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java +++ b/core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java @@ -44,6 +44,7 @@ import io.trino.spi.connector.CatalogSchemaTableName; import io.trino.spi.connector.ColumnHandle; import io.trino.spi.connector.ColumnMetadata; +import io.trino.spi.connector.ColumnPosition; import io.trino.spi.connector.ConnectorAnalyzeMetadata; import io.trino.spi.connector.ConnectorCapabilities; import io.trino.spi.connector.ConnectorInsertTableHandle; @@ -956,12 +957,12 @@ public void renameField(Session session, TableHandle tableHandle, List f } @Override - public void addColumn(Session session, TableHandle tableHandle, CatalogSchemaTableName table, ColumnMetadata column) + public void addColumn(Session session, TableHandle tableHandle, CatalogSchemaTableName table, ColumnMetadata column, ColumnPosition position) { CatalogHandle catalogHandle = tableHandle.catalogHandle(); CatalogMetadata catalogMetadata = getCatalogMetadataForWrite(session, catalogHandle.getCatalogName().toString()); ConnectorMetadata metadata = getMetadataForWrite(session, catalogHandle); - metadata.addColumn(session.toConnectorSession(catalogHandle), tableHandle.connectorHandle(), column); + metadata.addColumn(session.toConnectorSession(catalogHandle), tableHandle.connectorHandle(), column, position); if (catalogMetadata.getSecurityManagement() == SYSTEM) { systemSecurityMetadata.columnCreated(session, table, column.getName()); } diff --git a/core/trino-main/src/main/java/io/trino/tracing/TracingConnectorMetadata.java b/core/trino-main/src/main/java/io/trino/tracing/TracingConnectorMetadata.java index c4e2ecc31266..0d21c9446ed9 100644 --- a/core/trino-main/src/main/java/io/trino/tracing/TracingConnectorMetadata.java +++ b/core/trino-main/src/main/java/io/trino/tracing/TracingConnectorMetadata.java @@ -23,6 +23,7 @@ import io.trino.spi.connector.CatalogSchemaTableName; import io.trino.spi.connector.ColumnHandle; import io.trino.spi.connector.ColumnMetadata; +import io.trino.spi.connector.ColumnPosition; import io.trino.spi.connector.ConnectorAccessControl; import io.trino.spi.connector.ConnectorAnalyzeMetadata; import io.trino.spi.connector.ConnectorInsertTableHandle; @@ -478,6 +479,15 @@ public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle } } + @Override + public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column, ColumnPosition position) + { + Span span = startSpan("addColumn", tableHandle); + try (var ignored = scopedSpan(span)) { + delegate.addColumn(session, tableHandle, column, position); + } + } + @Override public void addField(ConnectorSession session, ConnectorTableHandle tableHandle, List parentPath, String fieldName, Type type, boolean ignoreExisting) { diff --git a/core/trino-main/src/main/java/io/trino/tracing/TracingMetadata.java b/core/trino-main/src/main/java/io/trino/tracing/TracingMetadata.java index 04a3f5b443c6..81a6c4c0866a 100644 --- a/core/trino-main/src/main/java/io/trino/tracing/TracingMetadata.java +++ b/core/trino-main/src/main/java/io/trino/tracing/TracingMetadata.java @@ -56,6 +56,7 @@ import io.trino.spi.connector.CatalogSchemaTableName; import io.trino.spi.connector.ColumnHandle; import io.trino.spi.connector.ColumnMetadata; +import io.trino.spi.connector.ColumnPosition; import io.trino.spi.connector.ConnectorCapabilities; import io.trino.spi.connector.ConnectorOutputMetadata; import io.trino.spi.connector.ConnectorTableMetadata; @@ -485,11 +486,11 @@ public void renameField(Session session, TableHandle tableHandle, List f } @Override - public void addColumn(Session session, TableHandle tableHandle, CatalogSchemaTableName table, ColumnMetadata column) + public void addColumn(Session session, TableHandle tableHandle, CatalogSchemaTableName table, ColumnMetadata column, ColumnPosition position) { Span span = startSpan("addColumn", table); try (var _ = scopedSpan(span)) { - delegate.addColumn(session, tableHandle, table, column); + delegate.addColumn(session, tableHandle, table, column, position); } } diff --git a/core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java b/core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java index 6e3f7e5e38f5..912325d6775c 100644 --- a/core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java +++ b/core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java @@ -43,6 +43,7 @@ import io.trino.spi.connector.CatalogSchemaTableName; import io.trino.spi.connector.ColumnHandle; import io.trino.spi.connector.ColumnMetadata; +import io.trino.spi.connector.ColumnPosition; import io.trino.spi.connector.ConnectorTableMetadata; import io.trino.spi.connector.SaveMode; import io.trino.spi.connector.SchemaTableName; @@ -374,14 +375,30 @@ public Optional getSupportedType(Session session, CatalogHandle catalogHan } @Override - public void addColumn(Session session, TableHandle tableHandle, CatalogSchemaTableName table, ColumnMetadata column) + public void addColumn(Session session, TableHandle tableHandle, CatalogSchemaTableName table, ColumnMetadata column, ColumnPosition position) { SchemaTableName tableName = table.getSchemaTableName(); ConnectorTableMetadata metadata = tables.get(tableName); ImmutableList.Builder columns = ImmutableList.builderWithExpectedSize(metadata.getColumns().size() + 1); - columns.addAll(metadata.getColumns()); - columns.add(column); + switch (position) { + case ColumnPosition.First _ -> { + columns.add(column); + columns.addAll(metadata.getColumns()); + } + case ColumnPosition.After after -> { + for (ColumnMetadata existingColumn : metadata.getColumns()) { + columns.add(existingColumn); + if (existingColumn.getName().equals(after.columnName())) { + columns.add(column); + } + } + } + case ColumnPosition.Last _ -> { + columns.addAll(metadata.getColumns()); + columns.add(column); + } + } tables.put(tableName, new ConnectorTableMetadata(tableName, columns.build())); } diff --git a/core/trino-main/src/test/java/io/trino/execution/TestAddColumnTask.java b/core/trino-main/src/test/java/io/trino/execution/TestAddColumnTask.java index 0d1a001ef658..8ad2e10e8257 100644 --- a/core/trino-main/src/test/java/io/trino/execution/TestAddColumnTask.java +++ b/core/trino-main/src/test/java/io/trino/execution/TestAddColumnTask.java @@ -28,6 +28,7 @@ import io.trino.spi.type.TypeOperators; import io.trino.sql.tree.AddColumn; import io.trino.sql.tree.ColumnDefinition; +import io.trino.sql.tree.ColumnPosition; import io.trino.sql.tree.Identifier; import io.trino.sql.tree.LongLiteral; import io.trino.sql.tree.NodeLocation; @@ -50,6 +51,7 @@ import static io.trino.spi.type.BigintType.BIGINT; import static io.trino.spi.type.IntegerType.INTEGER; import static io.trino.spi.type.RowType.rowType; +import static io.trino.spi.type.VarcharType.VARCHAR; import static io.trino.sql.analyzer.TypeSignatureTranslator.toSqlType; import static io.trino.testing.TestingHandles.TEST_CATALOG_NAME; import static io.trino.testing.assertions.TrinoExceptionAssert.assertTrinoExceptionThrownBy; @@ -67,11 +69,43 @@ public void testAddColumn() assertThat(metadata.getTableMetadata(testSession, table).columns()) .containsExactly(new ColumnMetadata("test", BIGINT)); - getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("new_col"), INTEGER, Optional.empty(), false, false)); + getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("new_col"), INTEGER, Optional.empty(), new io.trino.sql.tree.ColumnPosition.Last(), false, false)); assertThat(metadata.getTableMetadata(testSession, table).columns()) .containsExactly(new ColumnMetadata("test", BIGINT), new ColumnMetadata("new_col", INTEGER)); } + @Test + public void testAddColumnFirst() + { + QualifiedObjectName tableName = qualifiedObjectName("existing_table"); + metadata.createTable(testSession, TEST_CATALOG_NAME, someTable(tableName), FAIL); + TableHandle table = metadata.getTableHandle(testSession, tableName).get(); + assertThat(metadata.getTableMetadata(testSession, table).columns()) + .containsExactly(new ColumnMetadata("test", BIGINT)); + + getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("first_col"), INTEGER, Optional.empty(), new ColumnPosition.First(), false, false)); + assertThat(metadata.getTableMetadata(testSession, table).columns()) + .containsExactly(new ColumnMetadata("first_col", INTEGER), new ColumnMetadata("test", BIGINT)); + } + + @Test + public void testAddColumnAfter() + { + QualifiedObjectName tableName = qualifiedObjectName("existing_table"); + metadata.createTable(testSession, TEST_CATALOG_NAME, someTable(tableName), FAIL); + TableHandle table = metadata.getTableHandle(testSession, tableName).get(); + assertThat(metadata.getTableMetadata(testSession, table).columns()) + .containsExactly(new ColumnMetadata("test", BIGINT)); + + getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("last"), INTEGER, Optional.empty(), new ColumnPosition.Last(), false, false)); + assertThat(metadata.getTableMetadata(testSession, table).columns()) + .containsExactly(new ColumnMetadata("test", BIGINT), new ColumnMetadata("last", INTEGER)); + + getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("second"), VARCHAR, Optional.empty(), new ColumnPosition.After(new Identifier("test")), false, false)); + assertThat(metadata.getTableMetadata(testSession, table).columns()) + .containsExactly(new ColumnMetadata("test", BIGINT), new ColumnMetadata("second", VARCHAR), new ColumnMetadata("last", INTEGER)); + } + @Test public void testAddColumnWithComment() { @@ -79,7 +113,7 @@ public void testAddColumnWithComment() metadata.createTable(testSession, TEST_CATALOG_NAME, someTable(tableName), FAIL); TableHandle table = metadata.getTableHandle(testSession, tableName).get(); - getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("new_col"), INTEGER, Optional.of("test comment"), false, false)); + getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("new_col"), INTEGER, Optional.of("test comment"), new ColumnPosition.Last(), false, false)); assertThat(metadata.getTableMetadata(testSession, table).columns()) .containsExactly( new ColumnMetadata("test", BIGINT), @@ -110,7 +144,7 @@ public void testAddColumnNotExistingTable() { QualifiedObjectName tableName = qualifiedObjectName("not_existing_table"); - assertTrinoExceptionThrownBy(() -> getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("test"), INTEGER, Optional.empty(), false, false))) + assertTrinoExceptionThrownBy(() -> getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("test"), INTEGER, Optional.empty(), new ColumnPosition.Last(), false, false))) .hasErrorCode(TABLE_NOT_FOUND) .hasMessageContaining("Table '%s' does not exist", tableName); } @@ -120,7 +154,7 @@ public void testAddColumnNotExistingTableIfExists() { QualifiedName tableName = qualifiedName("not_existing_table"); - getFutureValue(executeAddColumn(tableName, QualifiedName.of("test"), INTEGER, Optional.empty(), true, false)); + getFutureValue(executeAddColumn(tableName, QualifiedName.of("test"), INTEGER, Optional.empty(), new ColumnPosition.Last(), true, false)); // no exception } @@ -133,7 +167,7 @@ public void testAddColumnNotExists() assertThat(metadata.getTableMetadata(testSession, table).columns()) .containsExactly(new ColumnMetadata("test", BIGINT)); - getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("test"), INTEGER, Optional.empty(), false, true)); + getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("test"), INTEGER, Optional.empty(), new ColumnPosition.Last(), false, true)); assertThat(metadata.getTableMetadata(testSession, table).columns()) .containsExactly(new ColumnMetadata("test", BIGINT)); } @@ -144,7 +178,7 @@ public void testAddColumnAlreadyExist() QualifiedObjectName tableName = qualifiedObjectName("existing_table"); metadata.createTable(testSession, TEST_CATALOG_NAME, someTable(tableName), FAIL); - assertTrinoExceptionThrownBy(() -> getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("test"), INTEGER, Optional.empty(), false, false))) + assertTrinoExceptionThrownBy(() -> getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("test"), INTEGER, Optional.empty(), new ColumnPosition.Last(), false, false))) .hasErrorCode(COLUMN_ALREADY_EXISTS) .hasMessageContaining("Column 'test' already exists"); } @@ -155,7 +189,7 @@ public void testAddColumnOnView() QualifiedObjectName viewName = qualifiedObjectName("existing_view"); metadata.createView(testSession, viewName, someView(), ImmutableMap.of(), false); - assertTrinoExceptionThrownBy(() -> getFutureValue(executeAddColumn(asQualifiedName(viewName), QualifiedName.of("test"), INTEGER, Optional.empty(), false, false))) + assertTrinoExceptionThrownBy(() -> getFutureValue(executeAddColumn(asQualifiedName(viewName), QualifiedName.of("test"), INTEGER, Optional.empty(), new ColumnPosition.Last(), false, false))) .hasErrorCode(TABLE_NOT_FOUND) .hasMessageContaining("Table '%s' does not exist", viewName); } @@ -166,7 +200,7 @@ public void testAddColumnOnMaterializedView() QualifiedObjectName materializedViewName = qualifiedObjectName("existing_materialized_view"); metadata.createMaterializedView(testSession, QualifiedObjectName.valueOf(materializedViewName.toString()), someMaterializedView(), MATERIALIZED_VIEW_PROPERTIES, false, false); - assertTrinoExceptionThrownBy(() -> getFutureValue(executeAddColumn(asQualifiedName(materializedViewName), QualifiedName.of("test"), INTEGER, Optional.empty(), false, false))) + assertTrinoExceptionThrownBy(() -> getFutureValue(executeAddColumn(asQualifiedName(materializedViewName), QualifiedName.of("test"), INTEGER, Optional.empty(), new ColumnPosition.Last(), false, false))) .hasErrorCode(TABLE_NOT_FOUND) .hasMessageContaining("Table '%s' does not exist", materializedViewName); } @@ -200,6 +234,39 @@ public void testAddFieldToNotExistingField() .hasMessageContaining("Field 'x' does not exist within row(a row(b integer))"); } + @Test + public void testAddFieldToWithUnsupportedPosition() + { + QualifiedObjectName tableName = qualifiedObjectName("existing_table"); + metadata.createTable( + testSession, + TEST_CATALOG_NAME, + rowTable(tableName, new RowType.Field(Optional.of("a"), rowType(new RowType.Field(Optional.of("b"), INTEGER)))), + FAIL); + + assertTrinoExceptionThrownBy(() -> getFutureValue(executeAddColumn( + asQualifiedName(tableName), + QualifiedName.of("col", "x", "c"), + INTEGER, + Optional.empty(), + new ColumnPosition.First(), + false, + false))) + .hasErrorCode(NOT_SUPPORTED) + .hasMessageContaining("Specifying column position is not supported for nested columns"); + + assertTrinoExceptionThrownBy(() -> getFutureValue(executeAddColumn( + asQualifiedName(tableName), + QualifiedName.of("col", "x", "c"), + INTEGER, + Optional.empty(), + new ColumnPosition.After(new Identifier("a")), + false, + false))) + .hasErrorCode(NOT_SUPPORTED) + .hasMessageContaining("Specifying column position is not supported for nested columns"); + } + @Test public void testUnsupportedMapTypeInRowField() { @@ -265,25 +332,25 @@ public void testUnsupportedAddAmbiguousField() private ListenableFuture executeAddColumn(QualifiedName table, QualifiedName column, Type type, boolean tableExists, boolean columnNotExists) { - return executeAddColumn(table, column, type, Optional.empty(), tableExists, columnNotExists); + return executeAddColumn(table, column, type, Optional.empty(), new ColumnPosition.Last(), tableExists, columnNotExists); } - private ListenableFuture executeAddColumn(QualifiedName table, QualifiedName column, Type type, Optional comment, boolean tableExists, boolean columnNotExists) + private ListenableFuture executeAddColumn(QualifiedName table, QualifiedName column, Type type, Optional comment, ColumnPosition position, boolean tableExists, boolean columnNotExists) { ColumnDefinition columnDefinition = new ColumnDefinition(column, toSqlType(type), true, ImmutableList.of(), comment); - return executeAddColumn(table, columnDefinition, tableExists, columnNotExists); + return executeAddColumn(table, columnDefinition, position, tableExists, columnNotExists); } private ListenableFuture executeAddColumn(QualifiedName table, QualifiedName column, Type type, List properties, boolean tableExists, boolean columnNotExists) { ColumnDefinition columnDefinition = new ColumnDefinition(column, toSqlType(type), true, properties, Optional.empty()); - return executeAddColumn(table, columnDefinition, tableExists, columnNotExists); + return executeAddColumn(table, columnDefinition, new ColumnPosition.Last(), tableExists, columnNotExists); } - private ListenableFuture executeAddColumn(QualifiedName table, ColumnDefinition columnDefinition, boolean tableExists, boolean columnNotExists) + private ListenableFuture executeAddColumn(QualifiedName table, ColumnDefinition columnDefinition, ColumnPosition position, boolean tableExists, boolean columnNotExists) { return new AddColumnTask(plannerContext, new AllowAllAccessControl(), columnPropertyManager) - .execute(new AddColumn(new NodeLocation(1, 1), table, columnDefinition, tableExists, columnNotExists), queryStateMachine, ImmutableList.of(), WarningCollector.NOOP); + .execute(new AddColumn(new NodeLocation(1, 1), table, columnDefinition, Optional.of(position), tableExists, columnNotExists), queryStateMachine, ImmutableList.of(), WarningCollector.NOOP); } private static ConnectorTableMetadata rowTable(QualifiedObjectName tableName, RowType.Field... fields) diff --git a/core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java b/core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java index 7edfd163ab0a..283e73dd6da8 100644 --- a/core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java +++ b/core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java @@ -32,6 +32,7 @@ import io.trino.spi.connector.CatalogSchemaTableName; import io.trino.spi.connector.ColumnHandle; import io.trino.spi.connector.ColumnMetadata; +import io.trino.spi.connector.ColumnPosition; import io.trino.spi.connector.ConnectorCapabilities; import io.trino.spi.connector.ConnectorOutputMetadata; import io.trino.spi.connector.ConnectorTableMetadata; @@ -339,7 +340,7 @@ public void renameField(Session session, TableHandle tableHandle, List f } @Override - public void addColumn(Session session, TableHandle tableHandle, CatalogSchemaTableName table, ColumnMetadata column) + public void addColumn(Session session, TableHandle tableHandle, CatalogSchemaTableName table, ColumnMetadata column, ColumnPosition position) { throw new UnsupportedOperationException(); } diff --git a/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java b/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java index 90f29216f630..63c198cf3ab1 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java +++ b/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java @@ -26,6 +26,7 @@ import io.trino.sql.tree.CaseStatement; import io.trino.sql.tree.CaseStatementWhenClause; import io.trino.sql.tree.ColumnDefinition; +import io.trino.sql.tree.ColumnPosition; import io.trino.sql.tree.Comment; import io.trino.sql.tree.CommentCharacteristic; import io.trino.sql.tree.Commit; @@ -1826,6 +1827,14 @@ protected Void visitAddColumn(AddColumn node, Integer indent) } builder.append(formatColumnDefinition(node.getColumn())); + node.getPosition().ifPresent(position -> { + switch (position) { + case ColumnPosition.First _ -> builder.append(" FIRST"); + case ColumnPosition.After after -> builder.append(" AFTER ").append(formatName(after.column())); + case ColumnPosition.Last _ -> builder.append(" LAST"); + } + }); + return null; } diff --git a/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java b/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java index d5afc46d87fe..e025a9e050b8 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java +++ b/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java @@ -42,6 +42,7 @@ import io.trino.sql.tree.Cast; import io.trino.sql.tree.CoalesceExpression; import io.trino.sql.tree.ColumnDefinition; +import io.trino.sql.tree.ColumnPosition; import io.trino.sql.tree.Comment; import io.trino.sql.tree.CommentCharacteristic; import io.trino.sql.tree.Commit; @@ -822,10 +823,28 @@ public Node visitAddColumn(SqlBaseParser.AddColumnContext context) return new AddColumn(getLocation(context), getQualifiedName(context.qualifiedName()), (ColumnDefinition) visit(context.columnDefinition()), + toColumnPosition(context), context.EXISTS().stream().anyMatch(node -> node.getSymbol().getTokenIndex() < context.COLUMN().getSymbol().getTokenIndex()), context.EXISTS().stream().anyMatch(node -> node.getSymbol().getTokenIndex() > context.COLUMN().getSymbol().getTokenIndex())); } + private Optional toColumnPosition(SqlBaseParser.AddColumnContext context) + { + if (context.FIRST() != null) { + return Optional.of(new ColumnPosition.First()); + } + + if (context.AFTER() != null) { + return Optional.of(new ColumnPosition.After(getIdentifierIfPresent(context.after).orElseThrow(() -> new IllegalArgumentException("AFTER requires an identifier")))); + } + + if (context.LAST() != null) { + return Optional.of(new ColumnPosition.Last()); + } + + return Optional.empty(); + } + @Override public Node visitSetColumnType(SqlBaseParser.SetColumnTypeContext context) { diff --git a/core/trino-parser/src/main/java/io/trino/sql/tree/AddColumn.java b/core/trino-parser/src/main/java/io/trino/sql/tree/AddColumn.java index 8de253a58832..5332cb42e9e5 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/tree/AddColumn.java +++ b/core/trino-parser/src/main/java/io/trino/sql/tree/AddColumn.java @@ -17,6 +17,7 @@ import java.util.List; import java.util.Objects; +import java.util.Optional; import static com.google.common.base.MoreObjects.toStringHelper; import static java.util.Objects.requireNonNull; @@ -26,14 +27,16 @@ public class AddColumn { private final QualifiedName name; private final ColumnDefinition column; + private final Optional position; private final boolean tableExists; private final boolean columnNotExists; - public AddColumn(NodeLocation location, QualifiedName name, ColumnDefinition column, boolean tableExists, boolean columnNotExists) + public AddColumn(NodeLocation location, QualifiedName name, ColumnDefinition column, Optional position, boolean tableExists, boolean columnNotExists) { super(location); this.name = requireNonNull(name, "name is null"); this.column = requireNonNull(column, "column is null"); + this.position = requireNonNull(position, "position is null"); this.tableExists = tableExists; this.columnNotExists = columnNotExists; } @@ -48,6 +51,11 @@ public ColumnDefinition getColumn() return column; } + public Optional getPosition() + { + return position; + } + public boolean isTableExists() { return tableExists; @@ -67,13 +75,18 @@ public R accept(AstVisitor visitor, C context) @Override public List getChildren() { - return ImmutableList.of(column); + ImmutableList.Builder nodes = ImmutableList.builder(); + nodes.add(column); + if (position.isPresent() && position.get() instanceof ColumnPosition.After after) { + nodes.add(after.column()); + } + return nodes.build(); } @Override public int hashCode() { - return Objects.hash(name, column); + return Objects.hash(name, column, position); } @Override @@ -87,7 +100,8 @@ public boolean equals(Object obj) } AddColumn o = (AddColumn) obj; return Objects.equals(name, o.name) && - Objects.equals(column, o.column); + Objects.equals(column, o.column) && + Objects.equals(position, o.position); } @Override @@ -96,6 +110,7 @@ public String toString() return toStringHelper(this) .add("name", name) .add("column", column) + .add("position", position) .toString(); } } diff --git a/core/trino-parser/src/main/java/io/trino/sql/tree/ColumnPosition.java b/core/trino-parser/src/main/java/io/trino/sql/tree/ColumnPosition.java new file mode 100644 index 000000000000..11169a605b4d --- /dev/null +++ b/core/trino-parser/src/main/java/io/trino/sql/tree/ColumnPosition.java @@ -0,0 +1,24 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.sql.tree; + +public sealed interface ColumnPosition + permits ColumnPosition.First, ColumnPosition.After, ColumnPosition.Last +{ + record First() implements ColumnPosition {} + + record After(Identifier column) implements ColumnPosition {} + + record Last() implements ColumnPosition {} +} diff --git a/core/trino-parser/src/test/java/io/trino/sql/TestSqlFormatter.java b/core/trino-parser/src/test/java/io/trino/sql/TestSqlFormatter.java index 2d17bff06ada..85183a37337e 100644 --- a/core/trino-parser/src/test/java/io/trino/sql/TestSqlFormatter.java +++ b/core/trino-parser/src/test/java/io/trino/sql/TestSqlFormatter.java @@ -17,6 +17,7 @@ import io.trino.sql.tree.AddColumn; import io.trino.sql.tree.AllColumns; import io.trino.sql.tree.ColumnDefinition; +import io.trino.sql.tree.ColumnPosition; import io.trino.sql.tree.Comment; import io.trino.sql.tree.CreateCatalog; import io.trino.sql.tree.CreateMaterializedView; @@ -43,6 +44,7 @@ import java.util.Optional; import java.util.function.BiFunction; +import static io.trino.sql.QueryUtil.identifier; import static io.trino.sql.QueryUtil.selectList; import static io.trino.sql.QueryUtil.simpleQuery; import static io.trino.sql.QueryUtil.table; @@ -457,6 +459,7 @@ public void testAddColumn() true, emptyList(), Optional.empty()), + Optional.empty(), false, false))) .isEqualTo("ALTER TABLE foo.t ADD COLUMN c VARCHAR"); assertThat(formatSql( @@ -468,8 +471,48 @@ public void testAddColumn() true, emptyList(), Optional.of("攻殻機動隊")), + Optional.empty(), false, false))) .isEqualTo("ALTER TABLE foo.t ADD COLUMN c VARCHAR COMMENT '攻殻機動隊'"); + assertThat(formatSql( + new AddColumn( + new NodeLocation(1, 1), + QualifiedName.of("foo", "t"), + new ColumnDefinition(QualifiedName.of("c"), + new GenericDataType(new NodeLocation(1, 1), new Identifier("VARCHAR", false), ImmutableList.of()), + true, + emptyList(), + Optional.empty()), + Optional.of(new ColumnPosition.First()), + false, + false))) + .isEqualTo("ALTER TABLE foo.t ADD COLUMN c VARCHAR FIRST"); + assertThat(formatSql( + new AddColumn( + new NodeLocation(1, 1), + QualifiedName.of("foo", "t"), + new ColumnDefinition(QualifiedName.of("c"), + new GenericDataType(new NodeLocation(1, 1), new Identifier("VARCHAR", false), ImmutableList.of()), + true, + emptyList(), + Optional.empty()), + Optional.of(new ColumnPosition.Last()), + false, + false))) + .isEqualTo("ALTER TABLE foo.t ADD COLUMN c VARCHAR LAST"); + assertThat(formatSql( + new AddColumn( + new NodeLocation(1, 1), + QualifiedName.of("foo", "t"), + new ColumnDefinition(QualifiedName.of("c"), + new GenericDataType(new NodeLocation(1, 1), new Identifier("VARCHAR", false), ImmutableList.of()), + true, + emptyList(), + Optional.empty()), + Optional.of(new ColumnPosition.After(identifier("b"))), + false, + false))) + .isEqualTo("ALTER TABLE foo.t ADD COLUMN c VARCHAR AFTER b"); } @Test diff --git a/core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java b/core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java index dcc7e3f4b927..6b7f8a0c3c02 100644 --- a/core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java +++ b/core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java @@ -34,6 +34,7 @@ import io.trino.sql.tree.Cast; import io.trino.sql.tree.CoalesceExpression; import io.trino.sql.tree.ColumnDefinition; +import io.trino.sql.tree.ColumnPosition; import io.trino.sql.tree.Comment; import io.trino.sql.tree.Commit; import io.trino.sql.tree.ComparisonExpression; @@ -3443,35 +3444,65 @@ public void testAddColumn() .isEqualTo(new AddColumn( new NodeLocation(1, 1), QualifiedName.of("foo", "t"), - new ColumnDefinition(QualifiedName.of("c"), simpleType(location(1, 31), "bigint"), true, emptyList(), Optional.empty()), false, false)); + new ColumnDefinition(QualifiedName.of("c"), simpleType(location(1, 31), "bigint"), true, emptyList(), Optional.empty()), Optional.empty(), false, false)); assertThat(statement("ALTER TABLE foo.t ADD COLUMN d double NOT NULL")) .ignoringLocation() .isEqualTo(new AddColumn( location(1, 1), QualifiedName.of("foo", "t"), - new ColumnDefinition(QualifiedName.of("d"), simpleType(location(1, 31), "double"), false, emptyList(), Optional.empty()), false, false)); + new ColumnDefinition(QualifiedName.of("d"), simpleType(location(1, 31), "double"), false, emptyList(), Optional.empty()), Optional.empty(), false, false)); assertThat(statement("ALTER TABLE IF EXISTS foo.t ADD COLUMN d double NOT NULL")) .ignoringLocation() .isEqualTo(new AddColumn( location(1, 1), QualifiedName.of("foo", "t"), - new ColumnDefinition(QualifiedName.of("d"), simpleType(location(1, 31), "double"), false, emptyList(), Optional.empty()), true, false)); + new ColumnDefinition(QualifiedName.of("d"), simpleType(location(1, 31), "double"), false, emptyList(), Optional.empty()), Optional.empty(), true, false)); assertThat(statement("ALTER TABLE foo.t ADD COLUMN IF NOT EXISTS d double NOT NULL")) .ignoringLocation() .isEqualTo(new AddColumn( location(1, 1), QualifiedName.of("foo", "t"), - new ColumnDefinition(QualifiedName.of("d"), simpleType(location(1, 31), "double"), false, emptyList(), Optional.empty()), false, true)); + new ColumnDefinition(QualifiedName.of("d"), simpleType(location(1, 31), "double"), false, emptyList(), Optional.empty()), Optional.empty(), false, true)); assertThat(statement("ALTER TABLE IF EXISTS foo.t ADD COLUMN IF NOT EXISTS d double NOT NULL")) .ignoringLocation() .isEqualTo(new AddColumn( location(1, 1), QualifiedName.of("foo", "t"), - new ColumnDefinition(QualifiedName.of("d"), simpleType(location(1, 31), "double"), false, emptyList(), Optional.empty()), true, true)); + new ColumnDefinition(QualifiedName.of("d"), simpleType(location(1, 31), "double"), false, emptyList(), Optional.empty()), Optional.empty(), true, true)); + + assertThat(statement("ALTER TABLE foo.t ADD COLUMN c bigint FIRST")) + .ignoringLocation() + .isEqualTo(new AddColumn( + location(1, 1), + QualifiedName.of("foo", "t"), + new ColumnDefinition(QualifiedName.of("c"), simpleType(location(1, 31), "bigint"), true, emptyList(), Optional.empty()), + Optional.of(new ColumnPosition.First()), + false, + false)); + + assertThat(statement("ALTER TABLE foo.t ADD COLUMN c bigint LAST")) + .ignoringLocation() + .isEqualTo(new AddColumn( + location(1, 1), + QualifiedName.of("foo", "t"), + new ColumnDefinition(QualifiedName.of("c"), simpleType(location(1, 31), "bigint"), true, emptyList(), Optional.empty()), + Optional.of(new ColumnPosition.Last()), + false, + false)); + + assertThat(statement("ALTER TABLE foo.t ADD COLUMN c bigint AFTER b")) + .ignoringLocation() + .isEqualTo(new AddColumn( + location(1, 1), + QualifiedName.of("foo", "t"), + new ColumnDefinition(QualifiedName.of("c"), simpleType(location(1, 31), "bigint"), true, emptyList(), Optional.empty()), + Optional.of(new ColumnPosition.After(identifier("b"))), + false, + false)); // Add a field assertThat(statement("ALTER TABLE foo.t ADD COLUMN c.d double")) @@ -3485,6 +3516,7 @@ public void testAddColumn() true, ImmutableList.of(), Optional.empty()), + Optional.empty(), false, false)); @@ -3499,6 +3531,7 @@ public void testAddColumn() true, ImmutableList.of(), Optional.empty()), + Optional.empty(), false, true)); @@ -3513,6 +3546,7 @@ public void testAddColumn() true, ImmutableList.of(), Optional.empty()), + Optional.empty(), true, false)); @@ -3527,6 +3561,7 @@ public void testAddColumn() true, ImmutableList.of(), Optional.empty()), + Optional.empty(), true, false)); @@ -3541,6 +3576,7 @@ public void testAddColumn() true, ImmutableList.of(), Optional.empty()), + Optional.empty(), true, false)); @@ -3555,6 +3591,7 @@ public void testAddColumn() true, ImmutableList.of(), Optional.empty()), + Optional.empty(), true, true)); } diff --git a/core/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java b/core/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java index 9d61a461c7e9..6ea00d956b7a 100644 --- a/core/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java +++ b/core/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java @@ -207,6 +207,9 @@ public void testStatementBuilder() printStatement("alter table a.b.c set properties a=true, b=123, c='x'"); printStatement("alter table a.b.c set properties a=DEFAULT, b=123"); + printStatement("alter table a.b.c add column x bigint first"); + printStatement("alter table a.b.c add column x bigint after y"); + printStatement("alter table a.b.c add column x bigint last"); printStatement("alter table a.b.c add column x bigint"); printStatement("alter table a.b.c add column x bigint comment 'large x'"); diff --git a/core/trino-spi/src/main/java/io/trino/spi/connector/ColumnPosition.java b/core/trino-spi/src/main/java/io/trino/spi/connector/ColumnPosition.java new file mode 100644 index 000000000000..9dea4e7b4d64 --- /dev/null +++ b/core/trino-spi/src/main/java/io/trino/spi/connector/ColumnPosition.java @@ -0,0 +1,24 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.spi.connector; + +public sealed interface ColumnPosition + permits ColumnPosition.First, ColumnPosition.After, ColumnPosition.Last +{ + record First() implements ColumnPosition {} + + record After(String columnName) implements ColumnPosition {} + + record Last() implements ColumnPosition {} +} diff --git a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java index 4696d8c03ea4..bfcc719a262a 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java +++ b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java @@ -580,13 +580,26 @@ default void setColumnComment(ConnectorSession session, ConnectorTableHandle tab } /** - * Add the specified column + * @deprecated Use {@link #addColumn(ConnectorSession, ConnectorTableHandle, ColumnMetadata, ColumnPosition)} */ + @Deprecated default void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column) { throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns"); } + /** + * Add the specified column + */ + default void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column, ColumnPosition position) + { + switch (position) { + case ColumnPosition.First _ -> throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns with FIRST clause"); + case ColumnPosition.After _ -> throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns with AFTER clause"); + case ColumnPosition.Last _ -> addColumn(session, tableHandle, column); + } + } + /** * Add the specified field, potentially nested, to a row. * diff --git a/docs/src/main/sphinx/sql/alter-table.md b/docs/src/main/sphinx/sql/alter-table.md index f1bbedd71222..a6a2dd68182a 100644 --- a/docs/src/main/sphinx/sql/alter-table.md +++ b/docs/src/main/sphinx/sql/alter-table.md @@ -7,6 +7,7 @@ ALTER TABLE [ IF EXISTS ] name RENAME TO new_name ALTER TABLE [ IF EXISTS ] name ADD COLUMN [ IF NOT EXISTS ] column_name data_type [ NOT NULL ] [ COMMENT comment ] [ WITH ( property_name = expression [, ...] ) ] + [ FIRST | LAST | AFTER after_column_name ] ALTER TABLE [ IF EXISTS ] name DROP COLUMN [ IF EXISTS ] column_name ALTER TABLE [ IF EXISTS ] name RENAME COLUMN [ IF EXISTS ] old_name TO new_name ALTER TABLE [ IF EXISTS ] name ALTER COLUMN column_name SET DATA TYPE new_type @@ -96,6 +97,18 @@ not already exists: ALTER TABLE IF EXISTS users ADD COLUMN IF NOT EXISTS zip varchar; ``` +Add column `id` as the first column to the `users` table: + +``` +ALTER TABLE users ADD COLUMN id varchar FIRST; +``` + +Add column `zip` after column `country` to the `users` table: + +``` +ALTER TABLE users ADD COLUMN zip varchar AFTER country; +``` + Drop column `zip` from the `users` table: ``` diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java index 52f09777e537..93f50200cbf7 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java @@ -23,6 +23,7 @@ import io.trino.spi.connector.CatalogSchemaTableName; import io.trino.spi.connector.ColumnHandle; import io.trino.spi.connector.ColumnMetadata; +import io.trino.spi.connector.ColumnPosition; import io.trino.spi.connector.ConnectorAccessControl; import io.trino.spi.connector.ConnectorAnalyzeMetadata; import io.trino.spi.connector.ConnectorInsertTableHandle; @@ -372,6 +373,14 @@ public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle } } + @Override + public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column, ColumnPosition position) + { + try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) { + delegate.addColumn(session, tableHandle, column, position); + } + } + @Override public void addField(ConnectorSession session, ConnectorTableHandle tableHandle, List parentPath, String fieldName, Type type, boolean ignoreExisting) { diff --git a/plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduMetadata.java b/plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduMetadata.java index 511a424be7b1..b5734de9138e 100755 --- a/plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduMetadata.java +++ b/plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduMetadata.java @@ -23,6 +23,7 @@ import io.trino.spi.connector.Assignment; import io.trino.spi.connector.ColumnHandle; import io.trino.spi.connector.ColumnMetadata; +import io.trino.spi.connector.ColumnPosition; import io.trino.spi.connector.ConnectorInsertTableHandle; import io.trino.spi.connector.ConnectorMergeTableHandle; import io.trino.spi.connector.ConnectorMetadata; @@ -295,10 +296,16 @@ public void renameTable(ConnectorSession session, ConnectorTableHandle tableHand } @Override - public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column) - { - KuduTableHandle kuduTableHandle = (KuduTableHandle) tableHandle; - clientSession.addColumn(kuduTableHandle.getSchemaTableName(), column); + public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column, ColumnPosition position) + { + switch (position) { + case ColumnPosition.First _ -> throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns with FIRST clause"); + case ColumnPosition.After _ -> throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns with AFTER clause"); + case ColumnPosition.Last _ -> { + KuduTableHandle kuduTableHandle = (KuduTableHandle) tableHandle; + clientSession.addColumn(kuduTableHandle.getSchemaTableName(), column); + } + } } @Override diff --git a/plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoMetadata.java b/plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoMetadata.java index e023e61e7211..e3cf4e18f2c6 100644 --- a/plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoMetadata.java +++ b/plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoMetadata.java @@ -28,6 +28,7 @@ import io.trino.spi.connector.Assignment; import io.trino.spi.connector.ColumnHandle; import io.trino.spi.connector.ColumnMetadata; +import io.trino.spi.connector.ColumnPosition; import io.trino.spi.connector.ConnectorInsertTableHandle; import io.trino.spi.connector.ConnectorMetadata; import io.trino.spi.connector.ConnectorOutputMetadata; @@ -294,9 +295,13 @@ public void renameTable(ConnectorSession session, ConnectorTableHandle tableHand } @Override - public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column) + public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column, ColumnPosition position) { - mongoSession.addColumn(((MongoTableHandle) tableHandle), column); + switch (position) { + case ColumnPosition.First _ -> throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns with FIRST clause"); + case ColumnPosition.After _ -> throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns with AFTER clause"); + case ColumnPosition.Last _ -> mongoSession.addColumn(((MongoTableHandle) tableHandle), column); + } } @Override From 0c5c12cfcf5bc926f224bd9aa1c9143f3736c7f6 Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Fri, 22 Nov 2024 13:45:47 +0900 Subject: [PATCH 2/2] Support FIRST and AFTER clause when adding a new column in Iceberg --- .../plugin/jdbc/BaseJdbcConnectorTest.java | 3 +- .../deltalake/TestDeltaLakeConnectorTest.java | 3 +- .../plugin/hive/BaseHiveConnectorTest.java | 3 +- .../trino/plugin/iceberg/IcebergMetadata.java | 14 +++++--- .../plugin/kudu/TestKuduConnectorTest.java | 3 +- .../memory/TestMemoryConnectorTest.java | 3 +- .../mongodb/TestMongoConnectorTest.java | 3 +- .../io/trino/testing/BaseConnectorTest.java | 35 +++++++++++++++++++ .../testing/TestingConnectorBehavior.java | 1 + 9 files changed, 58 insertions(+), 10 deletions(-) diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java index 59e9c9d75f5e..5db81e8405b9 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java @@ -148,7 +148,8 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) { return switch (connectorBehavior) { case SUPPORTS_UPDATE -> true; - case SUPPORTS_CREATE_MATERIALIZED_VIEW, + case SUPPORTS_ADD_COLUMN_WITH_POSITION, + SUPPORTS_CREATE_MATERIALIZED_VIEW, SUPPORTS_CREATE_VIEW, SUPPORTS_MERGE, SUPPORTS_PREDICATE_EXPRESSION_PUSHDOWN, diff --git a/plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeConnectorTest.java b/plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeConnectorTest.java index 943caec8b254..36173b7e5da3 100644 --- a/plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeConnectorTest.java +++ b/plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeConnectorTest.java @@ -171,7 +171,8 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) return switch (connectorBehavior) { case SUPPORTS_CREATE_OR_REPLACE_TABLE, SUPPORTS_REPORTING_WRITTEN_BYTES -> true; - case SUPPORTS_ADD_FIELD, + case SUPPORTS_ADD_COLUMN_WITH_POSITION, + SUPPORTS_ADD_FIELD, SUPPORTS_AGGREGATION_PUSHDOWN, SUPPORTS_CREATE_MATERIALIZED_VIEW, SUPPORTS_DROP_FIELD, diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java index f182f68d5a36..916099dbfd47 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java @@ -250,7 +250,8 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) return switch (connectorBehavior) { case SUPPORTS_MULTI_STATEMENT_WRITES, SUPPORTS_REPORTING_WRITTEN_BYTES -> true; // FIXME: Fails because only allowed with transactional tables - case SUPPORTS_ADD_FIELD, + case SUPPORTS_ADD_COLUMN_WITH_POSITION, + SUPPORTS_ADD_FIELD, SUPPORTS_CREATE_MATERIALIZED_VIEW, SUPPORTS_DROP_FIELD, SUPPORTS_MERGE, diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java index 2a6af35d5c1d..2f6e95c251b9 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java @@ -67,6 +67,7 @@ import io.trino.spi.connector.CatalogSchemaTableName; import io.trino.spi.connector.ColumnHandle; import io.trino.spi.connector.ColumnMetadata; +import io.trino.spi.connector.ColumnPosition; import io.trino.spi.connector.ConnectorAccessControl; import io.trino.spi.connector.ConnectorAnalyzeMetadata; import io.trino.spi.connector.ConnectorInsertTableHandle; @@ -2341,7 +2342,7 @@ private static Term toIcebergTerm(Schema schema, PartitionField partitionField) } @Override - public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column) + public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column, ColumnPosition position) { // Spark doesn't support adding a NOT NULL column to Iceberg tables // Also, Spark throws an exception when reading the table if we add such columns and execute a rollback procedure @@ -2355,9 +2356,14 @@ public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle // added - instead of relying on addColumn in iceberg library to assign Ids AtomicInteger nextFieldId = new AtomicInteger(icebergTable.schema().highestFieldId() + 2); try { - icebergTable.updateSchema() - .addColumn(column.getName(), toIcebergTypeForNewColumn(column.getType(), nextFieldId), column.getComment()) - .commit(); + UpdateSchema updateSchema = icebergTable.updateSchema(); + updateSchema.addColumn(column.getName(), toIcebergTypeForNewColumn(column.getType(), nextFieldId), column.getComment()); + switch (position) { + case ColumnPosition.First _ -> updateSchema.moveFirst(column.getName()); + case ColumnPosition.After after -> updateSchema.moveAfter(column.getName(), after.columnName()); + case ColumnPosition.Last _ -> {} + } + updateSchema.commit(); } catch (RuntimeException e) { throw new TrinoException(ICEBERG_COMMIT_ERROR, "Failed to add column: " + firstNonNull(e.getMessage(), e), e); diff --git a/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKuduConnectorTest.java b/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKuduConnectorTest.java index 73de900f28ba..a65dd81ffd79 100644 --- a/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKuduConnectorTest.java +++ b/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKuduConnectorTest.java @@ -59,7 +59,8 @@ protected QueryRunner createQueryRunner() protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) { return switch (connectorBehavior) { - case SUPPORTS_ARRAY, + case SUPPORTS_ADD_COLUMN_WITH_POSITION, + SUPPORTS_ARRAY, SUPPORTS_COMMENT_ON_COLUMN, SUPPORTS_COMMENT_ON_TABLE, SUPPORTS_CREATE_MATERIALIZED_VIEW, diff --git a/plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java b/plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java index abc54b106411..7d3293abf17f 100644 --- a/plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java +++ b/plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java @@ -88,7 +88,8 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) { return switch (connectorBehavior) { case SUPPORTS_TRUNCATE -> true; - case SUPPORTS_ADD_FIELD, + case SUPPORTS_ADD_COLUMN_WITH_POSITION, + SUPPORTS_ADD_FIELD, SUPPORTS_AGGREGATION_PUSHDOWN, SUPPORTS_CREATE_MATERIALIZED_VIEW, SUPPORTS_DELETE, diff --git a/plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java b/plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java index fd85c4abb8a6..5a10cd839050 100644 --- a/plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java +++ b/plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java @@ -99,7 +99,8 @@ public final void destroy() protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) { return switch (connectorBehavior) { - case SUPPORTS_ADD_FIELD, + case SUPPORTS_ADD_COLUMN_WITH_POSITION, + SUPPORTS_ADD_FIELD, SUPPORTS_CREATE_MATERIALIZED_VIEW, SUPPORTS_CREATE_VIEW, SUPPORTS_DROP_FIELD, diff --git a/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java b/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java index e53c808fa9bf..cda7da42f4fc 100644 --- a/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java +++ b/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java @@ -105,6 +105,7 @@ import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ADD_COLUMN; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ADD_COLUMN_NOT_NULL_CONSTRAINT; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ADD_COLUMN_WITH_COMMENT; +import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ADD_COLUMN_WITH_POSITION; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ADD_FIELD; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ADD_FIELD_IN_ARRAY; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ARRAY; @@ -2418,6 +2419,40 @@ protected void verifyAddNotNullColumnToNonEmptyTableFailurePermissible(Throwable throw new AssertionError("Unexpected failure when adding not null column", e); } + @Test + public void testAddColumnWithPosition() + { + skipTestUnless(hasBehavior(SUPPORTS_ADD_COLUMN)); // covered by testAddColumn + + if (!hasBehavior(SUPPORTS_ADD_COLUMN_WITH_POSITION)) { + try (TestTable table = new TestTable(getQueryRunner()::execute, "test_add_column_", "AS SELECT 2 second, 4 fourth")) { + assertQueryFails( + "ALTER TABLE " + table.getName() + " ADD COLUMN first integer FIRST", + "This connector does not support adding columns with FIRST clause"); + assertQueryFails( + "ALTER TABLE " + table.getName() + " ADD COLUMN third integer AFTER second", + "This connector does not support adding columns with AFTER clause"); + } + return; + } + + try (TestTable table = new TestTable(getQueryRunner()::execute, "test_add_column_", "AS SELECT 2 second, 4 fourth")) { + assertTableColumnNames(table.getName(), "second", "fourth"); + assertQuery("SELECT * FROM " + table.getName(), "VALUES (2, 4)"); + + assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN first integer FIRST"); + assertTableColumnNames(table.getName(), "first", "second", "fourth"); + assertQuery("SELECT * FROM " + table.getName(), "VALUES (null, 2, 4)"); + + assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN third integer AFTER second"); + assertTableColumnNames(table.getName(), "first", "second", "third", "fourth"); + assertQuery("SELECT * FROM " + table.getName(), "VALUES (null, 2, null, 4)"); + + assertUpdate("INSERT INTO " + table.getName() + " VALUES (10, 20, 30, 40)", 1); + assertQuery("SELECT * FROM " + table.getName(), "VALUES (null, 2, null, 4), (10, 20, 30, 40)"); + } + } + @Test public void testAddRowField() { diff --git a/testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java b/testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java index 1245d75ca2a2..fcab562f9762 100644 --- a/testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java +++ b/testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java @@ -85,6 +85,7 @@ public enum TestingConnectorBehavior SUPPORTS_ADD_COLUMN, SUPPORTS_ADD_COLUMN_WITH_COMMENT(SUPPORTS_ADD_COLUMN), + SUPPORTS_ADD_COLUMN_WITH_POSITION(SUPPORTS_ADD_COLUMN), SUPPORTS_ADD_FIELD(fallback -> fallback.test(SUPPORTS_ADD_COLUMN) && fallback.test(SUPPORTS_ROW_TYPE)), SUPPORTS_ADD_FIELD_IN_ARRAY(SUPPORTS_ADD_FIELD), SUPPORTS_DROP_COLUMN(SUPPORTS_ADD_COLUMN),