Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15123 )
Change subject: IMPALA-7002: Throw AuthorizationException when user accessing non-existent table/database in CTE without any privilege. ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/15123/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15123/2//COMMIT_MSG@10 PS2, Line 10: Register all privilege requests made from the withClause analyzer : when analyze function throw AnalysisException. nit: can you instead briefly explain what it fixed http://gerrit.cloudera.org:8080/#/c/15123/2//COMMIT_MSG@19 PS2, Line 19: pre-review-test on Jenkins, including FE tests, BE tests, : EE tests, JDBS test and cluster tests. nit: passed all core tests http://gerrit.cloudera.org:8080/#/c/15123/2/fe/src/main/java/org/apache/impala/analysis/WithClause.java File fe/src/main/java/org/apache/impala/analysis/WithClause.java: http://gerrit.cloudera.org:8080/#/c/15123/2/fe/src/main/java/org/apache/impala/analysis/WithClause.java@82 PS2, Line 82: try { : view.getQueryStmt().analyze(viewAnalyzer); : } catch (AnalysisException e) { : // Register all privilege requests made from the root analyzer. : for (PrivilegeRequest req: withClauseAnalyzer.getPrivilegeReqs()) { : analyzer.registerPrivReq(req); : } : throw e; Its not quite clear why we only have to repeat this step if an exceptions is encountered. Can add a small comment here and/or refactor to the code so that privilege requests are made regardless of exception (maybe in a final block) http://gerrit.cloudera.org:8080/#/c/15123/2/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/15123/2/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@597 PS2, Line 597: // Select from non-existent database with "With" clause. : authorize("with t as (select id from nodb.alltypes) select * from t") : .error(selectError("nodb.alltypes")); : : // Select from non-existent table with "With" clause. : authorize("with t as (select id from functional.notbl) select * from t") : .error(selectError("functional.notbl")); do we have the same test but with privileges?? -- To view, visit http://gerrit.cloudera.org:8080/15123 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia6b657a7147a136198a9a97a679c9131ee814577 Gerrit-Change-Number: 15123 Gerrit-PatchSet: 2 Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Fri, 31 Jan 2020 00:47:54 +0000 Gerrit-HasComments: Yes
