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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 01 Jun 2018 20:05:34 +0000 Gerrit-HasComments: Yes
