Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/22458 )
Change subject: IMPALA-13737: Directly load file metadata via IcebergFileMetadataLoader ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@246 PS2, Line 246: Whether to skip file metadata loading. (only used when HdfsTable is part of : // an Iceberg table > Could you explain what determines whether this flag is set to true or false If this property is determined by whether it is an Iceberg table, we could create a new constructor that could take it as a parameter, and then make it final and remove the setter. This could eliminate the time where the HdfsTable is in an invalid state, i.e. after the constructor and before calling setSkipFileMetadataLoading(). http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@248 PS2, Line 248: FileMetada If it's only used for Iceberg tables, it could be called 'skipIcebergFileMetadataLoading', which would describe it more clearly. http://gerrit.cloudera.org:8080/#/c/22458/2/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/22458/2/fe/src/main/java/org/apache/impala/catalog/ParallelFileMetadataLoader.java@102 PS2, Line 102: loader = new FileMetadataLoader(e.getKey(), isRecursive, oldFds, hostIndex, If it is not used for Iceberg tables, we could add a DCHECK that the format is not HdfsFileFormat.ICEBERG. http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java File fe/src/main/java/org/apache/impala/util/IcebergUtil.java: http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1043 PS2, Line 1043: d Unrelated nit: "cf". http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1126 PS2, Line 1126: partValueString = MetaStoreUtil.DEFAULT_NULL_PARTITION_KEY_VALUE; Couldn't the table have a different null partition key value set, is it always the default? -- To view, visit http://gerrit.cloudera.org:8080/22458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf7e23ec21b65036b47edadcb4cbe4b64be3baee Gerrit-Change-Number: 22458 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Comment-Date: Tue, 11 Feb 2025 12:22:59 +0000 Gerrit-HasComments: Yes
