diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java index c030c525f7e8..a3c6dbd077a3 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java +++ b/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 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 comment) { @@ -2443,7 +2471,12 @@ public Optional 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 toConnectorViewDefinition(ConnectorSession session, SchemaTableName viewName, Optional table) + { + return table .filter(ViewReaderUtil::canDecodeView) .map(view -> { if (!translateHiveViews && !isPrestoView(view)) { diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java index e6c20fb9cff8..412ee005eb9e 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java +++ b/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); } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java index 0f32b3332244..0d44a913ebd8 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java +++ b/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 comment) + { + catalog.updateViewComment(session, viewName, comment); + } + @Override public Optional getNewTableLayout(ConnectorSession session, ConnectorTableMetadata tableMetadata) { diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java index efc9f83cc116..ad3307005517 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java +++ b/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 comment); + void updateViewComment(ConnectorSession session, SchemaTableName schemaViewName, Optional comment); + String defaultTableLocation(ConnectorSession session, SchemaTableName schemaTableName); void setTablePrincipal(ConnectorSession session, SchemaTableName schemaTableName, TrinoPrincipal principal); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java index fc5e64867a1a..c5568153f8ee 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java @@ -578,6 +578,33 @@ public Optional getView(ConnectorSession session, Schem Optional.ofNullable(viewDefinition.getOwner())); } + @Override + public void updateViewComment(ConnectorSession session, SchemaTableName viewName, Optional 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 listMaterializedViews(ConnectorSession session, Optional namespace) { diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java index 518a737312a9..963bb5059e44 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java +++ b/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 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 comment) { 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 6d40f71d0551..106f4bbe86eb 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 @@ -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); } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConnectorSmokeTest.java index 90d1e3efa560..7e808efb3778 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConnectorSmokeTest.java +++ b/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); diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestComments.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestComments.java index e9cdcb7f2713..85a5dea1018a 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestComments.java +++ b/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 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)