HIVE-11995 : Remove repetitively setting permissions in insert/load overwrite partition (Chaoyu Tang via Szehon)
Project: http://git-wip-us.apache.org/repos/asf/hive/repo Commit: http://git-wip-us.apache.org/repos/asf/hive/commit/50744231 Tree: http://git-wip-us.apache.org/repos/asf/hive/tree/50744231 Diff: http://git-wip-us.apache.org/repos/asf/hive/diff/50744231 Branch: refs/heads/llap Commit: 507442319985198466b4f6c2ba18c6b068d8435e Parents: 467a117 Author: Szehon Ho <sze...@cloudera.com> Authored: Thu Oct 1 15:29:36 2015 -0700 Committer: Szehon Ho <sze...@cloudera.com> Committed: Thu Oct 1 15:29:36 2015 -0700 ---------------------------------------------------------------------- .../hive/ql/security/FolderPermissionBase.java | 53 +++++++-- .../apache/hadoop/hive/ql/metadata/Hive.java | 108 ++++--------------- 2 files changed, 65 insertions(+), 96 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hive/blob/50744231/itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/FolderPermissionBase.java ---------------------------------------------------------------------- diff --git a/itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/FolderPermissionBase.java b/itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/FolderPermissionBase.java index d98082f..d7149a7 100644 --- a/itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/FolderPermissionBase.java +++ b/itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/FolderPermissionBase.java @@ -261,6 +261,7 @@ public abstract class FolderPermissionBase { //insert overwrite test setPermission(warehouseDir + "/" + tableName, 1); + setPermission(warehouseDir + "/" + tableName + "/part1=1", 1); ret = driver.run("insert overwrite table " + tableName + " partition(part1='1') select key,value from mysrc where part1='1' and part2='1'"); Assert.assertEquals(0, ret.getResponseCode()); @@ -297,6 +298,9 @@ public abstract class FolderPermissionBase { //insert overwrite test setPermission(warehouseDir + "/" + tableName, 1); + setPermission(warehouseDir + "/" + tableName + "/part1=1", 1); + setPermission(warehouseDir + "/" + tableName + "/part1=1/part2=1", 1); + ret = driver.run("insert overwrite table " + tableName + " partition(part1='1', part2='1') select key,value from mysrc where part1='1' and part2='1'"); Assert.assertEquals(0, ret.getResponseCode()); @@ -325,8 +329,9 @@ public abstract class FolderPermissionBase { verifyDualPartitionTable(warehouseDir + "/" + tableName, 0); - //Insert overwrite test, with permission set 1. - setPermission(warehouseDir + "/" + tableName, 1); + //Insert overwrite test, with permission set 1. We need reset existing partitions to 1 since the permissions + //should be inherited from existing partition + setDualPartitionTable(warehouseDir + "/" + tableName, 1); ret = driver.run("insert overwrite table " + tableName + " partition (part1,part2) select key,value,part1,part2 from mysrc"); Assert.assertEquals(0, ret.getResponseCode()); @@ -348,8 +353,9 @@ public abstract class FolderPermissionBase { Assert.assertEquals(0,ret.getResponseCode()); verifySinglePartition(tableLoc, 0); - //Insert overwrite test, with permission set 1. - setPermission(tableLoc, 1); + //Insert overwrite test, with permission set 1. We need reset existing partitions to 1 since the permissions + //should be inherited from existing partition + setSinglePartition(tableLoc, 1); ret = driver.run("insert overwrite table " + tableName + " partition (part1) select key,value,part1 from mysrc"); Assert.assertEquals(0,ret.getResponseCode()); verifySinglePartition(tableLoc, 1); @@ -458,6 +464,9 @@ public abstract class FolderPermissionBase { //case1B: load data local into overwrite non-partitioned-table setPermission(warehouseDir + "/" + tableName, 1); + for (String child : listStatus(tableLoc)) { + setPermission(child, 1); + } ret = driver.run("load data local inpath '" + dataFilePath + "' overwrite into table " + tableName); Assert.assertEquals(0,ret.getResponseCode()); @@ -485,8 +494,13 @@ public abstract class FolderPermissionBase { verifyPermission(child); } - //case 2B: insert data overwrite into non-partitioned table. + //case 2B: insert data overwrite into partitioned table. set testing table/partition folder hierarchy 1 + //local load overwrite just overwrite the existing partition content but not the permission setPermission(tableLoc, 1); + setPermission(partLoc, 1); + for (String child : listStatus(partLoc)) { + setPermission(child, 1); + } ret = driver.run("LOAD DATA LOCAL INPATH '" + dataFilePath + "' OVERWRITE INTO TABLE " + tableName + " PARTITION (part1='1',part2='1')"); Assert.assertEquals(0,ret.getResponseCode()); @@ -521,6 +535,10 @@ public abstract class FolderPermissionBase { //case1B: load data into overwrite non-partitioned-table setPermission(warehouseDir + "/" + tableName, 1); + for (String child : listStatus(tableLoc)) { + setPermission(child, 1); + } + fs.copyFromLocalFile(dataFilePath, new Path(location)); ret = driver.run("load data inpath '" + location + "' overwrite into table " + tableName); Assert.assertEquals(0,ret.getResponseCode()); @@ -550,8 +568,15 @@ public abstract class FolderPermissionBase { verifyPermission(child); } - //case 2B: insert data overwrite into non-partitioned table. + //case 2B: insert data overwrite into partitioned table. set testing table/partition folder hierarchy 1 + //load overwrite just overwrite the existing partition content but not the permission setPermission(tableLoc, 1); + setPermission(partLoc, 1); + Assert.assertTrue(listStatus(partLoc).size() > 0); + for (String child : listStatus(partLoc)) { + setPermission(child, 1); + } + fs.copyFromLocalFile(dataFilePath, new Path(location)); ret = driver.run("LOAD DATA INPATH '" + location + "' OVERWRITE INTO TABLE " + tableName + " PARTITION (part1='1',part2='1')"); Assert.assertEquals(0,ret.getResponseCode()); @@ -693,7 +718,12 @@ public abstract class FolderPermissionBase { assertExistence(partition); verifyPermission(partition); } - + + private void setSinglePartition(String tableLoc, int index) throws Exception { + setPermission(tableLoc + "/part1=1", index); + setPermission(tableLoc + "/part1=2", index); + } + private void verifySinglePartition(String tableLoc, int index) throws Exception { verifyPermission(tableLoc + "/part1=1", index); verifyPermission(tableLoc + "/part1=2", index); @@ -709,6 +739,15 @@ public abstract class FolderPermissionBase { } } + private void setDualPartitionTable(String baseTablePath, int index) throws Exception { + setPermission(baseTablePath, index); + setPermission(baseTablePath + "/part1=1", index); + setPermission(baseTablePath + "/part1=1/part2=1", index); + + setPermission(baseTablePath + "/part1=2", index); + setPermission(baseTablePath + "/part1=2/part2=2", index); + } + private void verifyDualPartitionTable(String baseTablePath, int index) throws Exception { verifyPermission(baseTablePath, index); verifyPermission(baseTablePath + "/part1=1", index); http://git-wip-us.apache.org/repos/asf/hive/blob/50744231/ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java ---------------------------------------------------------------------- diff --git a/ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java b/ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java index 10cafb6..8efbb05 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java @@ -2932,8 +2932,7 @@ private void constructOneLBLocationMap(FileStatus fSta, LOG.info("No sources specified to move: " + srcf); return; } - List<List<Path[]>> result = checkPaths(conf, destFs, srcs, srcFs, destf, - true); + List<List<Path[]>> result = checkPaths(conf, destFs, srcs, srcFs, destf, true); if (oldPath != null) { try { @@ -2945,9 +2944,6 @@ private void constructOneLBLocationMap(FileStatus fSta, if (FileUtils.isSubDir(oldPath, destf, fs2)) { FileUtils.trashFilesUnderDir(fs2, oldPath, conf); } - if (inheritPerms) { - inheritFromTable(tablePath, destf, conf, destFs); - } } } catch (Exception e) { //swallow the exception @@ -2955,58 +2951,24 @@ private void constructOneLBLocationMap(FileStatus fSta, } } - // rename src directory to destf - if (srcs.length == 1 && srcs[0].isDir()) { - // rename can fail if the parent doesn't exist - Path destfp = destf.getParent(); - if (!destFs.exists(destfp)) { - boolean success = destFs.mkdirs(destfp); - if (!success) { - LOG.warn("Error creating directory " + destf.toString()); - } - if (inheritPerms && success) { - inheritFromTable(tablePath, destfp, conf, destFs); - } - } - - // Copy/move each file under the source directory to avoid to delete the destination - // directory if it is the root of an HDFS encryption zone. - for (List<Path[]> sdpairs : result) { - for (Path[] sdpair : sdpairs) { - Path destParent = sdpair[1].getParent(); - FileSystem destParentFs = destParent.getFileSystem(conf); - if (!destParentFs.isDirectory(destParent)) { - boolean success = destFs.mkdirs(destParent); - if (!success) { - LOG.warn("Error creating directory " + destParent); - } - if (inheritPerms && success) { - inheritFromTable(tablePath, destParent, conf, destFs); - } - } - if (!moveFile(conf, sdpair[0], sdpair[1], true, isSrcLocal)) { - throw new IOException("Unable to move file/directory from " + sdpair[0] + - " to " + sdpair[1]); - } - } - } - } else { // srcf is a file or pattern containing wildcards - if (!destFs.exists(destf)) { - boolean success = destFs.mkdirs(destf); - if (!success) { - LOG.warn("Error creating directory " + destf.toString()); - } - if (inheritPerms && success) { - inheritFromTable(tablePath, destf, conf, destFs); - } - } - // srcs must be a list of files -- ensured by LoadSemanticAnalyzer - for (List<Path[]> sdpairs : result) { - for (Path[] sdpair : sdpairs) { - if (!moveFile(conf, sdpair[0], sdpair[1], true, - isSrcLocal)) { - throw new IOException("Error moving: " + sdpair[0] + " into: " + sdpair[1]); - } + // first call FileUtils.mkdir to make sure that destf directory exists, if not, it creates + // destf with inherited permissions + boolean destfExist = FileUtils.mkdir(destFs, destf, true, conf); + if(!destfExist) { + throw new IOException("Directory " + destf.toString() + + " does not exist and could not be created."); + } + + // Two cases: + // 1. srcs has only a src directory, if rename src directory to destf, we also need to + // Copy/move each file under the source directory to avoid to delete the destination + // directory if it is the root of an HDFS encryption zone. + // 2. srcs must be a list of files -- ensured by LoadSemanticAnalyzer + // in both cases, we move the file under destf + for (List<Path[]> sdpairs : result) { + for (Path[] sdpair : sdpairs) { + if (!moveFile(conf, sdpair[0], sdpair[1], true, isSrcLocal)) { + throw new IOException("Error moving: " + sdpair[0] + " into: " + sdpair[1]); } } } @@ -3015,38 +2977,6 @@ private void constructOneLBLocationMap(FileStatus fSta, } } - /** - * This method sets all paths from tablePath to destf (including destf) to have same permission as tablePath. - * @param tablePath path of table - * @param destf path of table-subdir. - * @param conf - * @param fs - */ - private static void inheritFromTable(Path tablePath, Path destf, HiveConf conf, FileSystem fs) { - if (!FileUtils.isSubDir(destf, tablePath, fs)) { - //partition may not be under the parent. - return; - } - HadoopShims shims = ShimLoader.getHadoopShims(); - //Calculate all the paths from the table dir, to destf - //At end of this loop, currPath is table dir, and pathsToSet contain list of all those paths. - Path currPath = destf; - List<Path> pathsToSet = new LinkedList<Path>(); - while (!currPath.equals(tablePath)) { - pathsToSet.add(currPath); - currPath = currPath.getParent(); - } - - try { - HadoopShims.HdfsFileStatus fullFileStatus = shims.getFullFileStatus(conf, fs, currPath); - for (Path pathToSet : pathsToSet) { - shims.setFullFileStatus(conf, fullFileStatus, fs, pathToSet); - } - } catch (Exception e) { - LOG.warn("Error setting permissions or group of " + destf, e); - } - } - public static boolean isHadoop1() { return ShimLoader.getMajorVersion().startsWith("0.20"); }