[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/19379 )
Change subject: IMPALA-11662: Improve 'refresh iceberg_tbl_on_oss' performance ...................................................................... Patch Set 6: (6 comments) Thanks for comments! http://gerrit.cloudera.org:8080/#/c/19379/5/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java File fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/19379/5/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java@212 PS5, Line 212: edFds_.add > I think fd cannot be null here, see my comment in IcebergFileMetadataLoader Done http://gerrit.cloudera.org:8080/#/c/19379/5/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java File fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/19379/5/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@17 PS5, Line 17: > nit: please add empty line above this Done http://gerrit.cloudera.org:8080/#/c/19379/5/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@57 PS5, Line 57: IcebergFileMetadataLoader.class); > nit: please add comment why we need this Done http://gerrit.cloudera.org:8080/#/c/19379/5/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@89 PS5, Line 89: if (relUri.isAbsolute() || relUri.getPath().startsWith(Path.SEPARATOR)) { : if (canDataOutsideOfTableLocation_) { : absPath = fileStatus.getPath().toString(); : } else { > skippedFiles means the following based on its comment: "Number of files ski Yes, we should! http://gerrit.cloudera.org:8080/#/c/19379/5/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@124 PS5, Line 124: > This comment mentions "Avoid the cost of directory listing", then there is Done http://gerrit.cloudera.org:8080/#/c/19379/5/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java File fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/19379/5/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java@113 PS5, Line 113: if (format.equals(HdfsFileFormat.ICEBERG)) { > I see, sorry, I should have taken a closer look at IcebergFileMetadataLoade Done -- To view, visit http://gerrit.cloudera.org:8080/19379 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If2ee8b6b7559e6590698b46ef1d574e55ed52f9a Gerrit-Change-Number: 19379 Gerrit-PatchSet: 6 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Gergely Fürnstáhl <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 06 Jan 2023 06:24:14 +0000 Gerrit-HasComments: Yes
