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 6: (2 comments) 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: import static org.junit.Assert.fail; > Done. Avoiding calling catalog_.reset() and making Sentry RPCs makes the wh That's more like it! Fantastic. http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349 PS5, Line 349: "functional.alltypes", onTable("functional", "alltypes", allExcept( > Shouldn't this be a separate test case since this is to test a table with a In my mind the auth tests should check whether the authorization rules are applied correctly. A table reference with implicit or explicit aliases still refers to the same underlying table, so it's a variant of the same test, but not really a different auth test. Ideally, we should separate testing different SQL variants of referencing the same underlying entity from the auth tests themselves. For exampe, we could have a single test that asserts the following things generate the same PrivilegeRequests during analysis. - table reference with implicit/explicit aliases - qualified/unqualified table references - qualified/unqualified column references - qualified/unqualified star Then when we do the auth tests, we can focus on whatever variant is most convenient. -- 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: 6 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: Fri, 27 Apr 2018 16:51:01 +0000 Gerrit-HasComments: Yes
