Skip to content

Prevent COMMENT ON TABLE statement for view and materialized view #13161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ebyhr merged 2 commits into from
Jul 13, 2022
Merged

Prevent COMMENT ON TABLE statement for view and materialized view #13161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a59b90
Prevent COMMENT ON TABLE for view and materialized view
ebyhr Jul 13, 2022
c6da479
Improve error message when trying to comment on materialized views
ebyhr Jul 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion core/trino-main/src/main/java/io/trino/execution/CommentTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,20 @@ else if (statement.getType() == Comment.Type.COLUMN) {
private void commentOnTable(Comment statement, Session session)
{
QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, statement.getName());
if (metadata.isMaterializedView(session, originalTableName)) {
throw semanticException(
TABLE_NOT_FOUND,
statement,
"Table '%s' does not exist, but a materialized view with that name exists. Setting comments on materialized views is unsupported.", originalTableName);
}

if (metadata.isView(session, originalTableName)) {
throw semanticException(
TABLE_NOT_FOUND,
statement,
"Table '%1$s' does not exist, but a view with that name exists. Did you mean COMMENT ON VIEW %1$s IS ...?", originalTableName);
}

RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName);
if (redirectionAwareTableHandle.getTableHandle().isEmpty()) {
throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: %s", originalTableName);
Expand All @@ -105,7 +119,7 @@ private void commentOnView(Comment statement, Session session)
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.";
exceptionMessage += ", but a materialized view with that name exists. Setting comments on materialized views is unsupported.";
}
else if (metadata.getTableHandle(session, viewName).isPresent()) {
exceptionMessage += ", but a table with that name exists. Did you mean COMMENT ON TABLE " + viewName + " IS ...?";
Expand Down
12 changes: 12 additions & 0 deletions core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,18 @@ public void renameView(Session session, QualifiedObjectName source, QualifiedObj
views.remove(oldViewName);
}

@Override
public void setTableComment(Session session, TableHandle tableHandle, Optional<String> comment)
{
ConnectorTableMetadata tableMetadata = getTableMetadata(tableHandle);
ConnectorTableMetadata newTableMetadata = new ConnectorTableMetadata(
tableMetadata.getTable(),
tableMetadata.getColumns(),
tableMetadata.getProperties(),
comment);
tables.put(tableMetadata.getTable(), newTableMetadata);
}

@Override
public void setViewComment(Session session, QualifiedObjectName viewName, Optional<String> comment)
{
Expand Down
40 changes: 38 additions & 2 deletions core/trino-main/src/test/java/io/trino/execution/TestCommentTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

import static io.airlift.concurrent.MoreFutures.getFutureValue;
import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND;
import static io.trino.sql.tree.Comment.Type.TABLE;
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;
Expand All @@ -34,7 +35,42 @@
public class TestCommentTask
extends BaseDataDefinitionTaskTest
{
// TODO: Add test for 'COMMENT ON TABLE' and 'COMMENT ON COLUMN' statements
// TODO: Add test for 'COMMENT ON COLUMN' statements

@Test
public void testCommentTable()
{
QualifiedObjectName tableName = qualifiedObjectName("existing_table");
metadata.createTable(testSession, CATALOG_NAME, someTable(tableName), false);
assertThat(metadata.getTableMetadata(testSession, metadata.getTableHandle(testSession, tableName).get()).getMetadata().getComment())
.isEmpty();

getFutureValue(setComment(TABLE, asQualifiedName(tableName), Optional.of("new comment")));
assertThat(metadata.getTableMetadata(testSession, metadata.getTableHandle(testSession, tableName).get()).getMetadata().getComment())
.isEqualTo(Optional.of("new comment"));
}

@Test
public void testCommentTableOnView()
{
QualifiedObjectName viewName = qualifiedObjectName("existing_view");
metadata.createView(testSession, viewName, someView(), false);

assertTrinoExceptionThrownBy(() -> getFutureValue(setComment(TABLE, asQualifiedName(viewName), Optional.of("new comment"))))
.hasErrorCode(TABLE_NOT_FOUND)
.hasMessage("Table '%1$s' does not exist, but a view with that name exists. Did you mean COMMENT ON VIEW %1$s IS ...?", viewName);
}

@Test
public void testCommentTableOnMaterializedView()
{
QualifiedObjectName materializedViewName = qualifiedObjectName("existing_materialized_view");
metadata.createMaterializedView(testSession, QualifiedObjectName.valueOf(materializedViewName.toString()), someMaterializedView(), false, false);

assertTrinoExceptionThrownBy(() -> getFutureValue(setComment(TABLE, asQualifiedName(materializedViewName), Optional.of("new comment"))))
.hasErrorCode(TABLE_NOT_FOUND)
.hasMessage("Table '%s' does not exist, but a materialized view with that name exists. Setting comments on materialized views is unsupported.", materializedViewName);
}

@Test
public void testCommentView()
Expand Down Expand Up @@ -66,7 +102,7 @@ public void testCommentViewOnMaterializedView()

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);
.hasMessage("View '%s' does not exist, but a materialized view with that name exists. Setting comments on materialized views is unsupported.", materializedViewName);
}

private ListenableFuture<Void> setComment(Comment.Type type, QualifiedName viewName, Optional<String> comment)
Expand Down