Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20271 )
Change subject: IMPALA-12298: Improve incremental load of Iceberg tables ...................................................................... Patch Set 4: (6 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/20271/3/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/20271/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@61 PS3, Line 61: // Default value of 'newFilesThreshold_' if the given parameter or startup flag have > This variable is not meant to change so I guess can be final as well. Made it final and updated the tests. http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@77 PS3, Line 77: icebergFiles, canDataBeOutsideOfTableLocation, > This is only used for testing, right? Would it be possible to find a way no Removed this. http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@99 PS3, Line 99: @Override > nit: I think this string could go directly into the LOG.trace() call withou We also pass it to ThreadNameAnnotator at L102. http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@215 PS3, Line 215: // 'org.apache.impala.catalog.IcebergFileMetadataLoader.createFileStatus'. > Similarly to the setter above could this be private instead while somehow s I made it package-private and annotated with VisibleForTesting. http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java: http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@983 PS3, Line 983: return null; > merge with line above Done http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/test/java/org/apache/impala/catalog/FileMetadataLoaderTest.java File fe/src/test/java/org/apache/impala/catalog/FileMetadataLoaderTest.java: http://gerrit.cloudera.org:8080/#/c/20271/3/fe/src/test/java/org/apache/impala/catalog/FileMetadataLoaderTest.java@279 PS3, Line 279: partitioned", > nit: this name seems odd Switched to 'canDataBeOutsideOfTableLocation' which is the name we use in IcebergFileMetadataLoader. -- To view, visit http://gerrit.cloudera.org:8080/20271 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf643798a93e74ae7b0f37ceeab0a8052fb2699d Gerrit-Change-Number: 20271 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 31 Jul 2023 15:33:58 +0000 Gerrit-HasComments: Yes
