[Impala-ASF-CR] IMPALA-6917: Implement COMMENT ON TABLE/VIEW

["Vuk Ercegovac (Code Review)" <ge...@cloudera.org>]

Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10478 )

Change subject: IMPALA-6917: Implement COMMENT ON TABLE/VIEW
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/10478/4/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:

http://gerrit.cloudera.org:8080/#/c/10478/4/common/thrift/JniCatalog.thrift@635
PS4, Line 635: Name of comment to alter
I realize its from a prev. change, but what does "Name of comment" mean? Perhaps its should just be: Contents of comment to alter?


http://gerrit.cloudera.org:8080/#/c/10478/4/common/thrift/JniCatalog.thrift@638
PS4, Line 638: // Name of database to alter.
             :   2: optional string db
             : 
             :   // Name of table/view to alter.
             :   3: optional CatalogObjects.TTableName table_name
state that exactly one of these must be set. its ambiguous if comment can be applied to more than one obj at a time. ideally would use a protobuf one-of, but not sure if thrift has this.


http://gerrit.cloudera.org:8080/#/c/10478/4/fe/src/main/java/org/apache/impala/analysis/CommentOnViewStmt.java
File fe/src/main/java/org/apache/impala/analysis/CommentOnViewStmt.java:

http://gerrit.cloudera.org:8080/#/c/10478/4/fe/src/main/java/org/apache/impala/analysis/CommentOnViewStmt.java@39
PS4, Line 39: if (!(tableRef instanceof InlineViewRef)) {
            :       throw new AnalysisException(String.format(
            :           "COMMENT ON VIEW not allowed on a table: %s", tableName_));
            :     }
It looks like this is the key thing that differentiates view vs. table. How about we just differentiate these two with a flag-- that would squash these two classes for table and view back into the current super class.


http://gerrit.cloudera.org:8080/#/c/10478/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java:

http://gerrit.cloudera.org:8080/#/c/10478/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@3915
PS4, Line 3915: for (Pair<String, AnalysisContext> pair : new Pair[]{
              :         new Pair<>("functional.alltypes", createAnalysisCtx()),
              :      
perhaps factor this if it does not obfuscate the test.


http://gerrit.cloudera.org:8080/#/c/10478/4/tests/metadata/test_ddl.py
File tests/metadata/test_ddl.py:

http://gerrit.cloudera.org:8080/#/c/10478/4/tests/metadata/test_ddl.py@255
PS4, Line 255: 'comment'
perhaps try:

- a comment with escaped quote
- a very large comment (what's the limit?)



-- 
To view, visit http://gerrit.cloudera.org:8080/10478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I497c17342f79ff7c99931fd8a0ddec0c79303dbf
Gerrit-Change-Number: 10478
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Jun 2018 20:05:34 +0000
Gerrit-HasComments: Yes