Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-4847: Simplify HdfsTable block metadata loading code ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/7652/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: PS1, Line 252: for all partitions in 'partitions' > nit: "of all the 'partitions' that ..." Done Line 258: * and disk IDs. > Expand this to mention that we synthesize block metadata for FS that don't Done PS1, Line 331: if (partitions == null || partitions.isEmpty()) return; : RemoteIterator<LocatedFileStatus> fileStatusIter = : FileSystemUtil.listFiles(fs, partDir, false); : if (fileStatusIter == null) return; : while (fileStatusIter.hasNext()) { : LocatedFileStatus fileStatus = fileStatusIter.next(); : if (!FileSystemUtil.isValidDataFile(fileStatus)) continue; : // For the purpose of synthesizing block metadata, we assume that all partitions : // with the same location have the same file format. : FileDescriptor fd = FileDescriptor.createWithSynthesizedBlockMd(fileStatus, : partitions.get(0).getFileFormat(), hostIndex_); : // Update the partitions' metadata that this file belongs to. : for (HdfsPartition partition: partitions) { : partition.getFileDescriptors().add(fd); : } : } > With the exception of L340, this function resembles loadBlockMetadata. Mayb Did some refactoring. No more synthesize* methods in HdfsTable now. LMK if it looks ok. PS1, Line 581: Path tblLocation = FileSystemUtil.createFullyQualifiedPath(getHdfsBaseDirPath()); > It looks like this is only used in L590. Maybe move it closer to that. Done PS1, Line 695: .keys() > remove Done -- To view, visit http://gerrit.cloudera.org:8080/7652 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I963d647bd2ba11e3843c6ef2ac6be113c74280bf Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
