Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10471 )
Change subject: IMPALA-7016: Implement ALTER DATABASE SET OWNER ...................................................................... Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/10471/3/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/10471/3/common/thrift/JniCatalog.thrift@66 PS3, Line 66: SET_OWNER is there a plan (or jira) that points to more options here? http://gerrit.cloudera.org:8080/#/c/10471/3/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/10471/3/fe/src/main/cup/sql-parser.cup@268 PS3, Line 268: , KW_DRO that's not reserved nor on the list of planned keywords. will need to see what other places (docs?) need updates. http://gerrit.cloudera.org:8080/#/c/10471/5/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/10471/5/fe/src/main/cup/sql-parser.cup@1049 PS5, Line 1049: // Introducing OWNER keyword has a potential to be a breaking change, : // such that any names that use OWNER will need to be escaped. By using IDENT : // token we can make OWNER to be a keyword only in this statement. repeated comment... I'd prefer if the version on L1039 is used once and placed on L1034. http://gerrit.cloudera.org:8080/#/c/10471/5/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java: http://gerrit.cloudera.org:8080/#/c/10471/5/fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java@34 PS5, Line 34: owner_ noticed that there is no analysis method for this stmt. do we require that owner_ is a known user or any string is permitted? same question for the role. http://gerrit.cloudera.org:8080/#/c/10471/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/10471/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3535 PS5, Line 3535: private void alt move these two methods to L3516 http://gerrit.cloudera.org:8080/#/c/10471/5/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/10471/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@3917 PS5, Line 3917: foo is there a length limit on this string? http://gerrit.cloudera.org:8080/#/c/10471/5/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: http://gerrit.cloudera.org:8080/#/c/10471/5/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@3751 PS5, Line 3751: foo add the following variations for foo: 'foo' USER OWNER NULL '' add similar variations for db -- To view, visit http://gerrit.cloudera.org:8080/10471 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3b923021ebce5192d2d64784e7ddb952ba82bc3 Gerrit-Change-Number: 10471 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Adam Holley <g...@holleyism.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Wed, 06 Jun 2018 16:16:21 +0000 Gerrit-HasComments: Yes
