Add support for COMMENT ON VIEW statement

core/trino-main/src/main/java/io/trino/execution/CommentTask.java

@@ -39,6 +39,7 @@
 import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
 import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND;
 import static io.trino.sql.analyzer.SemanticExceptions.semanticException;
+import static java.lang.String.format;
 import static java.util.Objects.requireNonNull;
 
 public class CommentTask
@@ -70,43 +71,74 @@ public ListenableFuture<Void> execute(
         Session session = stateMachine.getSession();
 
         if (statement.getType() == Comment.Type.TABLE) {
-            QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, statement.getName());
-            RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName);
-            if (redirectionAwareTableHandle.getTableHandle().isEmpty()) {
-                throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: %s", originalTableName);
-            }
-
-            accessControl.checkCanSetTableComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName));
-            TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get();
-            metadata.setTableComment(session, tableHandle, statement.getComment());
+            commentOnTable(statement, session);
+        }
+        else if (statement.getType() == Comment.Type.VIEW) {
+            commentOnView(statement, session);
         }
         else if (statement.getType() == Comment.Type.COLUMN) {
-            Optional<QualifiedName> prefix = statement.getName().getPrefix();
-            if (prefix.isEmpty()) {
-                throw semanticException(MISSING_TABLE, statement, "Table must be specified");
-            }
+            commentOnColumn(statement, session);
+        }
+        else {
+            throw semanticException(NOT_SUPPORTED, statement, "Unsupported comment type: %s", statement.getType());
+        }
 
-            QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, prefix.get());
-            RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName);
-            if (redirectionAwareTableHandle.getTableHandle().isEmpty()) {
-                throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: " + originalTableName);
-            }
-            TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get();
+        return immediateVoidFuture();
+    }
+
+    private void commentOnTable(Comment statement, Session session)
+    {
+        QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, statement.getName());
+        RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName);
+        if (redirectionAwareTableHandle.getTableHandle().isEmpty()) {
+            throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: %s", originalTableName);
+        }
 
-            String columnName = statement.getName().getSuffix();
-            Map<String, ColumnHandle> columnHandles = metadata.getColumnHandles(session, tableHandle);
-            if (!columnHandles.containsKey(columnName)) {
-                throw semanticException(COLUMN_NOT_FOUND, statement, "Column does not exist: " + columnName);
+        accessControl.checkCanSetTableComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName));
+        TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get();
+        metadata.setTableComment(session, tableHandle, statement.getComment());
+    }
+
+    private void commentOnView(Comment statement, Session session)
+    {
+        QualifiedObjectName viewName = createQualifiedObjectName(session, statement, statement.getName());
+        if (metadata.getView(session, viewName).isEmpty()) {
+            String exceptionMessage = format("View '%s' does not exist", viewName);
+            if (metadata.getMaterializedView(session, viewName).isPresent()) {
+                exceptionMessage += ", but a materialized view with that name exists.";
+            }
+            else if (metadata.getTableHandle(session, viewName).isPresent()) {
+                exceptionMessage += ", but a table with that name exists. Did you mean COMMENT ON TABLE " + viewName + " IS ...?";
             }
+            throw semanticException(TABLE_NOT_FOUND, statement, exceptionMessage);
+        }
+
+        accessControl.checkCanSetViewComment(session.toSecurityContext(), viewName);
+        metadata.setViewComment(session, viewName, statement.getComment());
+    }
 
-            accessControl.checkCanSetColumnComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName));
+    private void commentOnColumn(Comment statement, Session session)
+    {
+        Optional<QualifiedName> prefix = statement.getName().getPrefix();
+        if (prefix.isEmpty()) {
+            throw semanticException(MISSING_TABLE, statement, "Table must be specified");
+        }
 
-            metadata.setColumnComment(session, tableHandle, columnHandles.get(columnName), statement.getComment());
+        QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, prefix.get());
+        RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName);
+        if (redirectionAwareTableHandle.getTableHandle().isEmpty()) {
+            throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: " + originalTableName);
         }
-        else {
-            throw semanticException(NOT_SUPPORTED, statement, "Unsupported comment type: %s", statement.getType());
+        TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get();
+
+        String columnName = statement.getName().getSuffix();
+        Map<String, ColumnHandle> columnHandles = metadata.getColumnHandles(session, tableHandle);
+        if (!columnHandles.containsKey(columnName)) {
+            throw semanticException(COLUMN_NOT_FOUND, statement, "Column does not exist: " + columnName);
         }
 
-        return immediateVoidFuture();
+        accessControl.checkCanSetColumnComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName));
+
+        metadata.setColumnComment(session, tableHandle, columnHandles.get(columnName), statement.getComment());
     }
 }

