Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12817 )
Change subject: IMPALA-8328: Add missing ToSql test cases for authorization statements ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java File fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java: http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java@44 PS1, Line 44: if (isGrantStmt_) { : return String.format("GRANT ROLE %s TO GROUP %s", roleName_, groupName_); : } else { : return String.format("REVOKE ROLE %s FROM GROUP %s", roleName_, groupName_); : } nit: can be simplified by using ternary operation. http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java File fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java: http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java@297 PS1, Line 297: public TPrivilegeScope getScope_() { : return scope_; : } : : public TableName getTableName_() { : return tableName_; : } : : public HdfsUri getUri_() { : return uri_; : } : : public String getDbName_() { : return dbName_; : } : : public String getServerName_() { : return serverName_; : } nit: method names should not have _ at the end. Impala code style is to put the methods into a single line if possible. http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java: http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java@93 PS1, Line 93: switch (privilegeSpec_.getScope_()) { : case SERVER: : break; : case DATABASE: : sb.append(String.format(" %s", privilegeSpec_.getDbName_())); : break; : case TABLE: : sb.append(String.format(" %s", privilegeSpec_.getTableName_())); : break; : case URI: : sb.append(String.format(" '%s'", privilegeSpec_.getUri_())); : break; : } add default or else the compiler may give a warning. http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1478 PS1, Line 1478: try { : catalog_.addRole(testRole); : testToSql(ctx, String.format("GRANT SELECT ON SERVER server1 TO %s", testRole)); : testToSql(ctx, String.format("GRANT SELECT ON SERVER TO %s", testRole), : String.format("GRANT SELECT ON SERVER server1 TO %s", testRole)); : testToSql(ctx, String.format( : "GRANT ALL ON SERVER server1 TO %s WITH GRANT OPTION", testRole)); : : testToSql(ctx, String.format("REVOKE SELECT ON SERVER server1 FROM %s", testRole)); : testToSql(ctx, String.format("REVOKE SELECT ON SERVER FROM %s", testRole), : String.format("REVOKE SELECT ON SERVER server1 FROM %s", testRole)); : testToSql(ctx, String.format( : "REVOKE GRANT OPTION FOR ALL ON SERVER server1 FROM %s", testRole)); : } finally { : catalog_.removeRole(testRole); : } We can have a better coverage by iterating this code over list of privileges via Privilege.values(). Can we also test all the supported objects, i.e. server, database, table, column, uri? http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1520 PS1, Line 1520: testToSql(ctx, String.format("SHOW GRANT ROLE %s", testRole)); : testToSql(ctx, String.format("SHOW GRANT USER %s", testUser)); : testToSql(ctx, String.format("SHOW GRANT ROLE %s ON SERVER", testRole)); : testToSql(ctx, String.format("SHOW GRANT ROLE %s ON DATABASE functional", : testRole)); : testToSql(ctx, String.format("SHOW GRANT ROLE %s ON TABLE functional.alltypes", : testRole)); : testToSql(ctx, String.format("SHOW GRANT ROLE %s ON URI '%s'", : testRole, testUri)); do we support ON COLUMN? -- To view, visit http://gerrit.cloudera.org:8080/12817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I828d0459b6c92f7f15006e2353ce108093aa9dab Gerrit-Change-Number: 12817 Gerrit-PatchSet: 1 Gerrit-Owner: Austin Nobis <[email protected]> Gerrit-Reviewer: Austin Nobis <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 20 Mar 2019 21:49:01 +0000 Gerrit-HasComments: Yes
