Austin Nobis 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 2: (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: return String.format("%s ROLE %s %s GROUP %s", : isGrantStmt_ ? "GRANT" : "REVOKE", roleName_, : isGrantStmt_ ? "TO" : "FROM", groupName_); : } : > nit: can be simplified by using ternary operation. Done 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 Done 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. Done 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: @Test : public void testGrantRevokePrivStmt() { : AnalysisContext ctx = createAnalysisCtx(createAuthorizationConfig()); : String testRole = "test_role"; : String testUri = "hdfs://localhost:20500/test-warehouse"; : : try { : catalog_.addRole(testRole); : List<Privilege> privileges = Arrays.stream(Privilege.values()) : .filter(p -> p != Privilege.OWNER && : p != Privilege.VIEW_METADATA && : p != Privilege.ANY) : .collect(Collectors.toList()); : : for (Privilege p : privileges) { : > We can have a better coverage by iterating this code over list of privilege Done http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1520 PS1, Line 1520: p != Privilege.VIEW_METADATA && : p != Privilege.ANY) : .collect(Collectors.toList()); : : for (Privilege p : privileges) { : // Table : testToSql(ctx, String.format("GRANT %s ON TABLE functional.alltypes TO %s", : p, testRole)); : testToSql(ctx, String. > do we support ON COLUMN? Checking the grammar in the .cup file I see support for the following Privilege Specifications: - null (not sure how this works) - ALL on SERVER - ALL on DATABASE - ALL on TABLE - ALL on HdfsUri -- 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: 2 Gerrit-Owner: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Thu, 21 Mar 2019 13:12:58 +0000 Gerrit-HasComments: Yes
