Escape comment value in JDBC connectors

plugin/trino-accumulo/src/test/java/io/trino/plugin/accumulo/TestAccumuloConnectorTest.java

@@ -114,6 +114,15 @@ public void testCreateTableWithColumnComment()
         assertUpdate("DROP TABLE " + tableName);
     }
 
+    @Override
+    public void testCreateTableWithColumnCommentSpecialCharacter(String comment)
+    {
+        // TODO https://github.com/trinodb/trino/issues/14095 Enable this test after fixing the issue
+        assertThatThrownBy(() -> super.testCreateTableWithColumnCommentSpecialCharacter(comment))
+                .hasMessageContaining("expected [%s] but found [Accumulo row ID]".formatted(comment));
+        throw new SkipException("Accumulo connector ignores column comments when creating a new table");
+    }
+
     @Override
     public void testCreateTableAsSelect()
     {

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

@@ -1131,6 +1131,12 @@ protected String quoted(@Nullable String catalog, @Nullable String schema, Strin
         return sb.toString();
     }
 
+    public static String varcharLiteral(String value)
+    {
+        requireNonNull(value, "value is null");
+        return "'" + value.replace("'", "''") + "'";
+    }
+
     protected static Optional<String> escapeNamePattern(Optional<String> name, String escape)
     {
         return name.map(string -> escapeNamePattern(string, escape));

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java

@@ -168,6 +168,8 @@
 import static java.math.RoundingMode.UNNECESSARY;
 import static java.time.ZoneOffset.UTC;
 import static java.util.Locale.ENGLISH;
+import static java.util.Objects.requireNonNull;
+import static ru.yandex.clickhouse.ClickHouseUtil.escape;
 
 public class ClickHouseClient
         extends BaseJdbcClient
@@ -282,7 +284,7 @@ protected String createTableSql(RemoteTableName remoteTableName, List<String> co
         formatProperty(ClickHouseTableProperties.getPrimaryKey(tableProperties)).ifPresent(value -> tableOptions.add("PRIMARY KEY " + value));
         formatProperty(ClickHouseTableProperties.getPartitionBy(tableProperties)).ifPresent(value -> tableOptions.add("PARTITION BY " + value));
         ClickHouseTableProperties.getSampleBy(tableProperties).ifPresent(value -> tableOptions.add("SAMPLE BY " + value));
-        tableMetadata.getComment().ifPresent(comment -> tableOptions.add(format("COMMENT '%s'", comment)));
+        tableMetadata.getComment().ifPresent(comment -> tableOptions.add(format("COMMENT %s", clickhouseVarcharLiteral(comment))));
 
         return format("CREATE TABLE %s (%s) %s", quoted(remoteTableName), join(", ", columns), join(" ", tableOptions.build()));
     }
@@ -374,7 +376,7 @@ protected String getColumnDefinitionSql(ConnectorSession session, ColumnMetadata
             sb.append(toWriteMapping(session, column.getType()).getDataType());
         }
         if (column.getComment() != null) {
-            sb.append(format(" COMMENT '%s'", column.getComment()));
+            sb.append(format(" COMMENT %s", clickhouseVarcharLiteral(column.getComment())));
         }
         return sb.toString();
     }
@@ -426,23 +428,29 @@ protected String renameColumnSql(JdbcTableHandle handle, JdbcColumnHandle jdbcCo
     public void setTableComment(ConnectorSession session, JdbcTableHandle handle, Optional<String> comment)
     {
         String sql = format(
-                "ALTER TABLE %s MODIFY COMMENT '%s'",
+                "ALTER TABLE %s MODIFY COMMENT %s",
                 quoted(handle.asPlainTable().getRemoteTableName()),
-                comment.orElse(NO_COMMENT));
+                clickhouseVarcharLiteral(comment.orElse(NO_COMMENT)));
         execute(session, sql);
     }
 
     @Override
     public void setColumnComment(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle column, Optional<String> comment)
     {
         String sql = format(
-                "ALTER TABLE %s COMMENT COLUMN %s '%s'",
+                "ALTER TABLE %s COMMENT COLUMN %s %s",
                 quoted(handle.asPlainTable().getRemoteTableName()),
                 quoted(column.getColumnName()),
-                comment.orElse(""));
+                clickhouseVarcharLiteral(comment.orElse("")));
         execute(session, sql);
     }
 
+    private static String clickhouseVarcharLiteral(String value)
+    {
+        requireNonNull(value, "value is null");
+        return "'" + escape(value) + "'";
+    }
+
     @Override
     protected Optional<List<String>> getTableTypes()
     {

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseConnectorTest.java

@@ -94,6 +94,16 @@ public void testRenameColumn()
         throw new SkipException("TODO: test not implemented yet");
     }
 
