smaheshwar-pltr commented on code in PR #16796:
URL: https://github.com/apache/iceberg/pull/16796#discussion_r3591675818
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -597,12 +598,18 @@ public static void deleteRemovedMetadataFiles(
// the log, thus we don't include metadata.previousFiles() for deletion
- everything else can
// be removed
removedPreviousMetadataFiles.removeAll(metadata.previousFiles());
- deleteFiles(
- io,
+
+ Set<String> metadataFilesToDelete =
removedPreviousMetadataFiles.stream()
.map(TableMetadata.MetadataLogEntry::file)
- .collect(Collectors.toSet()),
- "metadata");
+ .collect(Collectors.toCollection(Sets::newHashSet));
+ // delete base's metadata file too if log is empty
+ if (metadata.previousFiles().isEmpty()
Review Comment:
Thanks for bringing this up.
IMHO, `deleteRemovedMetadataFiles` as a public utility should be using the
logs to determine what to delete as opposed to a table property.
I'd even suggest that `previousFiles().isEmpty()` is the more accurate
condition. It indicates that the superseded file itself is no longer retained
by the log, so it should be cleaned up; `previous-versions-max` being 0 is more
so the means of arriving at an empty log.
Referencing this property for reader context makes a lot of sense to me
though - I've pushed
https://github.com/apache/iceberg/pull/16796/commits/74acbbea7687ea6a1311a44656bc652d1d715cd4
to address that with a comment.
Please let me know what you think!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
