Skip to content

Deny comment on unsupported table #11430

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

Closed
kyo-tom wants to merge 1 commit into from
Closed

Deny comment on unsupported table #11430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 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 @@ -15,12 +15,15 @@

import com.google.common.util.concurrent.ListenableFuture;
import io.trino.Session;
import io.trino.connector.CatalogName;
import io.trino.execution.warnings.WarningCollector;
import io.trino.metadata.Metadata;
import io.trino.metadata.MetadataUtil;
import io.trino.metadata.QualifiedObjectName;
import io.trino.metadata.RedirectionAwareTableHandle;
import io.trino.metadata.TableHandle;
import io.trino.security.AccessControl;
import io.trino.spi.TrinoException;
import io.trino.spi.connector.ColumnHandle;
import io.trino.sql.tree.Comment;
import io.trino.sql.tree.Expression;
Expand All @@ -38,6 +41,7 @@
import static io.trino.spi.StandardErrorCode.MISSING_TABLE;
import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND;
import static io.trino.spi.connector.ConnectorCapabilities.COMMENT_ON_TABLE;
import static io.trino.sql.analyzer.SemanticExceptions.semanticException;
import static java.util.Objects.requireNonNull;

Expand Down Expand Up @@ -76,6 +80,11 @@ public ListenableFuture<Void> execute(
throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: %s", originalTableName);
}

CatalogName catalogName = MetadataUtil.getRequiredCatalogHandle(metadata, session, statement, originalTableName.getCatalogName());
if (statement.getComment().isPresent() && !metadata.getConnectorCapabilities(session, catalogName).contains(COMMENT_ON_TABLE)) {
throw new TrinoException(NOT_SUPPORTED, "This connector does not support setting table comments");
}

accessControl.checkCanSetTableComment(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(originalTableName));
TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get();
metadata.setTableComment(session, tableHandle, statement.getComment());
Expand Down
5 changes: 5 additions & 0 deletions core/trino-main/src/main/java/io/trino/execution/CreateTableTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
import static io.trino.spi.StandardErrorCode.TABLE_ALREADY_EXISTS;
import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND;
import static io.trino.spi.StandardErrorCode.TYPE_NOT_FOUND;
import static io.trino.spi.connector.ConnectorCapabilities.COMMENT_ON_TABLE;
import static io.trino.spi.connector.ConnectorCapabilities.NOT_NULL_COLUMN_CONSTRAINT;
import static io.trino.sql.ParameterUtils.parameterExtractor;
import static io.trino.sql.analyzer.SemanticExceptions.semanticException;
Expand Down Expand Up @@ -135,6 +136,10 @@ ListenableFuture<Void> internalExecute(CreateTable statement, Session session, L

CatalogName catalogName = getRequiredCatalogHandle(plannerContext.getMetadata(), session, statement, tableName.getCatalogName());

if (statement.getComment().isPresent() && !plannerContext.getMetadata().getConnectorCapabilities(session, catalogName).contains(COMMENT_ON_TABLE)) {
throw new TrinoException(NOT_SUPPORTED, "This connector does not support setting table comments");
}

LinkedHashMap<String, ColumnMetadata> columns = new LinkedHashMap<>();
Map<String, Object> inheritedProperties = ImmutableMap.of();
boolean includingProperties = false;
Expand Down
1 change: 1 addition & 0 deletions core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorCapabilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,5 @@
public enum ConnectorCapabilities
{
NOT_NULL_COLUMN_CONSTRAINT,
COMMENT_ON_TABLE,
Copy link
Copy Markdown
Contributor Author

@kyo-tom kyo-tom Mar 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be add into MongoConnector after the pr #11424 merged

Copy link
Copy Markdown
Member

@findepi findepi Mar 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per #11348 (comment) i think it's too late to add this to ConnectorCapabilities. The damage has been done and the ship has sailed. Adding this now would be a breaking change.

cc @hashhar @ebyhr

Copy link
Copy Markdown
Member

@hashhar hashhar Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Instead I'd suggest to add a test to BaseConnectorTest that asserts that COMMENT ON TABLE works.
For the connectors where it fails you need to disallow it in their ConnectorMetadata implementations. (as discussed on #11348 (comment))

Copy link
Copy Markdown
Contributor Author

@kyo-tom kyo-tom Mar 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@findepi @hashhar Sorry, I'm a little busy these days so I can't reply in time.I don't understand why now is too late to add this to ConnectorCapabilities. Is there some connectors is supported COMMENT ON TABLE but is not supported CREATE TABLE ... COMENT.

}
9 changes: 9 additions & 0 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import io.trino.plugin.base.session.SessionPropertiesProvider;
import io.trino.spi.connector.Connector;
import io.trino.spi.connector.ConnectorAccessControl;
import io.trino.spi.connector.ConnectorCapabilities;
import io.trino.spi.connector.ConnectorMetadata;
import io.trino.spi.connector.ConnectorNodePartitioningProvider;
import io.trino.spi.connector.ConnectorPageSinkProvider;
Expand All @@ -39,6 +40,8 @@

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.Sets.immutableEnumSet;
import static io.trino.spi.connector.ConnectorCapabilities.COMMENT_ON_TABLE;
import static io.trino.spi.transaction.IsolationLevel.READ_UNCOMMITTED;
import static io.trino.spi.transaction.IsolationLevel.checkConnectorSupports;
import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -219,6 +222,12 @@ public final void shutdown()
lifeCycleManager.stop();
}

@Override
public Set<ConnectorCapabilities> getCapabilities()
{
return immutableEnumSet(COMMENT_ON_TABLE);
}

@Override
public Set<TableProcedureMetadata> getTableProcedures()
{
Expand Down
3 changes: 2 additions & 1 deletion plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.Sets.immutableEnumSet;
import static io.trino.spi.connector.ConnectorCapabilities.COMMENT_ON_TABLE;
import static io.trino.spi.connector.ConnectorCapabilities.NOT_NULL_COLUMN_CONSTRAINT;
import static io.trino.spi.transaction.IsolationLevel.SERIALIZABLE;
import static io.trino.spi.transaction.IsolationLevel.checkConnectorSupports;
Expand Down Expand Up @@ -94,7 +95,7 @@ public IcebergConnector(
@Override
public Set<ConnectorCapabilities> getCapabilities()
{
return immutableEnumSet(NOT_NULL_COLUMN_CONSTRAINT);
return immutableEnumSet(NOT_NULL_COLUMN_CONSTRAINT, COMMENT_ON_TABLE);
}

@Override
Expand Down
17 changes: 17 additions & 0 deletions testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2001,6 +2001,23 @@ public void testRenameTable()
assertFalse(getQueryRunner().tableExists(getSession(), renamedTable));
}

@Test
public void testCreateTableWithComment()
{
String tableName = "test_create_" + randomTableSuffix();
String createTable = "CREATE TABLE " + tableName + " (a bigint) COMMENT 'foo'";

if (!hasBehavior(SUPPORTS_COMMENT_ON_TABLE)) {
assertThatThrownBy(() ->
assertUpdate(createTable))
.hasMessage("This connector does not support setting table comments");
}
else {
assertUpdate(createTable);
assertThat(getTableComment(tableName)).isEqualTo("foo");
}
}

@Test
public void testCommentTable()
{
Expand Down