Fix memory leak in Iceberg remove orphan files procedure

Author: grantatspothero

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

@@ -149,6 +149,7 @@
 import org.apache.iceberg.ManifestFile;
 import org.apache.iceberg.ManifestFiles;
 import org.apache.iceberg.ManifestReader;
+import org.apache.iceberg.ManifestUtils;
 import org.apache.iceberg.MetadataColumns;
 import org.apache.iceberg.PartitionField;
 import org.apache.iceberg.PartitionSpec;
@@ -206,10 +207,12 @@
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.OptionalLong;
+import java.util.Queue;
 import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ConcurrentHashMap;
@@ -238,6 +241,7 @@
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.Iterables.getLast;
 import static com.google.common.collect.Iterables.getOnlyElement;
+import static com.google.common.collect.Lists.newArrayList;
 import static com.google.common.collect.Maps.transformValues;
 import static com.google.common.collect.Sets.difference;
 import static io.trino.filesystem.Locations.isS3Tables;
@@ -2251,12 +2255,13 @@ private void removeOrphanFiles(Table table, ConnectorSession session, SchemaTabl
         ImmutableSet.Builder<String> validMetadataFileNames = ImmutableSet.builder();
         ImmutableSet.Builder<String> validDataFileNames = ImmutableSet.builder();
 
-        for (Snapshot snapshot : table.snapshots()) {
-            if (snapshot.manifestListLocation() != null) {
-                validMetadataFileNames.add(fileName(snapshot.manifestListLocation()));
+        ArrayList<String> manifestListLocations = newArrayList(Iterables.transform(table.snapshots(), Snapshot::manifestListLocation));
+        for (String manifestListLocation : manifestListLocations) {
+            if (manifestListLocation != null) {
+                validMetadataFileNames.add(fileName(manifestListLocation));
             }
 
-            for (ManifestFile manifest : loadAllManifestsFromSnapshot(table, snapshot)) {
+            for (ManifestFile manifest : loadAllManifestsFromManifestList(table, manifestListLocation)) {
                 if (!processedManifestFilePaths.add(manifest.path())) {
                     // Already read this manifest
                     continue;
@@ -3460,6 +3465,21 @@ private static List<ManifestFile> loadAllManifestsFromSnapshot(Table icebergTabl
         }
     }
 
+    /**
+     * Use instead of loadAllManifestsFromSnapshot when loading manifests from multiple distinct snapshots
+     * Each BaseSnapshot object caches manifest files separately, so loading manifests from multiple distinct snapshots
+     * results in O(num_snapshots^2) copies of the same manifest file metadata in memory
+     */
+    private static List<ManifestFile> loadAllManifestsFromManifestList(Table icebergTable, String manifestListLocation)
+    {
+        try {
+            return ManifestUtils.read(icebergTable.io(), manifestListLocation);
+        }
+        catch (NotFoundException | UncheckedIOException e) {
+            throw new TrinoException(ICEBERG_INVALID_METADATA, "Error accessing manifest file for table %s".formatted(icebergTable.name()), e);
+        }
+    }
+
     private static Set<Integer> identityPartitionColumnsInAllSpecs(Table table)
     {
         // Extract identity partition column source ids common to ALL specs
@@ -3736,7 +3756,7 @@ public Optional<ConnectorOutputMetadata> finishRefreshMaterializedView(
                         icebergTable.schema().findType(field.sourceId())))
                 .toArray(Type[]::new);
 
-        AppendFiles appendFiles = transaction.newFastAppend();
+        AppendFiles appendFiles = isMergeManifestsOnWrite(session) ? transaction.newAppend() : transaction.newFastAppend();
         ImmutableSet.Builder<String> writtenFiles = ImmutableSet.builder();
         for (CommitTaskData task : commitTasks) {
             DataFiles.Builder builder = DataFiles.builder(icebergTable.spec())

plugin/trino-iceberg/src/main/java/org/apache/iceberg/ManifestUtils.java

@@ -0,0 +1,29 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.iceberg;
+
+import org.apache.iceberg.io.FileIO;
+
+import java.util.List;
+
+public class ManifestUtils
+{
+    private ManifestUtils() {}
+
+    public static List<ManifestFile> read(FileIO fileIO, String manifestListLocation)
+    {
+        // Avoid using snapshot.allManifests() which can easily leak memory
+        return ManifestLists.read(fileIO.newInputFile(manifestListLocation));
+    }
+}