Support COMMENT ON TABLE statement #200

ebyhr · 2019-02-09T10:21:47Z

martint · 2019-02-14T22:23:21Z

There's no equivalent functionality in the standard, but this syntax is fairly low risk with respect to future incompatibilities due to being a top-level statement that doesn't interact with anything else. While the syntax is not great (it's not consistent with every other operation on table metadata), it's similar to what Oracle and Postgres do, so let's stick with it.

martint

Some initial comments

martint · 2019-02-14T22:24:17Z

presto-accumulo/src/test/java/io/prestosql/plugin/accumulo/TestAccumuloDistributedQueries.java

+    public void testCommentTable()
+    {
+        // Accumulo connector currently does not support comment on table
+        assertQueryFails("COMMENT ON TABLE orders IS 'hello'", "This connector does not support commenting tables");


... does not support table comments ...

martint · 2019-02-14T22:26:20Z

presto-docs/src/main/sphinx/sql/comment-table.rst

@@ -0,0 +1,22 @@
+=============
+COMMENT TABLE


I'd refer to this just as "COMMENT". The current implementation only supports tables, but we might extend it in the future to support other object types like Postgres does (https://www.postgresql.org/docs/9.1/sql-comment.html)

martint · 2019-02-14T22:27:54Z

presto-main/src/main/java/io/prestosql/execution/CommentTableTask.java

+    @Override
+    public String getName()
+    {
+        return "COMMENT TABLE";


Just "COMMENT"

martint · 2019-02-14T22:38:41Z

presto-main/src/main/java/io/prestosql/security/AccessControl.java

+     * Check if identity is allowed to comment the specified table.
+     * @throws io.prestosql.spi.security.AccessDeniedException if not allowed
+     */
+    void checkCanAddComment(TransactionId transactionId, Identity identity, QualifiedObjectName tableName);


I'm not sure we want or need a dedicated access check for this operation. It should, generally, fall under "does the user have permission to update the table definition?".

We don't have generic permissions like that. There are separate checks like checkCanAddColumns, checkCanDropColumn, checkCanRenameColumn, checkCanRenameTable, etc.

How about making this checkCanSetTableComment?

martint · 2019-02-14T22:56:11Z

presto-parser/src/main/java/io/prestosql/sql/tree/CommentTable.java

+import static com.google.common.base.MoreObjects.toStringHelper;
+import static java.util.Objects.requireNonNull;
+
+public final class CommentTable


I'd make this a little more generic. I expect we'll be adding comments on other object types shortly after. It should be structured as

Comment

target: initially something that identifies a table, but eventually other types of entities, too. I'd make a new NamedObject that contains (type, name). Type can only be a table for now.

comment: a string

@martint My understanding is the suggestion means SqlBase.g4 also should be changed. Is it right?

Current:

| COMMENT ON TABLE qualifiedName IS string #commentTable

New:

| COMMENT ON commentObject qualifiedName IS string #comment ... commentObject : TABLE #commentTable ;

No, the grammar is fine. Rather than introducing a literal object, we can add a Type enum like other AST nodes use. It can be refactored into object later if needed.

I see. Thank you for answering.

electrum · 2019-02-25T09:52:17Z

presto-main/src/main/java/io/prestosql/security/AccessControl.java

+     * Check if identity is allowed to comment the specified table.
+     * @throws io.prestosql.spi.security.AccessDeniedException if not allowed
+     */
+    void checkCanAddComment(TransactionId transactionId, Identity identity, QualifiedObjectName tableName);


We don't have generic permissions like that. There are separate checks like checkCanAddColumns, checkCanDropColumn, checkCanRenameColumn, checkCanRenameTable, etc.

How about making this checkCanSetTableComment?

electrum · 2019-02-25T09:53:48Z

presto-docs/src/main/sphinx/sql/comment-table.rst

+Description
+-----------
+
+Add a comment to an existing table.


This is not just "add", since this can update or remove an existing comment. How about

Set the comment for a table. The comment can be removed by setting the comment to ``NULL``.

electrum · 2019-02-25T09:55:48Z

presto-docs/src/main/sphinx/sql/comment-table.rst

+Examples
+--------
+
+Add the comment ``master table`` to the ``users`` table::


Change the comment for the ``users`` table to be ``master table``::

electrum · 2019-02-25T10:00:37Z

presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorMetadata.java

+    /**
+     * Comments to the specified table
+     */
+    default void commentTable(ConnectorSession session, ConnectorTableHandle tableHandle, String comment)


Let's name this setTableComment and also change String to be Optional<String>. This aligns with how comment is represented in ConnectorTableMetadata and allows a single call to add/update/remove.

electrum · 2019-02-25T10:01:28Z

presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorMetadata.java

+     */
+    default void commentTable(ConnectorSession session, ConnectorTableHandle tableHandle, String comment)
+    {
+        throw new PrestoException(NOT_SUPPORTED, "This connector does not support commenting tables");


This connector does not support setting table comments

ebyhr · 2019-03-03T13:38:19Z

Changes:

Introduce enum for comment target object
Rename methods addComment* to setTableComment*
Use Optional<String> for comment body

martint · 2019-03-13T06:34:50Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java

+    public synchronized void commentTable(String databaseName, String tableName, Optional<String> comment)
+    {
+        alterTable(databaseName, tableName, oldTable -> {
+            return oldTable.withParameters(ImmutableMap.<String, String>builder()


This is a single expression, so return and {} are unnecessary.

martint · 2019-03-13T06:37:35Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/BridgingHiveMetastore.java

+        org.apache.hadoop.hive.metastore.api.Table table = source.get();
+
+        Map<String, String> parameters = table.getParameters();
+        parameters.put(TABLE_COMMENT, comment.orElse(""));


If comment is Optional.empty(), the comment should be removed. Does the hive metastore support removing comments explicitly or does it consider an empty comment the same as not having a comment? What happens if after clearing the comment you run DESCRIBE <table>? Does it show COMMENT ''?

hive supports both empty comment and removing a comment (ALTER UNSET). Actually, I tryied to remove the comment when it is empty, but followed the hive behaviour. Which do you think is better?

hive> ALTER TABLE table1 SET TBLPROPERTIES ('comment' = ''); hive> desc formatted table1; ... Table Parameters: comment last_modified_by root ...

hive> ALTER TABLE table1 UNSET TBLPROPERTIES ('comment'); hive> desc formatted table1; ... Table Parameters: last_modified_by root ...

martint · 2019-03-18T16:53:30Z

@ebyhr, can you squash the fixups into the corresponding commits? I'll do another review pass after that. Reassign to me when you've done it.

ebyhr · 2019-03-20T15:32:25Z

@martint Updated. Unfortunately, I don't have a permission to change the assignees.

ebyhr · 2019-03-20T15:56:53Z

I think this @electrum 's comment #200 (comment) requires SqlBase.g4 change like below. Please let me know if my understanding is wrong.

SqlBase.g4

COMMENT ON TABLE qualifiedName IS (string | NULL)                #commentTable

Current g4 shows mismatch error if I specify NULL.

presto:default> comment on table test_comment is null;
Query 20190320_160347_00001_s8r5r failed: line 1:34: mismatched input 'null'. Expecting: <string>
comment on table test_comment is null

electrum · 2019-03-25T18:24:40Z

@ebyhr Yes, we'll need a minor change to the grammar to support NULL.

ebyhr · 2019-03-26T11:13:30Z

Thank you for answering. Let me update to accept NULL.

ebyhr · 2019-03-26T15:13:23Z

Updated. Current behavior:

COMMENT ON TABLE table IS '' sets empty comment
COMMENT ON TABLE table IS NULL removes comment

martint

Just a few minor comments, but otherwise looks great! Once you rebased and address those issues, I'll merge it.

martint · 2019-04-02T00:03:42Z

presto-main/src/main/java/io/prestosql/execution/CommentTask.java

+    {
+        Session session = stateMachine.getSession();
+
+        if (statement.getType() == Comment.Type.TABLE) {


We should thrown a PrestoException with UNSUPPORTED code, otherwise.

Perhaps, NOT_SUPPORTED is right? I couldn't find UNSUPPORTED code. Updated like this.

else { throw new PrestoException(NOT_SUPPORTED, "Unsupported comment type: " + statement.getType()); }

Yeah, that’s what I meant.

martint · 2019-04-02T00:04:43Z

presto-main/src/main/java/io/prestosql/metadata/MetadataManager.java

+    @Override
+    public void setTableComment(Session session, TableHandle tableHandle, Optional<String> comment)
+    {
+        ConnectorId connectorId = tableHandle.getConnectorId();


This has been changed to CatalogName in recent commits.

martint · 2019-04-02T00:06:59Z

presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorAccessControl.java

+     *
+     * @throws io.prestosql.spi.security.AccessDeniedException if not allowed
+     */
+    default void checkCanCommentTable(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, SchemaTableName tableName)


Rename to checkCanSetTableComment for consistency with AccessControl.

martint · 2019-04-02T00:12:43Z

presto-parser/src/main/java/io/prestosql/sql/parser/AstBuilder.java

+    @Override
+    public Node visitCommentTable(SqlBaseParser.CommentTableContext context)
+    {
+        String comment = context.string() != null ? ((StringLiteral) visit(context.string())).getValue() : null;


I find this form easier to read:

Optional<String> comment = Optional.empy(); if (context.string() != null) { comment = Optional.of(((StringLiteral) visit(context.string())).getValue()) } return new Comment(...)

See how other visitXXX methods are implemented.

martint · 2019-04-02T00:19:43Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/BridgingHiveMetastore.java

+                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+        comment.ifPresent(value -> parameters.put(TABLE_COMMENT, comment.get()));
+
+        table.setParameters(ImmutableMap.copyOf(parameters));


Why ImmutableMap.copyOf?

Removed. It was debris during development.

ebyhr · 2019-04-03T04:46:47Z

@martint Thank you for your reviews. Rebased and applied the comments. Travis result is green.

martint · 2019-04-03T05:50:05Z

presto-spi/src/main/java/io/prestosql/spi/security/SystemAccessControl.java

+     *
+     * @throws AccessDeniedException if not allowed
+     */
+    default void checkCanCommentTable(Identity identity, CatalogSchemaTableName table)


This should be checkCanSetTableComment

ebyhr · 2019-04-03T06:03:16Z

Sorry, overlooked that. Updated once again.

martint

Looks great, thanks for your contribution!

cla-bot bot added the cla-signed label Feb 9, 2019

findepi added the syntax-needs-review label Feb 9, 2019

dain assigned martint Feb 14, 2019

martint removed the syntax-needs-review label Feb 14, 2019

martint reviewed Feb 14, 2019

View reviewed changes

martint assigned ebyhr and unassigned martint Feb 21, 2019

electrum reviewed Feb 25, 2019

View reviewed changes

ebyhr force-pushed the 4836 branch from e158f99 to 70fdf61 Compare March 3, 2019 13:33

martint reviewed Mar 13, 2019

View reviewed changes

ebyhr force-pushed the 4836 branch from f660bd0 to 30e6890 Compare March 20, 2019 15:31

ebyhr force-pushed the 4836 branch from 30e6890 to beb8fcc Compare March 26, 2019 15:10

martint requested changes Apr 2, 2019

View reviewed changes

ebyhr force-pushed the 4836 branch from beb8fcc to 5a57c08 Compare April 3, 2019 02:13

martint reviewed Apr 3, 2019

View reviewed changes

ebyhr added 3 commits April 3, 2019 15:01

Support COMMENT ON TABLE statement

3d0f76d

Implement COMMENT ON TABLE in hive connector

37d49d7

Add product test for COMMENT ON TABLE statement

5feedd1

ebyhr force-pushed the 4836 branch from 5a57c08 to 5feedd1 Compare April 3, 2019 06:02

martint approved these changes Apr 3, 2019

View reviewed changes

martint merged commit d813133 into trinodb:master Apr 3, 2019

martint mentioned this pull request Apr 3, 2019

Release notes for 307 #493

Closed

6 tasks

electrum added this to the 307 milestone Apr 4, 2019

martint mentioned this pull request Jan 16, 2020

Add support for COMMENT ON COLUMN statement #2516

Merged

ebyhr deleted the 4836 branch September 5, 2021 10:13

ebyhr mentioned this pull request Mar 2, 2022

Add table comments interface in base JDBC and implement in MySQL #11211

Merged

Support COMMENT ON TABLE statement #200

Support COMMENT ON TABLE statement #200

Conversation

ebyhr commented Feb 9, 2019 • edited by findepi

martint commented Feb 14, 2019

martint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum Mar 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Mar 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martint commented Mar 18, 2019 • edited

ebyhr commented Mar 20, 2019

ebyhr commented Mar 20, 2019 • edited

electrum commented Mar 25, 2019

ebyhr commented Mar 26, 2019

ebyhr commented Mar 26, 2019

martint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Apr 3, 2019

Choose a reason for hiding this comment

ebyhr commented Apr 3, 2019

martint left a comment

Choose a reason for hiding this comment

ebyhr commented Feb 9, 2019 •

edited by findepi

electrum Mar 2, 2019 •

edited

ebyhr commented Mar 3, 2019 •

edited

martint commented Mar 18, 2019 •

edited

ebyhr commented Mar 20, 2019 •

edited