Dimitris Tsirogiannis 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 ..." Line 258: * and disk IDs. Expand this to mention that we synthesize block metadata for FS that don't support disk ids. Also, mention (maybe in the beginning) that 'pathDir' could be in HDFS, S3 and ADLS. 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. Maybe see if there is a nice way to merge these two. PS1, Line 581: Path tblLocation = FileSystemUtil.createFullyQualifiedPath(getHdfsBaseDirPath()); It looks like this is only used in L590. Maybe move it closer to that. PS1, Line 695: .keys() remove -- 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 <bhara...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-HasComments: Yes