core/trino-main/src/main/java/io/trino/metadata/Metadata.java

@@ -212,6 +212,11 @@ Optional<TableExecuteHandle> getTableHandleForExecute(
      */
     void setTableComment(Session session, TableHandle tableHandle, Optional<String> comment);
 
+    /**
+     * Comments to the specified view.
+     */
+    void setViewComment(Session session, QualifiedObjectName viewName, Optional<String> comment);
+
     /**
      * Comments to the specified column.
      */

core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java

@@ -675,6 +675,14 @@ public void setTableComment(Session session, TableHandle tableHandle, Optional<S
         metadata.setTableComment(session.toConnectorSession(catalogName), tableHandle.getConnectorHandle(), comment);
     }
 
+    @Override
+    public void setViewComment(Session session, QualifiedObjectName viewName, Optional<String> comment)
+    {
+        CatalogName catalogName = new CatalogName(viewName.getCatalogName());
+        ConnectorMetadata metadata = getMetadataForWrite(session, catalogName);
+        metadata.setViewComment(session.toConnectorSession(catalogName), viewName.asSchemaTableName(), comment);
+    }
+
     @Override
     public void setColumnComment(Session session, TableHandle tableHandle, ColumnHandle column, Optional<String> comment)
     {

core/trino-main/src/main/java/io/trino/security/AccessControl.java

@@ -197,6 +197,13 @@ public interface AccessControl
      */
     void checkCanSetTableComment(SecurityContext context, QualifiedObjectName tableName);
 
+    /**
+     * Check if identity is allowed to comment the specified view.
+     *
+     * @throws AccessDeniedException if not allowed
+     */
+    void checkCanSetViewComment(SecurityContext context, QualifiedObjectName viewName);
+
     /**
      * Check if identity is allowed to comment the specified column.
      *

core/trino-main/src/main/java/io/trino/security/AccessControlManager.java

@@ -482,6 +482,19 @@ public void checkCanSetTableComment(SecurityContext securityContext, QualifiedOb
         catalogAuthorizationCheck(tableName.getCatalogName(), securityContext, (control, context) -> control.checkCanSetTableComment(context, tableName.asSchemaTableName()));
     }
 
+    @Override
+    public void checkCanSetViewComment(SecurityContext securityContext, QualifiedObjectName viewName)
+    {
+        requireNonNull(securityContext, "securityContext is null");
+        requireNonNull(viewName, "viewName is null");
+
+        checkCanAccessCatalog(securityContext, viewName.getCatalogName());
+
+        systemAuthorizationCheck(control -> control.checkCanSetViewComment(securityContext.toSystemSecurityContext(), viewName.asCatalogSchemaTableName()));
+
+        catalogAuthorizationCheck(viewName.getCatalogName(), securityContext, (control, context) -> control.checkCanSetViewComment(context, viewName.asSchemaTableName()));
+    }
+
     @Override
     public void checkCanSetColumnComment(SecurityContext securityContext, QualifiedObjectName tableName)
     {

core/trino-main/src/main/java/io/trino/security/AllowAllAccessControl.java

@@ -144,6 +144,11 @@ public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName
     {
     }
 
+    @Override
+    public void checkCanSetViewComment(SecurityContext context, QualifiedObjectName viewName)
+    {
+    }
+
     @Override
     public void checkCanSetColumnComment(SecurityContext context, QualifiedObjectName tableName)
     {

core/trino-main/src/main/java/io/trino/security/DenyAllAccessControl.java

@@ -33,6 +33,7 @@
 import static io.trino.spi.security.AccessDeniedException.denyAddColumn;
 import static io.trino.spi.security.AccessDeniedException.denyCommentColumn;
 import static io.trino.spi.security.AccessDeniedException.denyCommentTable;
+import static io.trino.spi.security.AccessDeniedException.denyCommentView;
 import static io.trino.spi.security.AccessDeniedException.denyCreateMaterializedView;
 import static io.trino.spi.security.AccessDeniedException.denyCreateRole;
 import static io.trino.spi.security.AccessDeniedException.denyCreateSchema;
@@ -216,6 +217,12 @@ public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName
         denyCommentTable(tableName.toString());
     }
 
+    @Override
+    public void checkCanSetViewComment(SecurityContext context, QualifiedObjectName viewName)
+    {
+        denyCommentView(viewName.toString());
+    }
+
     @Override
     public void checkCanSetColumnComment(SecurityContext context, QualifiedObjectName tableName)
     {

core/trino-main/src/main/java/io/trino/security/ForwardingAccessControl.java

@@ -191,6 +191,12 @@ public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName
         delegate().checkCanSetTableComment(context, tableName);
     }
 
+    @Override
+    public void checkCanSetViewComment(SecurityContext context, QualifiedObjectName viewName)
+    {
+        delegate().checkCanSetViewComment(context, viewName);
+    }
+
     @Override
     public void checkCanSetColumnComment(SecurityContext context, QualifiedObjectName tableName)
     {

core/trino-main/src/main/java/io/trino/security/InjectedConnectorAccessControl.java

@@ -149,6 +149,13 @@ public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTabl
         accessControl.checkCanSetTableComment(securityContext, getQualifiedObjectName(tableName));
     }
 
+    @Override
+    public void checkCanSetViewComment(ConnectorSecurityContext context, SchemaTableName viewName)
+    {
+        checkArgument(context == null, "context must be null");
+        accessControl.checkCanSetViewComment(securityContext, getQualifiedObjectName(viewName));
+    }
+
     @Override
     public void checkCanSetColumnComment(ConnectorSecurityContext context, SchemaTableName tableName)
     {

core/trino-main/src/main/java/io/trino/testing/AllowAllAccessControlManager.java

@@ -108,6 +108,9 @@ public void checkCanSetTableProperties(SecurityContext context, QualifiedObjectN
     @Override
     public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName tableName) {}
 
+    @Override
+    public void checkCanSetViewComment(SecurityContext context, QualifiedObjectName viewName) {}
+
     @Override
     public void checkCanSetColumnComment(SecurityContext context, QualifiedObjectName tableName) {}
 

core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java

@@ -49,6 +49,7 @@
 import static io.trino.spi.security.AccessDeniedException.denyAddColumn;
 import static io.trino.spi.security.AccessDeniedException.denyCommentColumn;
 import static io.trino.spi.security.AccessDeniedException.denyCommentTable;
+import static io.trino.spi.security.AccessDeniedException.denyCommentView;
 import static io.trino.spi.security.AccessDeniedException.denyCreateMaterializedView;
 import static io.trino.spi.security.AccessDeniedException.denyCreateSchema;
 import static io.trino.spi.security.AccessDeniedException.denyCreateTable;
@@ -86,6 +87,7 @@
 import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.ADD_COLUMN;
 import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.COMMENT_COLUMN;
 import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.COMMENT_TABLE;
+import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.COMMENT_VIEW;
 import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.CREATE_MATERIALIZED_VIEW;
 import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.CREATE_SCHEMA;
 import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.CREATE_TABLE;
@@ -387,6 +389,17 @@ public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName
         }
     }
 
+    @Override
+    public void checkCanSetViewComment(SecurityContext context, QualifiedObjectName viewName)
+    {
+        if (shouldDenyPrivilege(context.getIdentity().getUser(), viewName.getObjectName(), COMMENT_VIEW)) {
+            denyCommentView(viewName.toString());
+        }
+        if (denyPrivileges.isEmpty()) {
+            super.checkCanSetViewComment(context, viewName);
+        }
+    }
+
     @Override
     public void checkCanSetColumnComment(SecurityContext context, QualifiedObjectName tableName)
     {
@@ -698,7 +711,7 @@ public enum TestingPrivilegeType
         EXECUTE_QUERY, VIEW_QUERY, KILL_QUERY,
         EXECUTE_FUNCTION,
         CREATE_SCHEMA, DROP_SCHEMA, RENAME_SCHEMA,
-        SHOW_CREATE_TABLE, CREATE_TABLE, DROP_TABLE, RENAME_TABLE, COMMENT_TABLE, COMMENT_COLUMN, INSERT_TABLE, DELETE_TABLE, UPDATE_TABLE, TRUNCATE_TABLE, SET_TABLE_PROPERTIES, SHOW_COLUMNS,
+        SHOW_CREATE_TABLE, CREATE_TABLE, DROP_TABLE, RENAME_TABLE, COMMENT_TABLE, COMMENT_VIEW, COMMENT_COLUMN, INSERT_TABLE, DELETE_TABLE, UPDATE_TABLE, TRUNCATE_TABLE, SET_TABLE_PROPERTIES, SHOW_COLUMNS,
         ADD_COLUMN, DROP_COLUMN, RENAME_COLUMN, SELECT_COLUMN,
         CREATE_VIEW, RENAME_VIEW, DROP_VIEW, CREATE_VIEW_WITH_SELECT_COLUMNS,
         CREATE_MATERIALIZED_VIEW, REFRESH_MATERIALIZED_VIEW, DROP_MATERIALIZED_VIEW, RENAME_MATERIALIZED_VIEW, SET_MATERIALIZED_VIEW_PROPERTIES,

core/trino-main/src/test/java/io/trino/connector/MockConnector.java

@@ -489,6 +489,9 @@ public void setTableProperties(ConnectorSession session, ConnectorTableHandle ta
         @Override
         public void setTableComment(ConnectorSession session, ConnectorTableHandle tableHandle, Optional<String> comment) {}
 
+        @Override
+        public void setViewComment(ConnectorSession session, SchemaTableName viewName, Optional<String> comment) {}
+
         @Override
         public void setColumnComment(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnHandle column, Optional<String> comment) {}
 

core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java

@@ -391,6 +391,21 @@ public void renameView(Session session, QualifiedObjectName source, QualifiedObj
             views.remove(oldViewName);
         }
 
+        @Override
+        public void setViewComment(Session session, QualifiedObjectName viewName, Optional<String> comment)
+        {
+            ViewDefinition view = views.get(viewName.asSchemaTableName());
+            views.put(
+                    viewName.asSchemaTableName(),
+                    new ViewDefinition(
+                            view.getOriginalSql(),
+                            view.getCatalog(),
+                            view.getSchema(),
+                            view.getColumns(),
+                            comment,
+                            view.getRunAsIdentity()));
+        }
+
         @Override
         public void renameMaterializedView(Session session, QualifiedObjectName source, QualifiedObjectName target)
         {

core/trino-main/src/test/java/io/trino/execution/TestCommentTask.java

@@ -0,0 +1,76 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.execution;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.util.concurrent.ListenableFuture;
+import io.trino.execution.warnings.WarningCollector;
+import io.trino.metadata.QualifiedObjectName;
+import io.trino.security.AllowAllAccessControl;
+import io.trino.sql.tree.Comment;
+import io.trino.sql.tree.QualifiedName;
+import org.testng.annotations.Test;
+
+import java.util.Optional;
+
+import static io.airlift.concurrent.MoreFutures.getFutureValue;
+import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND;
+import static io.trino.sql.tree.Comment.Type.VIEW;
+import static io.trino.testing.assertions.TrinoExceptionAssert.assertTrinoExceptionThrownBy;
+import static org.assertj.core.api.Assertions.assertThat;
+
+@Test(singleThreaded = true)
+public class TestCommentTask
+        extends BaseDataDefinitionTaskTest
+{
+    // TODO: Add test for 'COMMENT ON TABLE' and 'COMMENT ON COLUMN' statements
+
+    @Test
+    public void testCommentView()
+    {
+        QualifiedObjectName viewName = qualifiedObjectName("existing_view");
+        metadata.createView(testSession, viewName, someView(), false);
+        assertThat(metadata.isView(testSession, viewName)).isTrue();
+
+        getFutureValue(setComment(VIEW, asQualifiedName(viewName), Optional.of("new comment")));
+        assertThat(metadata.getView(testSession, viewName).get().getComment()).isEqualTo(Optional.of("new comment"));
+    }
+
+    @Test
+    public void testCommentViewOnTable()
+    {
+        QualifiedObjectName tableName = qualifiedObjectName("existing_table");
+        metadata.createTable(testSession, CATALOG_NAME, someTable(tableName), false);
+
+        assertTrinoExceptionThrownBy(() -> getFutureValue(setComment(VIEW, asQualifiedName(tableName), Optional.of("new comment"))))
+                .hasErrorCode(TABLE_NOT_FOUND)
+                .hasMessage("View '%1$s' does not exist, but a table with that name exists. Did you mean COMMENT ON TABLE %1$s IS ...?", tableName);
+    }
+
+    @Test
+    public void testCommentViewOnMaterializedView()
+    {
+        QualifiedObjectName materializedViewName = qualifiedObjectName("existing_materialized_view");
+        metadata.createMaterializedView(testSession, QualifiedObjectName.valueOf(materializedViewName.toString()), someMaterializedView(), false, false);
+
+        assertTrinoExceptionThrownBy(() -> getFutureValue(setComment(VIEW, asQualifiedName(materializedViewName), Optional.of("new comment"))))
+                .hasErrorCode(TABLE_NOT_FOUND)
+                .hasMessage("View '%s' does not exist, but a materialized view with that name exists.", materializedViewName);
+    }
+
+    private ListenableFuture<Void> setComment(Comment.Type type, QualifiedName viewName, Optional<String> comment)
+    {
+        return new CommentTask(metadata, new AllowAllAccessControl()).execute(new Comment(type, viewName, comment), queryStateMachine, ImmutableList.of(), WarningCollector.NOOP);
+    }
+}