Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-2836: Change permissions for ADD/DROP PARTITION ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/3327/3//COMMIT_MSG Commit Message: PS3, Line 14: ALTER TABLE ADD PARTITION SET LOCATION 'hdfs_path_of_directory' There is ALTER TABLE ADD PARTITION LOCATION and ALTER TABLE (PARTITION SPEC) SET LOCATION .... Which one do you mean? http://gerrit.cloudera.org:8080/#/c/3327/3/fe/src/main/java/com/cloudera/impala/analysis/AlterTableAddPartitionStmt.java File fe/src/main/java/com/cloudera/impala/analysis/AlterTableAddPartitionStmt.java: PS3, Line 84: if (location_ != null) { : partitionSpec_.setPrivilegeRequirement(Privilege.ALTER); : location_.analyze(analyzer, Privilege.ALL, FsAction.READ_WRITE); : } else { : partitionSpec_.setPrivilegeRequirement(Privilege.INSERT); : } This is confusing to me. Why does the partition spec get a different privilege request depending on the existence of location? http://gerrit.cloudera.org:8080/#/c/3327/3/fe/src/main/java/com/cloudera/impala/analysis/AlterTableDropPartitionStmt.java File fe/src/main/java/com/cloudera/impala/analysis/AlterTableDropPartitionStmt.java: PS3, Line 73: INSERT Please correct me if I am wrong, but I thought the DROP will still require full permissions, no? http://gerrit.cloudera.org:8080/#/c/3327/3/fe/src/main/java/com/cloudera/impala/analysis/AlterTableStmt.java File fe/src/main/java/com/cloudera/impala/analysis/AlterTableStmt.java: PS3, Line 72: analyzeUsingPrivilege Let's keep the name "analyze" for the function. Also, you may want to add a function comment here and in the function above. -- To view, visit http://gerrit.cloudera.org:8080/3327 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6d971f0ab46fed87b0428d344d002a42644b9847 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