+    @Override
+    public void testAddColumnWithCommentSpecialCharacter(String comment)
+    {
+        // Override because default storage engine doesn't support renaming columns
+        try (TestTable table = new TestTable(getQueryRunner()::execute, "test_add_column_", "(a_varchar varchar NOT NULL) WITH (engine = 'mergetree', order_by = ARRAY['a_varchar'])")) {
+            assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN b_varchar varchar COMMENT " + varcharLiteral(comment));
+            assertEquals(getColumnComment(table.getName(), "b_varchar"), comment);
+        }
+    }
+
     @Override
     public void testAddAndDropColumnName(String columnName)
     {

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java

@@ -61,6 +61,30 @@ public void testCreateTableAsSelectWithTableComment()
                 .hasMessageMatching("(?s).* Syntax error: .* COMMENT 'test comment'.*");
     }
 
+    @Override
+    public void testCreateTableWithTableCommentSpecialCharacter(String comment)
+    {
+        // Table comment is unsupported in old ClickHouse version
+        assertThatThrownBy(() -> super.testCreateTableWithTableCommentSpecialCharacter(comment))
+                .hasMessageMatching("(?s).* Syntax error: .* COMMENT .*");
+    }
+
+    @Override
+    public void testCreateTableAsSelectWithTableCommentSpecialCharacter(String comment)
+    {
+        // Table comment is unsupported in old ClickHouse version
+        assertThatThrownBy(() -> super.testCreateTableAsSelectWithTableCommentSpecialCharacter(comment))
+                .hasMessageMatching("(?s).* Syntax error: .* COMMENT .*");
+    }
+
+    @Override
+    public void testCommentTableSpecialCharacter(String comment)
+    {
+        // Table comment is unsupported in old ClickHouse version
+        assertThatThrownBy(() -> super.testCommentTableSpecialCharacter(comment))
+                .hasMessageMatching("(?s).* Syntax error: .* COMMENT .*");
+    }
+
     @Override
     protected OptionalInt maxTableNameLength()
     {

plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKuduConnectorTest.java

@@ -72,6 +72,7 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
                 return true;
             case SUPPORTS_RENAME_SCHEMA:
             case SUPPORTS_CREATE_TABLE_WITH_TABLE_COMMENT:
+            case SUPPORTS_CREATE_TABLE_WITH_COLUMN_COMMENT:
             case SUPPORTS_COMMENT_ON_TABLE:
             case SUPPORTS_COMMENT_ON_COLUMN:
             case SUPPORTS_ARRAY:
@@ -519,6 +520,19 @@ public void testAddColumnWithComment()
         assertUpdate("DROP TABLE " + tableName);
     }
 
+    @Override
+    public void testAddColumnWithCommentSpecialCharacter(String comment)
+    {
+        // Override because Kudu connector doesn't support creating a new table without partition columns
+        try (TestTable table = new TestTable(
+                getQueryRunner()::execute,
+                "test_add_col_",
+                "(id INT WITH (primary_key=true), a_varchar varchar) WITH (partition_by_hash_columns = ARRAY['id'], partition_by_hash_buckets = 2)")) {
+            assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN b_varchar varchar COMMENT " + varcharLiteral(comment));
+            assertEquals(getColumnComment(table.getName(), "b_varchar"), comment);
+        }
+    }
+
     @Test
     public void testInsertIntoTableHavingRowUuid()
     {

plugin/trino-mariadb/src/main/java/io/trino/plugin/mariadb/MariaDbClient.java

@@ -135,6 +135,7 @@
 import static java.lang.Math.min;
 import static java.lang.String.format;
 import static java.lang.String.join;
+import static java.util.Objects.requireNonNull;
 import static java.util.stream.Collectors.joining;
 
 public class MariaDbClient
@@ -269,9 +270,9 @@ public Optional<String> getTableComment(ResultSet resultSet)
     public void setTableComment(ConnectorSession session, JdbcTableHandle handle, Optional<String> comment)
     {
         String sql = format(
-                "ALTER TABLE %s COMMENT = '%s'",
+                "ALTER TABLE %s COMMENT = %s",
                 quoted(handle.asPlainTable().getRemoteTableName()),
-                comment.orElse(NO_COMMENT)); // An empty character removes the existing comment in MariaDB
+                mariaDbVarcharLiteral(comment.orElse(NO_COMMENT))); // An empty character removes the existing comment in MariaDB
         execute(session, sql);
     }
 
