dramaticlly commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2755883017
##########
core/src/test/java/org/apache/iceberg/TestDeleteFiles.java:
##########
@@ -351,6 +351,62 @@ public void testDeleteFilesOnIndependentBranches() {
statuses(Status.EXISTING, Status.DELETED, Status.DELETED));
}
+ @TestTemplate
+ public void testDeleteFilesWithManifestSnapshotSummary() {
Review Comment:
removed `testDeleteFilesWithManifestSnapshotSummary` as well, as covered by
- testMultipleDeletes
- removingDataFileByPathAlsoRemovesDV
##########
core/src/test/java/org/apache/iceberg/TestCommitReporting.java:
##########
@@ -195,4 +213,68 @@ public void addAndDeleteManifests() {
assertThat(metrics.manifestsReplaced().value()).isEqualTo(2L);
assertThat(metrics.manifestEntriesProcessed().value()).isEqualTo(2L);
}
+
+ @TestTemplate
+ public void manifestMetricsForVariousOperations() {
Review Comment:
I believe existing tests already covered for append/delete/rowDelta, thus
remove this `manifestMetricsForVariousOperations`
##########
core/src/main/java/org/apache/iceberg/ManifestMergeManager.java:
##########
@@ -96,8 +111,13 @@ void cleanUncommitted(Set<ManifestFile> committed) {
ManifestFile merged = entry.getValue();
if (!committed.contains(merged)) {
deleteFile(merged.path());
- // remove the deleted file from the cache
- mergedManifests.remove(entry.getKey());
+ List<ManifestFile> bin = entry.getKey();
+ mergedManifests.remove(bin);
+ for (ManifestFile m : bin) {
+ if (snapshotId() != m.snapshotId()) {
Review Comment:
For multi transaction commit, each change will produce a new snapshot with
corresponding snapshot summary. We only stage the TableMetadata in memory, but
the snapshot workflow is exactly the same as non-transaction commit. I also
added a unit test in TestTransaction to cover this scenario.
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -645,6 +645,34 @@ protected boolean cleanupAfterCommit() {
return true;
}
+ /**
+ * Builds a snapshot summary with manifest counts.
+ *
+ * @param manifests the list of manifests in the new snapshot
+ * @param replacedManifestsCount the count of manifests that were replaced
(rewritten)
+ * @return a summary builder with manifest count metrics set
+ */
+ protected SnapshotSummary.Builder buildManifestCountSummary(
+ List<ManifestFile> manifests, int replacedManifestsCount) {
+ SnapshotSummary.Builder summaryBuilder = SnapshotSummary.builder();
+ int manifestsCreated = 0;
+ int manifestsKept = 0;
+
+ for (ManifestFile manifest : manifests) {
+ if (snapshotId() == manifest.snapshotId()) {
Review Comment:
For appending external manifests with null snapshotId, I see we will either
rewrite if (v2+ or enable SNAPSHOT_ID_INHERITANCE_ENABLED ), those are covered
by TestFastAppend::testAppendManifestWithSnapshotIdInheritance as well as
TestMergeAppend::testAppendManifestWithSnapshotIdInheritance.
- MergingSnapshotProducer:
https://github.com/apache/iceberg/blob/c4b5bc4723b35f5005c019ce5adeb7963b7c46e6/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java#L1036
- fastAppend:
https://github.com/apache/iceberg/blob/c4b5bc4723b35f5005c019ce5adeb7963b7c46e6/core/src/main/java/org/apache/iceberg/FastAppend.java#L162
When reading from manifest in InheritableMetadataFactory, we also mandate
the snapshot id to be set in
https://github.com/apache/iceberg/blob/f25e07db6318184272d5c98dede4d268ee5288ab/core/src/main/java/org/apache/iceberg/InheritableMetadataFactory.java#L36
--
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]
