Skip to content

Add support for view column comments #13800

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 4 commits into from
Oct 28, 2022
Merged

Add support for view column comments #13800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1bfc9ae
Add support for view column comments
findinpath Oct 13, 2022
b53b1bd
Support view column comments in Hive
findinpath Oct 13, 2022
6cb4840
Support view column comments in Iceberg
findinpath Oct 13, 2022
5853537
Support view column comments in Memory
findinpath Oct 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
42 changes: 30 additions & 12 deletions 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 @@ -20,6 +20,8 @@
import io.trino.metadata.QualifiedObjectName;
import io.trino.metadata.RedirectionAwareTableHandle;
import io.trino.metadata.TableHandle;
import io.trino.metadata.ViewColumn;
import io.trino.metadata.ViewDefinition;
import io.trino.security.AccessControl;
import io.trino.spi.connector.ColumnHandle;
import io.trino.sql.tree.Comment;
Expand Down Expand Up @@ -135,21 +137,37 @@ private void commentOnColumn(Comment statement, Session session)
QualifiedName prefix = statement.getName().getPrefix()
.orElseThrow(() -> semanticException(MISSING_TABLE, statement, "Table must be specified"));

QualifiedObjectName originalTableName = createQualifiedObjectName(session, statement, prefix);
RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalTableName);
if (redirectionAwareTableHandle.getTableHandle().isEmpty()) {
throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: " + originalTableName);
QualifiedObjectName originalObjectName = createQualifiedObjectName(session, statement, prefix);
if (metadata.isView(session, originalObjectName)) {
String columnName = statement.getName().getSuffix();
ViewDefinition viewDefinition = metadata.getView(session, originalObjectName).get();
ViewColumn viewColumn = viewDefinition.getColumns().stream()
.filter(column -> column.getName().equals(columnName))
.findAny()
.orElseThrow(() -> semanticException(COLUMN_NOT_FOUND, statement, "Column does not exist: " + columnName));

accessControl.checkCanSetColumnComment(session.toSecurityContext(), originalObjectName);
metadata.setViewColumnComment(session, originalObjectName, viewColumn.getName(), statement.getComment());
}
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);
else if (metadata.isMaterializedView(session, originalObjectName)) {
throw semanticException(TABLE_NOT_FOUND, statement, "Setting comments on the columns of materialized views is unsupported");
}
else {
RedirectionAwareTableHandle redirectionAwareTableHandle = metadata.getRedirectionAwareTableHandle(session, originalObjectName);
if (redirectionAwareTableHandle.getTableHandle().isEmpty()) {
throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: " + originalObjectName);
}
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);
}

accessControl.checkCanSetColumnComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName));
accessControl.checkCanSetColumnComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalObjectName));