@@ -487,7 +488,13 @@ protected void copyTableSchema(Connection connection, String catalogName, String
     protected String createTableSql(RemoteTableName remoteTableName, List<String> columns, ConnectorTableMetadata tableMetadata)
     {
         checkArgument(tableMetadata.getProperties().isEmpty(), "Unsupported table properties: %s", tableMetadata.getProperties());
-        return format("CREATE TABLE %s (%s) COMMENT '%s'", quoted(remoteTableName), join(", ", columns), tableMetadata.getComment().orElse(NO_COMMENT));
+        return format("CREATE TABLE %s (%s) COMMENT %s", quoted(remoteTableName), join(", ", columns), mariaDbVarcharLiteral(tableMetadata.getComment().orElse(NO_COMMENT)));
+    }
+
+    private static String mariaDbVarcharLiteral(String value)
+    {
+        requireNonNull(value, "value is null");
+        return "'" + value.replace("'", "''").replace("\\", "\\\\") + "'";
     }
 
     @Override

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

@@ -329,9 +329,9 @@ public Optional<String> getTableComment(ResultSet resultSet)
     public void setTableComment(ConnectorSession session, JdbcTableHandle handle, Optional<String> comment)
     {
         String sql = format(
-                "ALTER TABLE %s COMMENT = '%s'",
+                "ALTER TABLE %s COMMENT = %s",
                 quoted(handle.asPlainTable().getRemoteTableName()),
-                comment.orElse(NO_COMMENT)); // An empty character removes the existing comment in MySQL
+                mysqlVarcharLiteral(comment.orElse(NO_COMMENT))); // An empty character removes the existing comment in MySQL
         execute(session, sql);
     }
 
@@ -347,7 +347,13 @@ protected String getTableSchemaName(ResultSet resultSet)
     protected String createTableSql(RemoteTableName remoteTableName, List<String> columns, ConnectorTableMetadata tableMetadata)
     {
         checkArgument(tableMetadata.getProperties().isEmpty(), "Unsupported table properties: %s", tableMetadata.getProperties());
-        return format("CREATE TABLE %s (%s) COMMENT '%s'", quoted(remoteTableName), join(", ", columns), tableMetadata.getComment().orElse(NO_COMMENT));
+        return format("CREATE TABLE %s (%s) COMMENT %s", quoted(remoteTableName), join(", ", columns), mysqlVarcharLiteral(tableMetadata.getComment().orElse(NO_COMMENT)));
+    }
+
+    private static String mysqlVarcharLiteral(String value)
+    {
+        requireNonNull(value, "value is null");
+        return "'" + value.replace("'", "''").replace("\\", "\\\\") + "'";
     }
 
     @Override

plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java

@@ -677,11 +677,12 @@ public WriteMapping toWriteMapping(ConnectorSession session, Type type)
     @Override
     public void setColumnComment(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle column, Optional<String> comment)
     {
+        // Oracle doesn't support prepared statement for COMMENT statement
         String sql = format(
-                "COMMENT ON COLUMN %s.%s IS '%s'",
+                "COMMENT ON COLUMN %s.%s IS %s",
                 quoted(handle.asPlainTable().getRemoteTableName()),
                 quoted(column.getColumnName()),
-                comment.orElse(""));
+                varcharLiteral(comment.orElse("")));
         execute(session, sql);
     }
 }

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java

@@ -23,6 +23,7 @@
 import io.trino.testing.TestingConnectorBehavior;
 import io.trino.testing.sql.TestTable;
 import io.trino.testing.sql.TestView;
+import org.testng.SkipException;
 import org.testng.annotations.Test;
 
 import java.util.List;
@@ -180,6 +181,18 @@ public void testCommentColumn()
         assertThat((String) computeActual("SHOW CREATE TABLE " + tableName).getOnlyValue()).doesNotContain("COMMENT 'new comment'");
     }
 
