Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests ...................................................................... Patch Set 5: (8 comments) Overall the new mechanism seems fine to me. Main comments: * test execution speed needs to be improved * still a lot of redundancy in the tests http://gerrit.cloudera.org:8080/#/c/10135/5/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/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@47 PS5, Line 47: public class AuthorizationTestV2 extends FrontendTestBase { I tried running the test and it feels very slow. Do you know what's making it slow? Might be good to address the low hanging perf fruit if we can. At this execution speed this new test might ultimately take tens of minutes if we add coverage for the other statements. http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@75 PS5, Line 75: public void before() throws ImpalaException { Why do we need this? http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@122 PS5, Line 122: .ok(onColumn("functional", "alltypes", "id", TPrivilegeLevel.ALL)) What does this mean? My understanding is that columns can only have SELECT, so this should be illegal. http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@157 PS5, Line 157: allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT))); add comment that column-level privs are currently not supported http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@183 PS5, Line 183: authorize(createAnalysisCtx("functional"), "select id from alltypes") I feel like there's still a lot of redundancy in these tests, could we perhaps eliminate some of it? These ~20 lines are basically the same test as the ~20 lines in 115. http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@324 PS5, Line 324: authorize("select count(id) from functional.alltypes") also very similar to the other test blocks, can we condense them further? http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349 PS5, Line 349: authorize("select t.id from functional.alltypes t") condense further? http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@451 PS5, Line 451: authorize("select * from functional.alltypes a cross join " + I feel like these tests could be structured a little more. Not sure if you are following the existing tests or not. We seem to have a few dimensions: view vs. table, qualified vs. unqualified column/table reference. Then we have tests for specific clauses in a SELECT like the FROM clause, GROUP BY / HAVING clauses, etc. -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 5 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: Wed, 25 Apr 2018 22:51:14 +0000 Gerrit-HasComments: Yes
