From ec3aa36ad17d04c72371de8b88b860bdb2667667 Mon Sep 17 00:00:00 2001 From: Marius Grama Date: Sat, 26 Mar 2022 21:11:21 +0100 Subject: [PATCH] Deny adding column with comment if the connector does not support this feature Instead of silently ignoring the column comment if the connector does not support adding column comments, throw explicitly an exception to expose the fact that this feature is not supported. --- .../io/trino/plugin/jdbc/BaseJdbcClient.java | 4 ++++ .../plugin/jdbc/TestJdbcConnectorTest.java | 1 + .../plugin/clickhouse/ClickHouseClient.java | 3 +++ .../TestClickHouseConnectorTest.java | 7 ++++++ .../iceberg/BaseIcebergConnectorTest.java | 18 -------------- .../kudu/AbstractKuduConnectorTest.java | 1 + .../mongodb/BaseMongoConnectorTest.java | 15 ------------ .../plugin/mysql/BaseMySqlConnectorTest.java | 1 + .../oracle/BaseOracleConnectorTest.java | 3 +++ .../trino/plugin/phoenix/PhoenixMetadata.java | 4 ++++ .../phoenix/TestPhoenixConnectorTest.java | 1 + .../plugin/phoenix5/PhoenixMetadata.java | 4 ++++ .../phoenix5/TestPhoenixConnectorTest.java | 1 + .../TestPostgreSqlConnectorTest.java | 1 + .../plugin/raptor/legacy/RaptorMetadata.java | 4 ++++ .../legacy/BaseRaptorConnectorTest.java | 1 + .../TestSingleStoreConnectorTest.java | 1 + .../sqlserver/BaseSqlServerConnectorTest.java | 3 +++ .../io/trino/testing/BaseConnectorTest.java | 24 +++++++++++++++++++ .../testing/TestingConnectorBehavior.java | 1 + 20 files changed, 65 insertions(+), 33 deletions(-) diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java index 266d2e10f69b..ba806f48455b 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java @@ -694,6 +694,10 @@ protected String buildInsertSql(ConnectorSession session, RemoteTableName target @Override public void addColumn(ConnectorSession session, JdbcTableHandle handle, ColumnMetadata column) { + if (column.getComment() != null) { + throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns with comments"); + } + try (Connection connection = connectionFactory.openConnection(session)) { String columnName = column.getName(); String remoteColumnName = identifierMapping.toRemoteColumnName(connection, columnName); diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectorTest.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectorTest.java index f8aec7e060ea..80a4ea3be695 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectorTest.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectorTest.java @@ -68,6 +68,7 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) case SUPPORTS_COMMENT_ON_TABLE: case SUPPORTS_COMMENT_ON_COLUMN: + case SUPPORTS_ADD_COLUMN_WITH_COMMENT: return false; case SUPPORTS_ARRAY: diff --git a/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java b/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java index 0415fcd918b6..9db003fbcecc 100644 --- a/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java +++ b/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java @@ -401,6 +401,9 @@ protected String renameSchemaSql(String remoteSchemaName, String newRemoteSchema @Override public void addColumn(ConnectorSession session, JdbcTableHandle handle, ColumnMetadata column) { + if (column.getComment() != null) { + throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns with comments"); + } try (Connection connection = connectionFactory.openConnection(session)) { String remoteColumnName = getIdentifierMapping().toRemoteColumnName(connection, column.getName()); String sql = format( diff --git a/plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java b/plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java index 4bd8367e41e5..20ce36f55ab1 100644 --- a/plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java +++ b/plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java @@ -174,6 +174,13 @@ public void testAddColumn() assertFalse(getQueryRunner().tableExists(getSession(), tableName)); } + @Test + @Override + public void testAddColumnWithComment() + { + throw new SkipException("Clickhouse connector does not support specifying a comment when adding a column"); + } + @Test @Override public void testShowCreateTable() diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java index 9281f08970a7..5fa3fe802db3 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java @@ -905,24 +905,6 @@ public void testCreatePartitionedTableAs() dropTable("test_create_partitioned_table_as"); } - @Test - public void testColumnComments() - { - // TODO add support for setting comments on existing column and replace the test with io.trino.testing.BaseConnectorTest#testCommentColumn - - assertUpdate("CREATE TABLE test_column_comments (_bigint BIGINT COMMENT 'test column comment')"); - assertQuery( - "SHOW COLUMNS FROM test_column_comments", - "VALUES ('_bigint', 'bigint', '', 'test column comment')"); - - assertUpdate("ALTER TABLE test_column_comments ADD COLUMN _varchar VARCHAR COMMENT 'test new column comment'"); - assertQuery( - "SHOW COLUMNS FROM test_column_comments", - "VALUES ('_bigint', 'bigint', '', 'test column comment'), ('_varchar', 'varchar', '', 'test new column comment')"); - - dropTable("test_column_comments"); - } - @Test public void testTableComments() { diff --git a/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java b/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java index 8902e49c024f..5da45acb179b 100644 --- a/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java +++ b/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/AbstractKuduConnectorTest.java @@ -284,6 +284,7 @@ public void testAddColumn() } @Test + @Override public void testAddColumnWithComment() { String tableName = "test_add_column_with_comment" + randomTableSuffix(); diff --git a/plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/BaseMongoConnectorTest.java b/plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/BaseMongoConnectorTest.java index 7694250ab30c..6c607e4473bd 100644 --- a/plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/BaseMongoConnectorTest.java +++ b/plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/BaseMongoConnectorTest.java @@ -228,21 +228,6 @@ public void testInsertWithEveryType() assertFalse(getQueryRunner().tableExists(getSession(), "test_insert_types_table")); } - @Test - public void testAddColumnWithComment() - { - // TODO (https://github.com/trinodb/trino/issues/11486) Merge into BaseConnectorTest - assertUpdate("CREATE TABLE test_add_column_with_comment (id integer)"); - - assertUpdate("ALTER TABLE test_add_column_with_comment ADD COLUMN new_column integer COMMENT 'new comment'"); - assertEquals(getColumnComment("test_add_column_with_comment", "new_column"), "new comment"); - - assertUpdate("ALTER TABLE test_add_column_with_comment ADD COLUMN empty_comment integer COMMENT ''"); - assertEquals(getColumnComment("test_add_column_with_comment", "empty_comment"), ""); - - assertUpdate("DROP TABLE test_add_column_with_comment"); - } - @Test public void testJson() { diff --git a/plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java b/plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java index 790d1e25cdb5..a082c1e0f201 100644 --- a/plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java +++ b/plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java @@ -64,6 +64,7 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) return false; case SUPPORTS_COMMENT_ON_COLUMN: + case SUPPORTS_ADD_COLUMN_WITH_COMMENT: return false; case SUPPORTS_ARRAY: diff --git a/plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java b/plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java index eff4a1e02833..24e938c9f1b4 100644 --- a/plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java +++ b/plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java @@ -69,6 +69,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) case SUPPORTS_COMMENT_ON_TABLE: return false; + case SUPPORTS_ADD_COLUMN_WITH_COMMENT: + return false; + case SUPPORTS_ARRAY: case SUPPORTS_ROW_TYPE: return false; diff --git a/plugin/trino-phoenix/src/main/java/io/trino/plugin/phoenix/PhoenixMetadata.java b/plugin/trino-phoenix/src/main/java/io/trino/plugin/phoenix/PhoenixMetadata.java index 0e40b90147d4..d698283ac66e 100644 --- a/plugin/trino-phoenix/src/main/java/io/trino/plugin/phoenix/PhoenixMetadata.java +++ b/plugin/trino-phoenix/src/main/java/io/trino/plugin/phoenix/PhoenixMetadata.java @@ -197,6 +197,10 @@ public Optional finishInsert(ConnectorSession session, @Override public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column) { + if (column.getComment() != null) { + throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns with comments"); + } + JdbcTableHandle handle = (JdbcTableHandle) tableHandle; phoenixClient.execute(session, format( "ALTER TABLE %s ADD %s %s", diff --git a/plugin/trino-phoenix/src/test/java/io/trino/plugin/phoenix/TestPhoenixConnectorTest.java b/plugin/trino-phoenix/src/test/java/io/trino/plugin/phoenix/TestPhoenixConnectorTest.java index 940ef95b218f..9846fc6a559e 100644 --- a/plugin/trino-phoenix/src/test/java/io/trino/plugin/phoenix/TestPhoenixConnectorTest.java +++ b/plugin/trino-phoenix/src/test/java/io/trino/plugin/phoenix/TestPhoenixConnectorTest.java @@ -73,6 +73,7 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) case SUPPORTS_COMMENT_ON_TABLE: case SUPPORTS_COMMENT_ON_COLUMN: + case SUPPORTS_ADD_COLUMN_WITH_COMMENT: return false; case SUPPORTS_RENAME_TABLE: diff --git a/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMetadata.java b/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMetadata.java index cb340f739a40..6910f45217c6 100644 --- a/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMetadata.java +++ b/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMetadata.java @@ -218,6 +218,10 @@ public Optional finishInsert(ConnectorSession session, @Override public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column) { + if (column.getComment() != null) { + throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns with comments"); + } + JdbcTableHandle handle = (JdbcTableHandle) tableHandle; phoenixClient.execute(session, format( "ALTER TABLE %s ADD %s %s", diff --git a/plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java b/plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java index ca0edba29943..366287acc2fe 100644 --- a/plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java +++ b/plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java @@ -91,6 +91,7 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) case SUPPORTS_COMMENT_ON_TABLE: case SUPPORTS_COMMENT_ON_COLUMN: + case SUPPORTS_ADD_COLUMN_WITH_COMMENT: return false; case SUPPORTS_RENAME_TABLE: diff --git a/plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java b/plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java index 4522afdf8c02..5019ea77d8c5 100644 --- a/plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java +++ b/plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java @@ -121,6 +121,7 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) return true; case SUPPORTS_COMMENT_ON_TABLE: + case SUPPORTS_ADD_COLUMN_WITH_COMMENT: return false; case SUPPORTS_ARRAY: diff --git a/plugin/trino-raptor-legacy/src/main/java/io/trino/plugin/raptor/legacy/RaptorMetadata.java b/plugin/trino-raptor-legacy/src/main/java/io/trino/plugin/raptor/legacy/RaptorMetadata.java index ab851357fdb5..27914c8a5905 100644 --- a/plugin/trino-raptor-legacy/src/main/java/io/trino/plugin/raptor/legacy/RaptorMetadata.java +++ b/plugin/trino-raptor-legacy/src/main/java/io/trino/plugin/raptor/legacy/RaptorMetadata.java @@ -473,6 +473,10 @@ public void renameTable(ConnectorSession session, ConnectorTableHandle tableHand @Override public void addColumn(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnMetadata column) { + if (column.getComment() != null) { + throw new TrinoException(NOT_SUPPORTED, "This connector does not support adding columns with comments"); + } + RaptorTableHandle table = (RaptorTableHandle) tableHandle; // Always add new columns to the end. 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 7d90ff56c1f7..4621e39b4d8f 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 @@ -74,6 +74,7 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) case SUPPORTS_RENAME_SCHEMA: case SUPPORTS_COMMENT_ON_TABLE: case SUPPORTS_COMMENT_ON_COLUMN: + case SUPPORTS_ADD_COLUMN_WITH_COMMENT: case SUPPORTS_NOT_NULL_CONSTRAINT: case SUPPORTS_TOPN_PUSHDOWN: return false; diff --git a/plugin/trino-singlestore/src/test/java/io/trino/plugin/singlestore/TestSingleStoreConnectorTest.java b/plugin/trino-singlestore/src/test/java/io/trino/plugin/singlestore/TestSingleStoreConnectorTest.java index 0d5d49df3fa1..c5082ce0bd4f 100644 --- a/plugin/trino-singlestore/src/test/java/io/trino/plugin/singlestore/TestSingleStoreConnectorTest.java +++ b/plugin/trino-singlestore/src/test/java/io/trino/plugin/singlestore/TestSingleStoreConnectorTest.java @@ -81,6 +81,7 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) case SUPPORTS_COMMENT_ON_TABLE: case SUPPORTS_COMMENT_ON_COLUMN: + case SUPPORTS_ADD_COLUMN_WITH_COMMENT: return false; case SUPPORTS_ARRAY: diff --git a/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerConnectorTest.java b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerConnectorTest.java index e0cc07fcfc75..3383d64424b6 100644 --- a/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerConnectorTest.java +++ b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerConnectorTest.java @@ -75,6 +75,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) case SUPPORTS_COMMENT_ON_COLUMN: return false; + case SUPPORTS_ADD_COLUMN_WITH_COMMENT: + return false; + case SUPPORTS_ARRAY: case SUPPORTS_ROW_TYPE: return false; 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 f91e422f146c..4a6e07440dd9 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 @@ -72,6 +72,7 @@ import static io.trino.testing.QueryAssertions.assertContains; import static io.trino.testing.QueryAssertions.getTrinoExceptionCause; import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ADD_COLUMN; +import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ADD_COLUMN_WITH_COMMENT; 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_TABLE; @@ -1727,6 +1728,29 @@ public void testAddColumn() assertFalse(getQueryRunner().tableExists(getSession(), tableName)); } + @Test + public void testAddColumnWithComment() + { + if (!hasBehavior(SUPPORTS_ADD_COLUMN)) { + // Covered by testAddColumn + return; + } + if (!hasBehavior(SUPPORTS_ADD_COLUMN_WITH_COMMENT)) { + assertQueryFails("ALTER TABLE nation ADD COLUMN test_add_col_desc bigint COMMENT 'test column comment'", "This connector does not support adding columns with comments"); + return; + } + + try (TestTable table = new TestTable(getQueryRunner()::execute, "test_add_col_desc_", "(a_varchar varchar)")) { + String tableName = table.getName(); + + assertUpdate("ALTER TABLE " + tableName + " ADD COLUMN b_varchar varchar COMMENT 'test new column comment'"); + assertThat(getColumnComment(tableName, "b_varchar")).isEqualTo("test new column comment"); + + assertUpdate("ALTER TABLE " + tableName + " ADD COLUMN empty_comment varchar COMMENT ''"); + assertEquals(getColumnComment(tableName, "empty_comment"), ""); + } + } + @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 26732cdccf7a..0863433cc245 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 @@ -58,6 +58,7 @@ public enum TestingConnectorBehavior SUPPORTS_RENAME_TABLE_ACROSS_SCHEMAS(SUPPORTS_RENAME_TABLE), SUPPORTS_ADD_COLUMN, + SUPPORTS_ADD_COLUMN_WITH_COMMENT(SUPPORTS_ADD_COLUMN), SUPPORTS_DROP_COLUMN, SUPPORTS_RENAME_COLUMN,