Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 )
Change subject: WIP IMPALA-8851 : Do not throw authorization exception in drop if exists queries ...................................................................... Patch Set 3: (8 comments) Mostly looks good. Have some questions and left some suggestions below. http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2654 PS3, Line 2654: public boolean tableExists(TableName tblName) { Missing java doc for the two new methods http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java File fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java@77 PS3, Line 77: dbName_ What happens when we register a privilege for a db which doesn't exist? The initial thought was that user needs to have ANY privilege on server to list databases http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java File fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java@86 PS3, Line 86: registerFnPriv Is this registering the priv twice if the function exists and if exists clause it not provided? http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java@97 PS3, Line 97: !hasSignature() && db != null && db.getFunctions( : desc_.functionName()).isEmpty() Can we make this section cleaner by creating a method which checks for existance of the function Eg. boolean fnExists = doesFunctionExist(db, Reference<String> errMsg); if (!fnExists && !ifExists_) { // fucntion does not exist and if exists is not provided // throw new AnalysisException(errMsg.get()); } else if(fnExists && ifExists) { registerFnPriv(analyzer, Privilege.DROP); } else if (!fnExists && ifExists) { registerFnPriv(analyzer, Privilege.ANY); } http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java@119 PS3, Line 119: void this method can be private. http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@116 PS3, Line 116: onTable(dbName_, getTbl()) What is the behavior when the table doesn't exist and a privilege request is registered on that object? http://gerrit.cloudera.org:8080/#/c/14121/3/tests/authorization/test_owner_privileges.py File tests/authorization/test_owner_privileges.py: http://gerrit.cloudera.org:8080/#/c/14121/3/tests/authorization/test_owner_privileges.py@149 PS3, Line 149: test_db is this argument needed? http://gerrit.cloudera.org:8080/#/c/14121/3/tests/authorization/test_owner_privileges.py@182 PS3, Line 182: _execute_drop_if_exists Do you think we need a additional test which exercises the drop if exists from a user which has the required permissions and one which was not a owner of the object previously? -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 3 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Mon, 26 Aug 2019 18:36:08 +0000 Gerrit-HasComments: Yes
