Deny adding column with comment if the connector does not support thi…

…s 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.
trinodb · Apr 4, 2022 · ec3aa36 · ec3aa36
1 parent addbffa
commit ec3aa36
Show file tree

Hide file tree

Showing 20 changed files with 65 additions and 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
@@ -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
@@ -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
@@ -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/...rino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java b/...rino-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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -197,6 +197,10 @@ public Optional<ConnectorOutputMetadata> 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
@@ -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
@@ -218,6 +218,10 @@ public Optional<ConnectorOutputMetadata> 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
@@ -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/...rino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java b/...rino-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
@@ -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/...no-raptor-legacy/src/test/java/io/trino/plugin/raptor/legacy/BaseRaptorConnectorTest.java b/...no-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/...o-singlestore/src/test/java/io/trino/plugin/singlestore/TestSingleStoreConnectorTest.java b/...o-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/...n/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerConnectorTest.java b/...n/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
@@ -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
@@ -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,
-Original file line number
+Diff line change
@@ Expand Up / @@ -284,6 +284,7 @@ public void testAddColumn() @@
         }
         @Test
+        @Override
         public void testAddColumnWithComment()
         {
             String tableName = "test_add_column_with_comment" + randomTableSuffix();
@@ Expand Down @@