You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dramaticlly (via GitHub)" <gi...@apache.org> on 2026/01/08 23:28:00 UTC
[PR] Core: populate manifest created/kept count when commit a snapshot [iceberg]
dramaticlly opened a new pull request, #15003:
URL: https://github.com/apache/iceberg/pull/15003
Currently iceberg commit does not expose the manifest level information, so it's difficult to see how metadata will evolve with given snapshot.
I am reuse the `manifests-created` and `manifests-replaced` snapshot summary, as it already wired to the commit report.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#issuecomment-3916365392
Thanks @dramaticlly for the PR! LGTM from me. Let's wait a couple more days for any additional reviews before merging.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2750214616
##########
core/src/test/java/org/apache/iceberg/TestCommitReporting.java:
##########
@@ -195,4 +195,68 @@ public void addAndDeleteManifests() {
assertThat(metrics.manifestsReplaced().value()).isEqualTo(2L);
assertThat(metrics.manifestEntriesProcessed().value()).isEqualTo(2L);
}
+
+ @TestTemplate
+ public void snapshotProducerManifestMetrics() {
Review Comment:
Instead of adding a new method that cover multiple scenarios, should the new assertions just be added to exiting testing methods?
##########
core/src/main/java/org/apache/iceberg/ManifestMergeManager.java:
##########
@@ -200,8 +217,9 @@ private ManifestFile createManifest(int specId, List<ManifestFile> bin) {
ManifestFile manifest = writer.toManifestFile();
- // update the cache
+ // update the cache and track replaced manifests
mergedManifests.put(bin, manifest);
+ replacedManifestsCount.addAndGet(bin.size());
Review Comment:
This is buried deep in the code path to update the replaced counter, which is less clear about the intention. A better place might be inside the for loop (line 86)
```
for (Integer specId : groups.keySet()) {
List<ManifestFile> toBeMerged = groups.get(specId);
replacedManifestsCount.addAndGet(toBeMerged);
Iterables.addAll(merged, mergeGroup(first, specId, toBeMerged));
}
```
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -645,6 +645,35 @@ protected boolean cleanupAfterCommit() {
return true;
}
+ /**
Review Comment:
looks like it doesn't populate the kept manfiests count? maybe we can fix it here.
##########
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:
should the new assertions be added to existing test methods like the `TestFastAppend` change?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2752486001
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -645,6 +645,35 @@ protected boolean cleanupAfterCommit() {
return true;
}
+ /**
Review Comment:
I guess it's a bit difficult to read with auto formater, but LINE 103 of https://github.com/apache/iceberg/blob/026ec35b7969539b535af03944fae53d059cd233/core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java#L103 does populate the KEPT_MANIFESTS_COUNT and it's taken cared of.
##########
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:
I added assertions to existing tests which cares about manifest, also for the TestRowDelta
##########
core/src/main/java/org/apache/iceberg/ManifestMergeManager.java:
##########
@@ -200,8 +217,9 @@ private ManifestFile createManifest(int specId, List<ManifestFile> bin) {
ManifestFile manifest = writer.toManifestFile();
- // update the cache
+ // update the cache and track replaced manifests
mergedManifests.put(bin, manifest);
+ replacedManifestsCount.addAndGet(bin.size());
Review Comment:
I think this piece of code is a bit tricky but we would actually overcount the replaced manifest if this count is moved in line 86.
Let me share a bit more, not all manifests in a group are actually merged for `toBeMerged`. Say we have a bin with size =1, of any bin with size < minCountToMerge (default to 100), those manifest will be kept as-is and we expect the manifest replaced count = 0.
The manifest will only be merged when criteria met, and that's the reason why replaced count is tracked inside `createManifest` to handle cache hits on retries.
I also improve the code to only track the replaced manifest from previous snapshot in ManifestMergeManager
##########
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:
I guess if the manifest is written by ManifestWriter, it shall have snapshot configured on constructor. There's no strong reason to fail when we generate the optional snapshot summary, if you feel there's the need, we can only increment the kept-manifest count if `manifest.snapshotId()` is non-null?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2756189695
##########
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:
thx for checking
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2737827969
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -87,6 +87,8 @@ public String partition() {
// cache filtered manifests to avoid extra work when commits fail.
private final Map<ManifestFile, ManifestFile> filteredManifests = Maps.newConcurrentMap();
+ // count of manifests that were rewritten with different manifest entry status during filtering
Review Comment:
That's a good catch, moved to atomic integer in both ManifestFilterManager and ManifestMergeManager
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#issuecomment-3801408903
> I'm generally +1 on this idea but don't have time to do a full review right now, I do think we should consider using "existing" instead of "kept"? Or maybe skip it all together since I think we aren't tracking manifests which are not scanned in the first place right?
Thanks @RussellSpitzer , discussed offline as we reuse the SnapshotSummary already defined in https://iceberg.apache.org/spec/#optional-snapshot-summary-fields. Previously we only populate such for [rewrite-manifest](https://github.com/apache/iceberg/blob/f5635a65676df12b00f2c8365357d6026202b397/api/src/main/java/org/apache/iceberg/RewriteManifests.java#L38) operation, this change I want to introduce them for all commits result in a new snapshot, like append, row-delta and delete etc
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2734627192
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -645,6 +645,35 @@ protected boolean cleanupAfterCommit() {
return true;
}
+ /**
+ * Updates manifest count in the snapshot summary builder, including replaced manifests.
+ *
+ * @param summaryBuilder the summary builder to update
+ * @param manifests the list of manifests in the new snapshot
+ * @param replacedManifestsCount the count of manifests that were replaced (rewritten)
+ */
+ protected SnapshotSummary.Builder buildManifestCountSummary(
+ SnapshotSummary.Builder summaryBuilder,
+ List<ManifestFile> manifests,
+ int replacedManifestsCount) {
+ int manifestsCreated = 0;
+ int manifestsKept = 0;
+
+ for (ManifestFile manifest : manifests) {
Review Comment:
I guess this is probably a clever way to centralize the computation for created and kept manifests.
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -645,6 +645,35 @@ protected boolean cleanupAfterCommit() {
return true;
}
+ /**
+ * Updates manifest count in the snapshot summary builder, including replaced manifests.
+ *
+ * @param summaryBuilder the summary builder to update
+ * @param manifests the list of manifests in the new snapshot
+ * @param replacedManifestsCount the count of manifests that were replaced (rewritten)
+ */
+ protected SnapshotSummary.Builder buildManifestCountSummary(
Review Comment:
is `protected` or a `static` util method better here?
##########
core/src/main/java/org/apache/iceberg/FastAppend.java:
##########
@@ -36,6 +36,7 @@
class FastAppend extends SnapshotProducer<AppendFiles> implements AppendFiles {
private final String tableName;
private final SnapshotSummary.Builder summaryBuilder = SnapshotSummary.builder();
+ private final SnapshotSummary.Builder manifestsSummary = SnapshotSummary.builder();
Review Comment:
this doesn't need to be a class member. it can just be a stack local var.
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -87,6 +87,8 @@ public String partition() {
// cache filtered manifests to avoid extra work when commits fail.
private final Map<ManifestFile, ManifestFile> filteredManifests = Maps.newConcurrentMap();
+ // count of manifests that were rewritten with different manifest entry status during filtering
Review Comment:
nit: not adding this btw the to filtered maps.
also we probably have thread safety issue here. the filter manager runs in thread pool.
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -645,6 +645,35 @@ protected boolean cleanupAfterCommit() {
return true;
}
+ /**
+ * Updates manifest count in the snapshot summary builder, including replaced manifests.
+ *
+ * @param summaryBuilder the summary builder to update
+ * @param manifests the list of manifests in the new snapshot
+ * @param replacedManifestsCount the count of manifests that were replaced (rewritten)
+ */
+ protected SnapshotSummary.Builder buildManifestCountSummary(
+ SnapshotSummary.Builder summaryBuilder,
+ List<ManifestFile> manifests,
+ int replacedManifestsCount) {
+ int manifestsCreated = 0;
+ int manifestsKept = 0;
+
+ for (ManifestFile manifest : manifests) {
+ if (manifest.snapshotId() == snapshotId()) {
Review Comment:
`manifest.snapshotId()` return type is not a primitive. could it be null?
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -645,6 +645,35 @@ protected boolean cleanupAfterCommit() {
return true;
}
+ /**
Review Comment:
we missed one child class `BaseRewriteManifests`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#issuecomment-3929357233
Thanks @dramaticlly for the PR! Thanks @RussellSpitzer @stevenzwu for the review!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2737826045
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -645,6 +645,35 @@ protected boolean cleanupAfterCommit() {
return true;
}
+ /**
+ * Updates manifest count in the snapshot summary builder, including replaced manifests.
+ *
+ * @param summaryBuilder the summary builder to update
+ * @param manifests the list of manifests in the new snapshot
+ * @param replacedManifestsCount the count of manifests that were replaced (rewritten)
+ */
+ protected SnapshotSummary.Builder buildManifestCountSummary(
Review Comment:
discussed offline, the method still relies on non-static snapshotId() method.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2728961189
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -313,6 +315,18 @@ private Set<F> deletedFiles(ManifestFile[] manifests) {
return deletedFiles;
}
+ /**
+ * Returns the count of manifests that were replaced (rewritten) during filtering.
+ *
+ * <p>A manifest is considered replaced when a new manifest was created to replace the original
+ * one (i.e., the original manifest != filtered manifest).
Review Comment:
Just a note because of the normal append path in merging snapshot producer this can also be original manifest != appended manifest
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu closed pull request #15003: Core: populate manifest created/replaced/kept count when commit a snapshot
URL: https://github.com/apache/iceberg/pull/15003
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#issuecomment-3801390635
I'm generally +1 on this idea but don't have time to do a full review right now, I do think we should consider using "existing" instead of "kept"? Or maybe skip it all together since I think we aren't tracking manifests which are not scanned in the first place right?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao merged PR #15003:
URL: https://github.com/apache/iceberg/pull/15003
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2755712455
##########
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:
Is this correct in the case of multi-snapshot transaction commit?
##########
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:
do we still need this new test method? is any scenarios here not covered by the existing tests?
##########
core/src/main/java/org/apache/iceberg/ManifestMergeManager.java:
##########
@@ -200,8 +220,14 @@ private ManifestFile createManifest(int specId, List<ManifestFile> bin) {
ManifestFile manifest = writer.toManifestFile();
- // update the cache
+ // cache the merged manifest to reuse when retrying and track replaced manifests
mergedManifests.put(bin, manifest);
+ for (ManifestFile m : bin) {
+ // only count manifests from previous snapshots; in-memory manifests are not replaced
+ if (snapshotId() != m.snapshotId()) {
Review Comment:
same comment for multi-snapshot transaction commit
##########
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:
do we still need this method after adding assertions to existing test methods?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2750291752
##########
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:
still trying to think the scenario if `manifest.snapshotId()` can be null for new manifest files added in the new snapshot?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2728964460
##########
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java:
##########
@@ -313,6 +315,18 @@ private Set<F> deletedFiles(ManifestFile[] manifests) {
return deletedFiles;
}
+ /**
+ * Returns the count of manifests that were replaced (rewritten) during filtering.
+ *
+ * <p>A manifest is considered replaced when a new manifest was created to replace the original
+ * one (i.e., the original manifest != filtered manifest).
Review Comment:
ah nvm, didn't see we were in manifest filter manager
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#issuecomment-3838923794
looks like github action is recovered from previous incident per https://www.githubstatus.com/ and reopen the PR to rerun previously cancelled CI.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2737821715
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -645,6 +645,35 @@ protected boolean cleanupAfterCommit() {
return true;
}
+ /**
+ * Updates manifest count in the snapshot summary builder, including replaced manifests.
+ *
+ * @param summaryBuilder the summary builder to update
+ * @param manifests the list of manifests in the new snapshot
+ * @param replacedManifestsCount the count of manifests that were replaced (rewritten)
+ */
+ protected SnapshotSummary.Builder buildManifestCountSummary(
+ SnapshotSummary.Builder summaryBuilder,
+ List<ManifestFile> manifests,
+ int replacedManifestsCount) {
+ int manifestsCreated = 0;
+ int manifestsKept = 0;
+
+ for (ManifestFile manifest : manifests) {
+ if (manifest.snapshotId() == snapshotId()) {
Review Comment:
Sounds good, flipped equal and snapshotId() will return primitive long
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#issuecomment-3842966497
@nastra @amogh-jahagirdar for the changes in iceberg core on snapshot producer and manifest filter/merge manager, it would love to have your input/review.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#issuecomment-3745503219
also @huaxingao and @stevenzwu, if you are interested to take a look.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#issuecomment-3801615277
You are totally right! Better to keep with the original definitions then
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2734729862
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -645,6 +645,35 @@ protected boolean cleanupAfterCommit() {
return true;
}
+ /**
Review Comment:
I believe it has its own logic to populate those snapshot summary in https://github.com/apache/iceberg/blob/026ec35b7969539b535af03944fae53d059cd233/core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java#L101-L106
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2737826045
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -645,6 +645,35 @@ protected boolean cleanupAfterCommit() {
return true;
}
+ /**
+ * Updates manifest count in the snapshot summary builder, including replaced manifests.
+ *
+ * @param summaryBuilder the summary builder to update
+ * @param manifests the list of manifests in the new snapshot
+ * @param replacedManifestsCount the count of manifests that were replaced (rewritten)
+ */
+ protected SnapshotSummary.Builder buildManifestCountSummary(
Review Comment:
I believe this is already protected as of now. I dont have strong opinion with respect to static part.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#issuecomment-3730136503
@nastra @amogh-jahagirdar if you can help take a look
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2755656794
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -645,6 +645,35 @@ protected boolean cleanupAfterCommit() {
return true;
}
+ /**
Review Comment:
thx. I misread the code
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly closed pull request #15003: Core: populate manifest created/replaced/kept count when commit a snapshot
URL: https://github.com/apache/iceberg/pull/15003
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: populate manifest created/replaced/kept count when commit a snapshot [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #15003:
URL: https://github.com/apache/iceberg/pull/15003#discussion_r2755704435
##########
core/src/main/java/org/apache/iceberg/ManifestMergeManager.java:
##########
@@ -200,8 +217,9 @@ private ManifestFile createManifest(int specId, List<ManifestFile> bin) {
ManifestFile manifest = writer.toManifestFile();
- // update the cache
+ // update the cache and track replaced manifests
mergedManifests.put(bin, manifest);
+ replacedManifestsCount.addAndGet(bin.size());
Review Comment:
thx for explaining.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org