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 5abac917b35218..92dfc13e19d23f 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 @@ -611,6 +611,7 @@ default void setColumnComment(ConnectorSession session, ConnectorTableHandle tab /** * @deprecated Use {{@link #addColumn(ConnectorSession, ConnectorTableHandle, ColumnMetadata, ColumnPosition, Optional)}} */ + @Deprecated default void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column) { throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns"); @@ -633,6 +634,7 @@ default void addColumn(ConnectorSession session, ConnectorTableHandle tableHandl /** * @deprecated Use {{@link #addField(ConnectorSession, ConnectorTableHandle, List, String, Type, ColumnPosition, Optional, boolean)}} */ + @Deprecated default void addField(ConnectorSession session, ConnectorTableHandle tableHandle, List parentPath, String fieldName, Type type, boolean ignoreExisting) { throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding fields"); 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 42ef84c3619db4..cbbfd7c1ad9ce1 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 @@ -140,7 +140,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 3d9da314a7a5e2..7034fcc28d66be 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 @@ -150,7 +150,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 fb6cfdaf99c75f..401fc2ee0dffec 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 @@ -251,7 +251,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 1a9ea9571ac892..1249ddda57e3f6 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 @@ -57,6 +57,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.ConnectorInsertTableHandle; import io.trino.spi.connector.ConnectorMaterializedViewDefinition; @@ -1879,7 +1880,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, Optional afterColumnName) { // 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 @@ -1893,9 +1894,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 FIRST -> updateSchema.moveFirst(column.getName()); + case AFTER -> updateSchema.moveAfter(column.getName(), afterColumnName.orElseThrow()); + case LAST -> {} + } + updateSchema.commit(); } catch (RuntimeException e) { throw new TrinoException(ICEBERG_COMMIT_ERROR, "Failed to add column: " + firstNonNull(e.getMessage(), e), e); @@ -1903,7 +1909,7 @@ public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle } @Override - public void addField(ConnectorSession session, ConnectorTableHandle tableHandle, List parentPath, String fieldName, io.trino.spi.type.Type type, boolean ignoreExisting) + public void addField(ConnectorSession session, ConnectorTableHandle tableHandle, List parentPath, String fieldName, io.trino.spi.type.Type type, ColumnPosition position, Optional afterFieldName, boolean ignoreExisting) { // Iceberg disallows ambiguous field names in a table. e.g. (a row(b int), "a.b" int) String parentName = String.join(".", parentPath); @@ -1921,9 +1927,14 @@ public void addField(ConnectorSession session, ConnectorTableHandle tableHandle, } try { - icebergTable.updateSchema() - .addColumn(caseSensitiveParentName, fieldName, toIcebergTypeForNewColumn(type, new AtomicInteger())) // Iceberg library assigns fresh id internally - .commit(); + UpdateSchema updateSchema = icebergTable.updateSchema(); + updateSchema.addColumn(caseSensitiveParentName, fieldName, toIcebergTypeForNewColumn(type, new AtomicInteger())); // Iceberg library assigns fresh id internally; + switch (position) { + case FIRST -> updateSchema.moveFirst(caseSensitiveParentName + "." + fieldName); + case AFTER -> updateSchema.moveAfter(caseSensitiveParentName + "." + fieldName, caseSensitiveParentName + "." + afterFieldName.orElseThrow()); + case LAST -> {} + } + updateSchema.commit(); } catch (RuntimeException e) { throw new TrinoException(ICEBERG_COMMIT_ERROR, "Failed to add field: " + 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 b836e716dfaff6..fbf74b1942ed7f 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 @@ -62,6 +62,7 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) { return switch (connectorBehavior) { case SUPPORTS_ARRAY, + SUPPORTS_ADD_COLUMN_WITH_POSITION, SUPPORTS_COMMENT_ON_COLUMN, SUPPORTS_COMMENT_ON_TABLE, SUPPORTS_CREATE_MATERIALIZED_VIEW, 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 e3d3250b5660bd..a487d0172a96b3 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 @@ -98,7 +98,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/plugin/trino-raptor-legacy/src/test/java/io/trino/plugin/raptor/legacy/BaseRaptorConnectorTest.java b/plugin/trino-raptor-legacy/src/test/java/io/trino/plugin/raptor/legacy/BaseRaptorConnectorTest.java index b49b9024a15023..86593dd24a1fae 100644 --- a/plugin/trino-raptor-legacy/src/test/java/io/trino/plugin/raptor/legacy/BaseRaptorConnectorTest.java +++ b/plugin/trino-raptor-legacy/src/test/java/io/trino/plugin/raptor/legacy/BaseRaptorConnectorTest.java @@ -67,6 +67,7 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) { return switch (connectorBehavior) { case SUPPORTS_ADD_COLUMN_WITH_COMMENT, + SUPPORTS_ADD_COLUMN_WITH_POSITION, SUPPORTS_COMMENT_ON_COLUMN, SUPPORTS_COMMENT_ON_TABLE, SUPPORTS_CREATE_MATERIALIZED_VIEW, 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 8d0dcc2b7f81a9..2c4dea2f5e2ce7 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 @@ -104,7 +104,9 @@ 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_WITH_POSITION; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ARRAY; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_COMMENT_ON_COLUMN; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_COMMENT_ON_MATERIALIZED_VIEW_COLUMN; @@ -2395,6 +2397,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() { @@ -2433,6 +2469,59 @@ public void testAddRowField() } } + @Test + public void testAddRowFieldWithPosition() + { + skipTestUnless(hasBehavior(SUPPORTS_ADD_FIELD)); // covered by testAddRowField + + if (!hasBehavior(SUPPORTS_ADD_FIELD_WITH_POSITION)) { + try (TestTable table = new TestTable( + getQueryRunner()::execute, + "test_add_field_", + "AS SELECT CAST(row(1) AS row(field integer)) AS col")) { + assertQueryFails( + "ALTER TABLE " + table.getName() + " ADD COLUMN col.new_field integer FIRST", + "This connector does not support adding columns with FIRST clause"); + assertQueryFails( + "ALTER TABLE " + table.getName() + " ADD COLUMN col.new_field integer AFTER field", + "This connector does not support adding columns with AFTER clause"); + } + return; + } + + try (TestTable table = new TestTable( + getQueryRunner()::execute, + "test_add_field_", + "AS SELECT CAST(row(2, row(22, 44)) AS row(b integer, d row(f2 integer, f4 integer))) AS col")) { + assertThat(getColumnType(table.getName(), "col")).isEqualTo("row(b integer, d row(f2 integer, f4 integer))"); + + assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN col.a integer FIRST"); + assertThat(getColumnType(table.getName(), "col")).isEqualTo("row(a integer, b integer, d row(f2 integer, f4 integer))"); + assertThat(query("SELECT * FROM " + table.getName())) + .matches("SELECT CAST(row(NULL, 2, row(22, 44)) AS row(a integer, b integer, d row(f2 integer, f4 integer)))"); + + assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN col.c integer AFTER b"); + assertThat(getColumnType(table.getName(), "col")).isEqualTo("row(a integer, b integer, c integer, d row(f2 integer, f4 integer))"); + assertThat(query("SELECT * FROM " + table.getName())) + .matches("SELECT CAST(row(NULL, 2, NULL, row(22, 44)) AS row(a integer, b integer, c integer, d row(f2 integer, f4 integer)))"); + + assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN col.d.f1 integer FIRST"); + assertThat(getColumnType(table.getName(), "col")).isEqualTo("row(a integer, b integer, c integer, d row(f1 integer, f2 integer, f4 integer))"); + assertThat(query("SELECT * FROM " + table.getName())) + .matches("SELECT CAST(row(NULL, 2, NULL, row(NULL, 22, 44)) AS row(a integer, b integer, c integer, d row(f1 integer, f2 integer, f4 integer)))"); + + assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN col.d.f3 integer AFTER f2"); + assertThat(getColumnType(table.getName(), "col")).isEqualTo("row(a integer, b integer, c integer, d row(f1 integer, f2 integer, f3 integer, f4 integer))"); + assertThat(query("SELECT * FROM " + table.getName())) + .matches("SELECT CAST(row(NULL, 2, NULL, row(NULL, 22, NULL, 44)) AS row(a integer, b integer, c integer, d row(f1 integer, f2 integer, f3 integer, f4 integer)))"); + + assertUpdate("INSERT INTO " + table.getName() + " SELECT row(10, 20, 30, row(111, 222, 333, 444))", 1); + assertThat(query("SELECT * FROM " + table.getName())) + .skippingTypesCheck() + .matches("SELECT row(NULL, 2, NULL, row(NULL, 22, NULL, 44)) UNION SELECT row(10, 20, 30, row(111, 222, 333, 444))"); + } + } + @Test public void testDropColumn() { 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 a233c8c6d244c8..1589db7a9ebf04 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 @@ -84,7 +84,9 @@ 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_WITH_POSITION(SUPPORTS_ADD_FIELD), SUPPORTS_DROP_COLUMN(SUPPORTS_ADD_COLUMN), SUPPORTS_DROP_FIELD(and(SUPPORTS_DROP_COLUMN, SUPPORTS_ROW_TYPE)), SUPPORTS_RENAME_COLUMN,