Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4847: Simplify HdfsTable block metadata loading code

Patch Set 1:

File fe/src/main/java/org/apache/impala/catalog/

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 

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 =;
             :       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 = 
             :           partitions.get(0).getFileFormat(), hostIndex_);
             :       // Update the partitions' metadata that this file belongs 
             :       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 = 
> It looks like this is only used in L590. Maybe move it closer to that.

PS1, Line 695: .keys()
> remove

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I963d647bd2ba11e3843c6ef2ac6be113c74280bf
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-HasComments: Yes

Reply via email to