+    /**
+     * See {@link TestOraclePoolRemarksReportingConnectorSmokeTest#testCommentColumnSpecialCharacter(String comment)}
+     */
+    @Override
+    public void testCommentColumnSpecialCharacter(String comment)
+    {
+        // Oracle connector doesn't return column comments by default
+        assertThatThrownBy(() -> super.testCommentColumnSpecialCharacter(comment))
+                .hasMessageContaining("expected [%s] but found [null]".formatted(comment));
+        throw new SkipException("The test is covered in TestOraclePoolRemarksReportingConnectorSmokeTest");
+    }
+
     @Override
     public void testInformationSchemaFiltering()
     {

plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/TestOraclePoolRemarksReportingConnectorSmokeTest.java

@@ -15,16 +15,22 @@
 
 import com.google.common.collect.ImmutableMap;
 import io.airlift.testing.Closeables;
+import io.trino.testing.MaterializedResult;
 import io.trino.testing.QueryRunner;
+import io.trino.testing.sql.TestTable;
 import org.testng.annotations.AfterClass;
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
 import java.io.IOException;
 
 import static io.trino.plugin.oracle.TestingOracleServer.TEST_PASS;
 import static io.trino.plugin.oracle.TestingOracleServer.TEST_USER;
 import static io.trino.testing.sql.TestTable.randomTableSuffix;
+import static java.lang.String.format;
+import static java.util.Objects.requireNonNull;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.testng.Assert.assertEquals;
 
 public class TestOraclePoolRemarksReportingConnectorSmokeTest
         extends BaseOracleConnectorSmokeTest
@@ -72,4 +78,45 @@ public void testCommentColumn()
 
         assertUpdate("DROP TABLE " + tableName);
     }
+
+    @Test(dataProvider = "testCommentDataProvider")
+    public void testCommentColumnSpecialCharacter(String comment)
+    {
+        try (TestTable table = new TestTable(getQueryRunner()::execute, "test_comment_column_", "(a integer)")) {
+            assertUpdate("COMMENT ON COLUMN " + table.getName() + ".a IS " + varcharLiteral(comment));
+            assertEquals(getColumnComment(table.getName(), "a"), comment);
+        }
+    }
+
+    @DataProvider
+    public Object[][] testCommentDataProvider()
+    {
+        return new Object[][] {
+                {"a;semicolon"},
+                {"an@at"},
+                {"a\"quote"},
+                {"an'apostrophe"},
+                {"a`backtick`"},
+                {"a/slash`"},
+                {"a\\backslash`"},
+                {"a?question"},
+                {"[square bracket]"},
+        };
+    }
+
+    private static String varcharLiteral(String value)
+    {
+        requireNonNull(value, "value is null");
+        return "'" + value.replace("'", "''") + "'";
+    }
+
+    protected String getColumnComment(String tableName, String columnName)
+    {
+        MaterializedResult materializedResult = computeActual(format(
+                "SELECT comment FROM information_schema.columns WHERE table_schema = '%s' AND table_name = '%s' AND column_name = '%s'",
+                getSession().getSchema().orElseThrow(),
+                tableName,
+                columnName));
+        return (String) materializedResult.getOnlyValue();
+    }
 }

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java

@@ -1142,11 +1142,12 @@ private static ObjectWriteFunction longTimestampWriteFunction()
     @Override
     public void setColumnComment(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle column, Optional<String> comment)
     {
+        // PostgreSQL doesn't support prepared statement for COMMENT statement
         String sql = format(
                 "COMMENT ON COLUMN %s.%s IS %s",
                 quoted(handle.asPlainTable().getRemoteTableName()),
                 quoted(column.getColumnName()),
-                comment.isPresent() ? format("'%s'", comment.get()) : "NULL");
+                comment.map(BaseJdbcClient::varcharLiteral).orElse("NULL"));
         execute(session, sql);
     }
 

plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftClient.java

@@ -85,6 +85,7 @@
 import static io.trino.spi.type.VarbinaryType.VARBINARY;
 import static java.lang.Math.max;
 import static java.lang.String.format;
+import static java.util.Objects.requireNonNull;
 
 public class RedshiftClient
         extends BaseJdbcClient
@@ -172,14 +173,21 @@ public boolean isLimitGuaranteed(ConnectorSession session)
     @Override
     public void setColumnComment(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle column, Optional<String> comment)
     {
+        // Redshift doesn't support prepared statement for COMMENT statement
         String sql = format(
                 "COMMENT ON COLUMN %s.%s IS %s",
                 quoted(handle.asPlainTable().getRemoteTableName()),
                 quoted(column.getColumnName()),
-                comment.isPresent() ? format("'%s'", comment.get()) : "NULL");
+                comment.map(RedshiftClient::redshiftVarcharLiteral).orElse("NULL"));
         execute(session, sql);
     }
 
+    private static String redshiftVarcharLiteral(String value)
+    {
+        requireNonNull(value, "value is null");
+        return "'" + value.replace("'", "''").replace("\\", "\\\\") + "'";
+    }
+
     private static Optional<ColumnMapping> legacyDefaultColumnMapping(JdbcTypeHandle typeHandle)
     {
         // TODO (https://github.com/trinodb/trino/issues/497) Implement proper type mapping and add test