Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21847 )
Change subject: IMPALA-11265: Part1: Clear GroupContentFiles once used ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/21847/1/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java: http://gerrit.cloudera.org:8080/#/c/21847/1/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@427 PS1, Line 427: // We no longer need to keep Iceberg's content files in memory. : icebergFiles.clear(); > Alternatively we could unset icebergFiles_ in hdfsTable_, so GC can collect Unsetting 'icebergFiles_' seems better to me because having an empty 'icebergFiles_' could be mistaken for a situation when there are actually no Iceberg files (e.g. empty table). http://gerrit.cloudera.org:8080/#/c/21847/1/fe/src/main/java/org/apache/impala/catalog/iceberg/GroupedContentFiles.java File fe/src/main/java/org/apache/impala/catalog/iceberg/GroupedContentFiles.java: http://gerrit.cloudera.org:8080/#/c/21847/1/fe/src/main/java/org/apache/impala/catalog/iceberg/GroupedContentFiles.java@92 PS1, Line 92: to null Isn't it that it would delete the contents of the collections but would not set them to null. Instead of creating new lists and sets, wouldn't it be better to set them to null, i.e. "dataFilesWithoutDeletes = null" etc.? -- To view, visit http://gerrit.cloudera.org:8080/21847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1efdd2a46c9675f7461535259e5892ed213a6b21 Gerrit-Change-Number: 21847 Gerrit-PatchSet: 1 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 24 Sep 2024 15:21:17 +0000 Gerrit-HasComments: Yes
