Support COMMENT ON VIEW statement in Hive & Iceberg connector

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

@@ -1257,6 +1257,34 @@ public void setTableComment(ConnectorSession session, ConnectorTableHandle table
         metastore.commentTable(handle.getSchemaName(), handle.getTableName(), comment);
     }
 
+    @Override
+    public void setViewComment(ConnectorSession session, SchemaTableName viewName, Optional<String> comment)
+    {
+        Table view = metastore.getTable(viewName.getSchemaName(), viewName.getTableName())
+                .orElseThrow(() -> new ViewNotFoundException(viewName));
+        if (translateHiveViews && !isPrestoView(view)) {
+            throw new HiveViewNotSupportedException(viewName);
+        }
+
+        ConnectorViewDefinition definition = toConnectorViewDefinition(session, viewName, Optional.of(view))
+                .orElseThrow(() -> new ViewNotFoundException(viewName));
+        ConnectorViewDefinition newDefinition = new ConnectorViewDefinition(
+                definition.getOriginalSql(),
+                definition.getCatalog(),
+                definition.getSchema(),
+                definition.getColumns(),
+                comment,
+                definition.getOwner(),
+                definition.isRunAsInvoker());
+
+        Table.Builder viewBuilder = Table.builder(view)
+                .setViewOriginalText(Optional.of(encodeViewData(newDefinition)));
+
+        PrincipalPrivileges principalPrivileges = accessControlMetadata.isUsingSystemSecurity() ? NO_PRIVILEGES : buildInitialPrivilegeSet(session.getUser());
+
+        metastore.replaceTable(viewName.getSchemaName(), viewName.getTableName(), viewBuilder.build(), principalPrivileges);
+    }
+
     @Override
     public void setColumnComment(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnHandle column, Optional<String> comment)
     {
@@ -2443,7 +2471,12 @@ public Optional<ConnectorViewDefinition> getView(ConnectorSession session, Schem
         if (isHiveSystemSchema(viewName.getSchemaName())) {
             return Optional.empty();
         }
-        return metastore.getTable(viewName.getSchemaName(), viewName.getTableName())
+        return toConnectorViewDefinition(session, viewName, metastore.getTable(viewName.getSchemaName(), viewName.getTableName()));
+    }
+
+    private Optional<ConnectorViewDefinition> toConnectorViewDefinition(ConnectorSession session, SchemaTableName viewName, Optional<Table> table)
+    {
+        return table
                 .filter(ViewReaderUtil::canDecodeView)
                 .map(view -> {
                     if (!translateHiveViews && !isPrestoView(view)) {

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java

@@ -227,6 +227,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
             case SUPPORTS_MULTI_STATEMENT_WRITES:
                 return true;
 
+            case SUPPORTS_COMMENT_ON_VIEW:
+                return true;
+
             default:
                 return super.hasBehavior(connectorBehavior);
         }

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

@@ -620,6 +620,12 @@ public void setTableComment(ConnectorSession session, ConnectorTableHandle table
         catalog.updateTableComment(session, ((IcebergTableHandle) tableHandle).getSchemaTableName(), comment);
     }
 
+    @Override
+    public void setViewComment(ConnectorSession session, SchemaTableName viewName, Optional<String> comment)
+    {
+        catalog.updateViewComment(session, viewName, comment);
+    }
+
     @Override
     public Optional<ConnectorTableLayout> getNewTableLayout(ConnectorSession session, ConnectorTableMetadata tableMetadata)
     {

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java

@@ -86,6 +86,8 @@ Transaction newCreateTableTransaction(
 
     void updateTableComment(ConnectorSession session, SchemaTableName schemaTableName, Optional<String> comment);
 
+    void updateViewComment(ConnectorSession session, SchemaTableName schemaViewName, Optional<String> comment);
+
     String defaultTableLocation(ConnectorSession session, SchemaTableName schemaTableName);
 
     void setTablePrincipal(ConnectorSession session, SchemaTableName schemaTableName, TrinoPrincipal principal);

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

@@ -578,6 +578,33 @@ public Optional<ConnectorViewDefinition> getView(ConnectorSession session, Schem
                 Optional.ofNullable(viewDefinition.getOwner()));
     }
 
+    @Override
+    public void updateViewComment(ConnectorSession session, SchemaTableName viewName, Optional<String> comment)
+    {
+        ConnectorViewDefinition definition = getView(session, viewName)
+                .orElseThrow(() -> new ViewNotFoundException(viewName));
+        ConnectorViewDefinition newDefinition = new ConnectorViewDefinition(
+                definition.getOriginalSql(),
+                definition.getCatalog(),
+                definition.getSchema(),
+                definition.getColumns(),
+                comment,
+                definition.getOwner(),
+                definition.isRunAsInvoker());
+
+        TableInput viewTableInput = getViewTableInput(viewName.getTableName(), encodeViewData(newDefinition), session.getUser(), createViewProperties(session));
+
+        try {
+            stats.getUpdateTable().call(() ->
+                    glueClient.updateTable(new UpdateTableRequest()
+                            .withDatabaseName(viewName.getSchemaName())
+                            .withTableInput(viewTableInput)));
+        }
+        catch (AmazonServiceException e) {
+            throw new TrinoException(ICEBERG_CATALOG_ERROR, e);
+        }
+    }
+
     @Override
     public List<SchemaTableName> listMaterializedViews(ConnectorSession session, Optional<String> namespace)
     {

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java

@@ -297,6 +297,31 @@ public void updateTableComment(ConnectorSession session, SchemaTableName schemaT
         super.updateTableComment(session, schemaTableName, comment);
     }
 
+    @Override
+    public void updateViewComment(ConnectorSession session, SchemaTableName viewName, Optional<String> comment)
+    {
+        io.trino.plugin.hive.metastore.Table view = metastore.getTable(viewName.getSchemaName(), viewName.getTableName())
+                .orElseThrow(() -> new ViewNotFoundException(viewName));
+
+        ConnectorViewDefinition definition = getView(viewName, view.getViewOriginalText(), view.getTableType(), view.getParameters(), view.getOwner())
+                .orElseThrow(() -> new ViewNotFoundException(viewName));
+        ConnectorViewDefinition newDefinition = new ConnectorViewDefinition(
+                definition.getOriginalSql(),
+                definition.getCatalog(),
+                definition.getSchema(),
+                definition.getColumns(),
+                comment,
+                definition.getOwner(),
+                definition.isRunAsInvoker());
+
+        io.trino.plugin.hive.metastore.Table.Builder viewBuilder = io.trino.plugin.hive.metastore.Table.builder(view)
+                .setViewOriginalText(Optional.of(encodeViewData(newDefinition)));
+
+        PrincipalPrivileges principalPrivileges = isUsingSystemSecurity ? NO_PRIVILEGES : buildInitialPrivilegeSet(session.getUser());
+
+        metastore.replaceTable(viewName.getSchemaName(), viewName.getTableName(), viewBuilder.build(), principalPrivileges);
+    }
+
     @Override
     public void updateColumnComment(ConnectorSession session, SchemaTableName schemaTableName, ColumnIdentity columnIdentity, Optional<String> comment)
     {

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

@@ -174,6 +174,9 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
             case SUPPORTS_DELETE:
             case SUPPORTS_UPDATE:
                 return true;
+
+            case SUPPORTS_COMMENT_ON_VIEW:
+                return true;
             default:
                 return super.hasBehavior(connectorBehavior);
         }

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConnectorSmokeTest.java

@@ -25,6 +25,7 @@
 import io.trino.plugin.iceberg.IcebergQueryRunner;
 import io.trino.plugin.iceberg.SchemaInitializer;
 import io.trino.testing.QueryRunner;
+import io.trino.testing.sql.TestView;
 import org.apache.iceberg.FileFormat;
 import org.testng.annotations.AfterClass;
 import org.testng.annotations.Parameters;
@@ -133,6 +134,37 @@ public void testRenameSchema()
                 .hasStackTraceContaining("renameNamespace is not supported for Iceberg Glue catalogs");
     }
 
+    @Test
+    public void testCommentView()
+    {
+        // TODO: Consider moving to BaseConnectorSmokeTest
+        try (TestView view = new TestView(getQueryRunner()::execute, "test_comment_view", "SELECT * FROM region")) {
+            // comment set
+            assertUpdate("COMMENT ON VIEW " + view.getName() + " IS 'new comment'");
+            assertThat((String) computeScalar("SHOW CREATE VIEW " + view.getName())).contains("COMMENT 'new comment'");
+            assertThat(getTableComment(view.getName())).isEqualTo("new comment");
+
+            // comment updated
+            assertUpdate("COMMENT ON VIEW " + view.getName() + " IS 'updated comment'");
+            assertThat(getTableComment(view.getName())).isEqualTo("updated comment");
+
+            // comment set to empty
+            assertUpdate("COMMENT ON VIEW " + view.getName() + " IS ''");
+            assertThat(getTableComment(view.getName())).isEmpty();
+
+            // comment deleted
+            assertUpdate("COMMENT ON VIEW " + view.getName() + " IS 'a comment'");
+            assertThat(getTableComment(view.getName())).isEqualTo("a comment");
+            assertUpdate("COMMENT ON VIEW " + view.getName() + " IS NULL");
+            assertThat(getTableComment(view.getName())).isNull();
+        }
+    }
+
+    private String getTableComment(String tableName)
+    {
+        return (String) computeScalar("SELECT comment FROM system.metadata.table_comments WHERE catalog_name = 'iceberg' AND schema_name = '" + schemaName + "' AND table_name = '" + tableName + "'");
+    }
+
     private String schemaPath()
     {
         return format("s3://%s/%s", bucketName, schemaName);

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestComments.java

@@ -19,25 +19,29 @@
 import io.trino.tempto.query.QueryResult;
 import org.testng.annotations.Test;
 
-import java.util.Optional;
-
 import static io.trino.tests.product.TestGroups.COMMENT;
+import static io.trino.tests.product.utils.QueryExecutors.onHive;
 import static io.trino.tests.product.utils.QueryExecutors.onTrino;
 import static java.lang.String.format;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 public class TestComments
         extends ProductTest
 {
     private static final String COMMENT_TABLE_NAME = "comment_test";
+    private static final String COMMENT_VIEW_NAME = "comment_view_test";
     private static final String COMMENT_COLUMN_NAME = "comment_column_test";
+    private static final String COMMENT_HIVE_VIEW_NAME = "comment_hive_view_test";
 
     @BeforeTestWithContext
     @AfterTestWithContext
     public void dropTestTable()
     {
         onTrino().executeQuery("DROP TABLE IF EXISTS " + COMMENT_TABLE_NAME);
+        onTrino().executeQuery("DROP VIEW IF EXISTS " + COMMENT_VIEW_NAME);
         onTrino().executeQuery("DROP TABLE IF EXISTS " + COMMENT_COLUMN_NAME);
+        onHive().executeQuery("DROP VIEW IF EXISTS " + COMMENT_HIVE_VIEW_NAME);
     }
 
     @Test(groups = COMMENT)
@@ -54,27 +58,50 @@ public void testCommentTable()
                 COMMENT_TABLE_NAME);
         onTrino().executeQuery(createTableSql);
 
-        QueryResult actualResult = onTrino().executeQuery("SHOW CREATE TABLE " + COMMENT_TABLE_NAME);
-        assertThat((String) actualResult.row(0).get(0)).matches(tableWithCommentPattern(COMMENT_TABLE_NAME, Optional.of("old comment")));
+        assertThat(getTableComment("hive", "default", COMMENT_TABLE_NAME)).isEqualTo("old comment");
 
         onTrino().executeQuery(format("COMMENT ON TABLE %s IS 'new comment'", COMMENT_TABLE_NAME));
-        actualResult = onTrino().executeQuery("SHOW CREATE TABLE " + COMMENT_TABLE_NAME);
-        assertThat((String) actualResult.row(0).get(0)).matches(tableWithCommentPattern(COMMENT_TABLE_NAME, Optional.of("new comment")));
+        assertThat(getTableComment("hive", "default", COMMENT_TABLE_NAME)).isEqualTo("new comment");
 
         onTrino().executeQuery(format("COMMENT ON TABLE %s IS ''", COMMENT_TABLE_NAME));
-        actualResult = onTrino().executeQuery("SHOW CREATE TABLE " + COMMENT_TABLE_NAME);
-        assertThat((String) actualResult.row(0).get(0)).matches(tableWithCommentPattern(COMMENT_TABLE_NAME, Optional.of("")));
+        assertThat(getTableComment("hive", "default", COMMENT_TABLE_NAME)).isEmpty();
 
         onTrino().executeQuery(format("COMMENT ON TABLE %s IS NULL", COMMENT_TABLE_NAME));
-        actualResult = onTrino().executeQuery("SHOW CREATE TABLE " + COMMENT_TABLE_NAME);
-        assertThat((String) actualResult.row(0).get(0)).matches(tableWithCommentPattern(COMMENT_TABLE_NAME, Optional.empty()));
+        assertThat(getTableComment("hive", "default", COMMENT_TABLE_NAME)).isNull();
+    }
+
+    @Test(groups = COMMENT)
+    public void testCommentView()
+    {
+        String createViewSql = format("" +
+                        "CREATE VIEW hive.default.%s " +
+                        "COMMENT 'old comment' " +
+                        "AS SELECT 1 AS col",
+                COMMENT_VIEW_NAME);
+        onTrino().executeQuery(createViewSql);
+
+        assertThat(getTableComment("hive", "default", COMMENT_VIEW_NAME)).isEqualTo("old comment");
+
+        onTrino().executeQuery(format("COMMENT ON VIEW %s IS 'new comment'", COMMENT_VIEW_NAME));
+        assertThat(getTableComment("hive", "default", COMMENT_VIEW_NAME)).isEqualTo("new comment");
+
+        onTrino().executeQuery(format("COMMENT ON VIEW %s IS ''", COMMENT_VIEW_NAME));
+        assertThat(getTableComment("hive", "default", COMMENT_VIEW_NAME)).isEmpty();
+
+        onTrino().executeQuery(format("COMMENT ON VIEW %s IS NULL", COMMENT_VIEW_NAME));
+        assertThat(getTableComment("hive", "default", COMMENT_VIEW_NAME)).isNull();
+
+        onTrino().executeQuery(format("CREATE TABLE hive.default.%s (col int)", COMMENT_TABLE_NAME));
+        onHive().executeQuery(format("CREATE VIEW default.%s AS SELECT * FROM default.%s", COMMENT_HIVE_VIEW_NAME, COMMENT_TABLE_NAME));
+        assertThatThrownBy(() -> onTrino().executeQuery(format("COMMENT ON VIEW %s IS NULL", COMMENT_HIVE_VIEW_NAME)))
+                .hasMessageContaining("Hive views are not supported");
     }
 
-    private String tableWithCommentPattern(String tableName, Optional<String> expectedComment)
+    private static String getTableComment(String catalogName, String schemaName, String tableName)
     {
-        return String.format("CREATE TABLE hive.default.\\Q%s\\E \\((?s:[^)]+)\\)\n", tableName) +
-                expectedComment.map(comment -> "COMMENT '" + comment + "'\n").orElse("") +
-                "WITH(?s:.*)";
+        String sql = "SELECT comment FROM system.metadata.table_comments WHERE catalog_name = '" + catalogName + "' AND schema_name = '" + schemaName + "' AND table_name = '" + tableName + "'";
+        QueryResult result = onTrino().executeQuery(sql);
+        return (String) result.row(0).get(0);
     }
 
     @Test(groups = COMMENT)