Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22535 )
Change subject: IMPALA-13789: Defer creating Path objects in loading file metadata ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/22535/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java File fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java: http://gerrit.cloudera.org:8080/#/c/22535/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@a506 PS2, Line 506: > It looks like Path didn't automatically normalize the URL, so I think keyin By "keying off getLocation", do you mean passing in tbl.getSd() (StorageDescriptor) directly here? Note that the constructor of FileMetadataLoader has some callers that don't have this, e.g. in IcebergFileMetadataLoader https://github.com/apache/impala/blob/d682e226d5220e828ce473f7f9a7aa8898c20f32/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java#L95 http://gerrit.cloudera.org:8080/#/c/22535/2/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/22535/2/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java@184 PS2, Line 184: Path partPath = FileSystemUtil.createFullyQualifiedPath(new Path(partDir_)); > Did we call createFullyQualifiedPath somewhere else before? Looks like Para Yeah, we also used it in IcebergFileMetadataLoader, FeIcebergTable, HdfsTable, HdfsUri. Before this patch, the callers create the Path objects using this. Now we do it in FileMetadataLoader itself. http://gerrit.cloudera.org:8080/#/c/22535/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/22535/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@384 PS2, Line 384: // by this code path. However, DirectMetaProvider is not yet a supported feature. > Do we still need this TODO? Removed this. The MetaException is replaced with CatalogException in https://gerrit.cloudera.org/c/16008/21/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java#322 And there is a code path that does throw CatalogExceptions: FileMetadataLoader.load() -> loadInternal() -> AcidUtils.filterFilesForAcidState() https://github.com/apache/impala/blob/d682e226d5220e828ce473f7f9a7aa8898c20f32/fe/src/main/java/org/apache/impala/util/AcidUtils.java#L533-L537 -- To view, visit http://gerrit.cloudera.org:8080/22535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ec1fc932eaf7c833ef6ee6cdb08bba235e38271 Gerrit-Change-Number: 22535 Gerrit-PatchSet: 3 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 26 Feb 2025 00:11:59 +0000 Gerrit-HasComments: Yes
