Alex Behm 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) 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 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 any "read" query that produces the same privilege requests will have these same checks. We should think about how we can coalesce them. We don't need to do that in this patch, but we should not forget. 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) 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 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 above 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 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() 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 negative tests that only SELECT on a column is not sufficient. 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? -- 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: Mon, 14 May 2018 22:43:14 +0000 Gerrit-HasComments: Yes
