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: [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]