metadata.setColumnComment(session, tableHandle, columnHandles.get(columnName), statement.getComment());
metadata.setColumnComment(session, tableHandle, columnHandles.get(columnName), statement.getComment());
}
}
}
2 changes: 1 addition & 1 deletion core/trino-main/src/main/java/io/trino/execution/CreateMaterializedViewTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public ListenableFuture<Void> execute(

List<ViewColumn> columns = analysis.getOutputDescriptor(statement.getQuery())
.getVisibleFields().stream()
.map(field -> new ViewColumn(field.getName().get(), field.getType().getTypeId()))
.map(field -> new ViewColumn(field.getName().get(), field.getType().getTypeId(), Optional.empty()))
.collect(toImmutableList());

String catalogName = name.getCatalogName();
Expand Down
2 changes: 1 addition & 1 deletion core/trino-main/src/main/java/io/trino/execution/CreateViewTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ else if (metadata.getTableHandle(session, name).isPresent()) {

List<ViewColumn> columns = analysis.getOutputDescriptor(statement.getQuery())
.getVisibleFields().stream()
.map(field -> new ViewColumn(field.getName().get(), field.getType().getTypeId()))
.map(field -> new ViewColumn(field.getName().get(), field.getType().getTypeId(), Optional.empty()))
.collect(toImmutableList());

// use DEFINER security by default
Expand Down
2 changes: 1 addition & 1 deletion core/trino-main/src/main/java/io/trino/metadata/MaterializedViewDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public MaterializedViewDefinition(ConnectorMaterializedViewDefinition view, Iden
view.getCatalog(),
view.getSchema(),
view.getColumns().stream()
.map(column -> new ViewColumn(column.getName(), column.getType()))
.map(column -> new ViewColumn(column.getName(), column.getType(), Optional.empty()))
.collect(toImmutableList()),
view.getComment(),
Optional.of(runAsIdentity));
Expand Down
5 changes: 5 additions & 0 deletions core/trino-main/src/main/java/io/trino/metadata/Metadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,11 @@ Optional<TableExecuteHandle> getTableHandleForExecute(
*/
void setViewComment(Session session, QualifiedObjectName viewName, Optional<String> comment);

/**
* Comments to the specified view column.
*/
void setViewColumnComment(Session session, QualifiedObjectName viewName, String columnName, Optional<String> comment);

/**
* Comments to the specified column.
*/
Expand Down
15 changes: 14 additions & 1 deletion core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,11 @@ public List<TableColumnsMetadata> listTableColumns(Session session, QualifiedTab
ImmutableList.Builder<ColumnMetadata> columns = ImmutableList.builder();
for (ViewColumn column : entry.getValue().getColumns()) {
try {
columns.add(new ColumnMetadata(column.getName(), typeManager.getType(column.getType())));
columns.add(ColumnMetadata.builder()
.setName(column.getName())
.setType(typeManager.getType(column.getType()))
.setComment(column.getComment())
.build());
}
catch (TypeNotFoundException e) {
throw new TrinoException(INVALID_VIEW, format("Unknown type '%s' for column '%s' in view: %s", column.getType(), column.getName(), entry.getKey()));
Expand Down Expand Up @@ -698,6 +702,15 @@ public void setViewComment(Session session, QualifiedObjectName viewName, Option
metadata.setViewComment(session.toConnectorSession(catalogHandle), viewName.asSchemaTableName(), comment);
}

@Override
public void setViewColumnComment(Session session, QualifiedObjectName viewName, String columnName, Optional<String> comment)
{
CatalogMetadata catalogMetadata = getCatalogMetadataForWrite(session, viewName.getCatalogName());
CatalogHandle catalogHandle = catalogMetadata.getCatalogHandle();
ConnectorMetadata metadata = catalogMetadata.getMetadata(session);
metadata.setViewColumnComment(session.toConnectorSession(catalogHandle), viewName.asSchemaTableName(), columnName, comment);
}

@Override
public void setColumnComment(Session session, TableHandle tableHandle, ColumnHandle column, Optional<String> comment)
{
Expand Down
22 changes: 18 additions & 4 deletions core/trino-main/src/main/java/io/trino/metadata/ViewColumn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,23 @@
import io.trino.spi.type.TypeId;

import java.util.Objects;
import java.util.Optional;

import static com.google.common.base.MoreObjects.toStringHelper;
import static java.util.Objects.requireNonNull;

public final class ViewColumn
{
private final String name;
private final TypeId type;

public ViewColumn(String name, TypeId type)
private final Optional<String> comment;

public ViewColumn(String name, TypeId type, Optional<String> comment)
{
this.name = requireNonNull(name, "name is null");
this.type = requireNonNull(type, "type is null");
this.comment = requireNonNull(comment, "comment is null");
}

public String getName()
Expand All @@ -40,10 +45,19 @@ public TypeId getType()
return type;
}

public Optional<String> getComment()
{
return comment;
}

@Override
public String toString()
{
return name + " " + type;
return toStringHelper(this).omitNullValues()
.add("name", name)
.add("type", type)
.add("comment", comment.orElse(null))
.toString();
}

@Override
Expand All @@ -56,12 +70,12 @@ public boolean equals(Object o)
return false;
}
ViewColumn that = (ViewColumn) o;
return Objects.equals(name, that.name) && Objects.equals(type, that.type);
return Objects.equals(name, that.name) && Objects.equals(type, that.type) && Objects.equals(comment, that.comment);
}

@Override
public int hashCode()
{
return Objects.hash(name, type);
return Objects.hash(name, type, comment);
}
}
4 changes: 2 additions & 2 deletions core/trino-main/src/main/java/io/trino/metadata/ViewDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ private ViewDefinition(QualifiedObjectName viewName, ConnectorViewDefinition vie
this.catalog = view.getCatalog();
this.schema = view.getSchema();
this.columns = view.getColumns().stream()
.map(column -> new ViewColumn(column.getName(), column.getType()))
.map(column -> new ViewColumn(column.getName(), column.getType(), column.getComment()))
.collect(toImmutableList());
this.comment = view.getComment();
this.runAsIdentity = runAsIdentity;
Expand Down Expand Up @@ -124,7 +124,7 @@ public ConnectorViewDefinition toConnectorViewDefinition()
catalog,
schema,
columns.stream()
.map(column -> new ConnectorViewDefinition.ViewColumn(column.getName(), column.getType()))
.map(column -> new ConnectorViewDefinition.ViewColumn(column.getName(), column.getType(), column.getComment()))
.collect(toImmutableList()),
comment,
runAsIdentity.map(Identity::getUser),
Expand Down
4 changes: 2 additions & 2 deletions core/trino-main/src/main/java/io/trino/metadata/ViewInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public ViewInfo(ConnectorViewDefinition viewDefinition)
{
this.originalSql = viewDefinition.getOriginalSql();
this.columns = viewDefinition.getColumns().stream()
.map(column -> new ViewColumn(column.getName(), column.getType()))
.map(column -> new ViewColumn(column.getName(), column.getType(), column.getComment()))
.collect(toImmutableList());
this.comment = viewDefinition.getComment();
this.storageTable = Optional.empty();
Expand All @@ -44,7 +44,7 @@ public ViewInfo(ConnectorMaterializedViewDefinition viewDefinition)
{
this.originalSql = viewDefinition.getOriginalSql();
this.columns = viewDefinition.getColumns().stream()
.map(column -> new ViewColumn(column.getName(), column.getType()))
.map(column -> new ViewColumn(column.getName(), column.getType(), Optional.empty()))
.collect(toImmutableList());
this.comment = viewDefinition.getComment();
this.storageTable = viewDefinition.getStorageTable();
Expand Down
3 changes: 3 additions & 0 deletions core/trino-main/src/test/java/io/trino/connector/MockConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,9 @@ public void setTableComment(ConnectorSession session, ConnectorTableHandle table
@Override
public void setViewComment(ConnectorSession session, SchemaTableName viewName, Optional<String> comment) {}

@Override
public void setViewColumnComment(ConnectorSession session, SchemaTableName viewName, String columnName, Optional<String> comment) {}

@Override
public void setColumnComment(ConnectorSession session, ConnectorTableHandle tableHandle, ColumnHandle column, Optional<String> comment) {}

Expand Down
21 changes: 19 additions & 2 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 @@ -158,7 +158,7 @@ protected static QualifiedName asQualifiedName(QualifiedObjectName qualifiedObje

protected MaterializedViewDefinition someMaterializedView()
{
return someMaterializedView("select * from some_table", ImmutableList.of(new ViewColumn("test", BIGINT.getTypeId())));
return someMaterializedView("select * from some_table", ImmutableList.of(new ViewColumn("test", BIGINT.getTypeId(), Optional.empty())));
}

protected MaterializedViewDefinition someMaterializedView(String sql, List<ViewColumn> columns)
Expand All @@ -181,7 +181,7 @@ protected static ConnectorTableMetadata someTable(QualifiedObjectName tableName)

protected static ViewDefinition someView()
{
return viewDefinition("SELECT 1", ImmutableList.of(new ViewColumn("test", BIGINT.getTypeId())));
return viewDefinition("SELECT 1", ImmutableList.of(new ViewColumn("test", BIGINT.getTypeId(), Optional.empty())));
}

protected static ViewDefinition viewDefinition(String sql, ImmutableList<ViewColumn> columns)
Expand Down Expand Up @@ -442,6 +442,23 @@ public void setColumnComment(Session session, TableHandle tableHandle, ColumnHan
tables.put(tableMetadata.getTable(), newTableMetadata);
}

@Override
public void setViewColumnComment(Session session, QualifiedObjectName viewName, String columnName, Optional<String> comment)
{
ViewDefinition view = views.get(viewName.asSchemaTableName());
views.put(
viewName.asSchemaTableName(),
new ViewDefinition(
view.getOriginalSql(),
view.getCatalog(),
view.getSchema(),
view.getColumns().stream()
.map(currentViewColumn -> columnName.equals(currentViewColumn.getName()) ? new ViewColumn(currentViewColumn.getName(), currentViewColumn.getType(), comment) : currentViewColumn)
.collect(toImmutableList()),
view.getComment(),
view.getRunAsIdentity()));
}

@Override
public void renameMaterializedView(Session session, QualifiedObjectName source, QualifiedObjectName target)
{
Expand Down
19 changes: 19 additions & 0 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 @@ -27,6 +27,7 @@
import java.util.Optional;

import static io.airlift.concurrent.MoreFutures.getFutureValue;
import static io.trino.spi.StandardErrorCode.COLUMN_NOT_FOUND;
import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND;
import static io.trino.sql.tree.Comment.Type.COLUMN;
import static io.trino.sql.tree.Comment.Type.TABLE;
Expand Down Expand Up @@ -121,6 +122,24 @@ public void testCommentTableColumn()
.isEqualTo(Optional.of("new test column comment"));
}

@Test
public void testCommentViewColumn()
{
QualifiedObjectName viewName = qualifiedObjectName("existing_view");
QualifiedName columnName = qualifiedColumnName("existing_view", "test");
QualifiedName missingColumnName = qualifiedColumnName("existing_view", "missing");
metadata.createView(testSession, viewName, someView(), false);
assertThat(metadata.isView(testSession, viewName)).isTrue();

getFutureValue(setComment(COLUMN, columnName, Optional.of("new test column comment")));
assertThat(metadata.getView(testSession, viewName).get().getColumns().stream().filter(column -> "test".equals(column.getName())).findAny().get().getComment())
.isEqualTo(Optional.of("new test column comment"));

assertTrinoExceptionThrownBy(() -> getFutureValue(setComment(COLUMN, missingColumnName, Optional.of("comment for missing column"))))
.hasErrorCode(COLUMN_NOT_FOUND)
.hasMessage("Column does not exist: %s", missingColumnName.getSuffix());
}

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);
Expand Down
6 changes: 6 additions & 0 deletions core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,12 @@ public void setViewComment(Session session, QualifiedObjectName viewName, Option
throw new UnsupportedOperationException();
}

@Override
public void setViewColumnComment(Session session, QualifiedObjectName viewName, String columnName, Optional<String> comment)
{
throw new UnsupportedOperationException();
}

@Override
public void setColumnComment(Session session, TableHandle tableHandle, ColumnHandle column, Optional<String> comment)
{
Expand Down
6 changes: 6 additions & 0 deletions core/trino-main/src/test/java/io/trino/metadata/CountingAccessMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,12 @@ public void setViewComment(Session session, QualifiedObjectName viewName, Option
delegate.setViewComment(session, viewName, comment);
}

@Override
public void setViewColumnComment(Session session, QualifiedObjectName viewName, String columnName, Optional<String> comment)
{
delegate.setViewColumnComment(session, viewName, columnName, comment);
}

@Override
public void setColumnComment(Session session, TableHandle tableHandle, ColumnHandle column, Optional<String> comment)
{
Expand Down
2 changes: 1 addition & 1 deletion core/trino-main/src/test/java/io/trino/metadata/TestInformationSchemaMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public TestInformationSchemaMetadata()
"select 1",
Optional.of("test_catalog"),
Optional.of("test_schema"),
ImmutableList.of(new ViewColumn("test", BIGINT.getTypeId())),
ImmutableList.of(new ViewColumn("test", BIGINT.getTypeId(), Optional.of("test column comment"))),
Optional.of("comment"),
Optional.empty(),
true);
Expand Down
Loading