Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10471 )
Change subject: IMPALA-7016: Implement ALTER DATABASE SET OWNER ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/10471/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10471/5//COMMIT_MSG@9 PS5, Line 9: wner > Is there some info that explains how the owner attribute will be used in Im Done 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? I remember seeing this: https://issues.apache.org/jira/browse/IMPALA-462 It will also be nice in the future if Impala can support ALTER DATABASE RENAME in the future. 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 w I think you were looking at the wrong patch set. I have removed KW_OWNER from the scanner. 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: parser.checkIdentKeyword("OWNER", owner_id); : RESULT = new AlterDbSetOwnerStmt(db, new Owner(TOwnerType.ROLE, role)); : :} > repeated comment... I'd prefer if the version on L1039 is used once and pla Done 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: super( > noticed that there is no analysis method for this stmt. do we require that In Hive, the user/role can be arbitrary. I'll add a check for the length though. 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: Preconditions. > move these two methods to L3516 Done 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: > is there a length limit on this string? Done 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: er" > add the following variations for foo: Done -- 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: 7 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Adam Holley <[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: Wed, 06 Jun 2018 19:19:57 +0000 Gerrit-HasComments: Yes
