Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10358 )
Change subject: IMPALA-6802 (part 3): Clean up authorization tests ...................................................................... Patch Set 2: (9 comments) Taking over for Fredy while he's out. http://gerrit.cloudera.org:8080/#/c/10358/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java: http://gerrit.cloudera.org:8080/#/c/10358/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@96 PS2, Line 96: Set<String> expectedAuthorizables = Sets.newHashSet( > Let's factor this out somewhere, it's repeated many times Done http://gerrit.cloudera.org:8080/#/c/10358/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@718 PS2, Line 718: authorize("with t as (select id from functional.alltypes) select * from t") > Tests are fine, but overall there's still a lot of redundancy. Basically an Done http://gerrit.cloudera.org:8080/#/c/10358/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@736 PS2, Line 736: authorize("with t as (select id, int_col, 2019 from functional.alltypesagg) " + > Same here, this is basically the same as INSERT (without WITH) Done http://gerrit.cloudera.org:8080/#/c/10358/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@772 PS2, Line 772: authorize("select id from functional.alltypes union all " + > Same as SELECT with a join... also redundancy here Done http://gerrit.cloudera.org:8080/#/c/10358/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@842 PS2, Line 842: for (AuthzTest test : new AuthzTest[]{ > This is an interesting pattern for handling the redundancy I mentioned abov Done http://gerrit.cloudera.org:8080/#/c/10358/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@859 PS2, Line 859: for (AuthzTest test : new AuthzTest[]{ > redundancy with L842 This redundancy cannot be removed since the objects are different. http://gerrit.cloudera.org:8080/#/c/10358/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@880 PS2, Line 880: .error(refreshError("functional"), onDatabase("functional", allExcept( > also error with onServer() Done http://gerrit.cloudera.org:8080/#/c/10358/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@917 PS2, Line 917: // Show partitions. > Many of these require db or table level show metadata privs. Also add negat Done http://gerrit.cloudera.org:8080/#/c/10358/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1053 PS2, Line 1053: return new TPrivilegeLevel[]{TPrivilegeLevel.INSERT, TPrivilegeLevel.INSERT, > INSERT listed twice, is SELECT missing? Done -- To view, visit http://gerrit.cloudera.org:8080/10358 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9681cc3c7094db33ab7c5caa69b99dd803b908b7 Gerrit-Change-Number: 10358 Gerrit-PatchSet: 2 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-Comment-Date: Tue, 15 May 2018 18:06:28 +0000 Gerrit-HasComments: Yes
