[GitHub] [iceberg] rymurr commented on a change in pull request #1080: Split Snapshot.manifests into dataManifests and deleteManifests

[GitBox <gi...@apache.org>]

rymurr commented on a change in pull request #1080:
URL: https://github.com/apache/iceberg/pull/1080#discussion_r433760536



##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -136,7 +136,7 @@ protected long targetSplitSize(TableOperations ops) {
         } else {
           return StaticDataTask.of(
               ops.io().newInputFile(ops.current().file().location()),
-              snap.manifests(),
+              snap.dataManifests(), // if the manifest list is missing, the table must be v1 and has no delete manifests

Review comment:
       I am not sure I understand the comment here or why `dataManifests` is used instead of `allManifests`

##########
File path: core/src/test/java/org/apache/iceberg/TableTestBase.java
##########
@@ -224,10 +224,10 @@ void validateSnapshot(Snapshot old, Snapshot snap, long sequenceNumber, DataFile
   }
 
   void validateSnapshot(Snapshot old, Snapshot snap, Long sequenceNumber, DataFile... newFiles) {
-    List<ManifestFile> oldManifests = old != null ? old.manifests() : ImmutableList.of();
+    List<ManifestFile> oldManifests = old != null ? old.dataManifests() : ImmutableList.of();

Review comment:
       This and two other tests use `dataManifests()` where all other tests have been changed to `allManifests()`. Is this intentional?
   
   Other tests:
   `TestRemoveSnapshots`
   `RewriteManifestsAction`

##########
File path: core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java
##########
@@ -174,13 +173,13 @@ private ManifestFile copyManifest(ManifestFile manifest) {
 
     validateFilesCounts();
 
-    // TODO: add sequence numbers here
     Iterable<ManifestFile> newManifestsWithMetadata = Iterables.transform(
         Iterables.concat(newManifests, addedManifests, rewrittenAddedManifests),
         manifest -> GenericManifestFile.copyOf(manifest).withSnapshotId(snapshotId()).build());
 
     // put new manifests at the beginning
-    List<ManifestFile> apply = new ArrayList<>();
+    List<ManifestFile> apply = Lists.newArrayList();
+    apply.addAll(base.currentSnapshot().deleteManifests());

Review comment:
       Here data manifests are (potentially) rewritten while keeping deletes the same. Again with deletes at the front. Do I understand correctly? The comment is a bit misleading as immediately below it old delete manifests are added to the list.

##########
File path: core/src/main/java/org/apache/iceberg/FastAppend.java
##########
@@ -122,6 +122,10 @@ private ManifestFile copyManifest(ManifestFile manifest) {
   public List<ManifestFile> apply(TableMetadata base) {
     List<ManifestFile> newManifests = Lists.newArrayList();
 
+    if (base.currentSnapshot() != null) {
+      newManifests.addAll(base.currentSnapshot().deleteManifests());
+    }

Review comment:
       The addition of delete files in `newManifests` far above the addition of data files threw me a bit. Is it intentional to ensure the delete files are at the front of the list?

##########
File path: core/src/main/java/org/apache/iceberg/ScanSummary.java
##########
@@ -159,7 +159,7 @@ private void removeTimeFilters(List<Expression> expressions, Expression expressi
       removeTimeFilters(filters, Expressions.rewriteNot(scan.filter()));
       Expression rowFilter = joinFilters(filters);
 
-      Iterable<ManifestFile> manifests = table.currentSnapshot().manifests();
+      Iterable<ManifestFile> manifests = table.currentSnapshot().dataManifests();

Review comment:
       Is this explicitly ignoring the effect of deleted rows on partition metrics or is it just that you are short circuiting any delete files (as we can't use them anyways)




----------------------------------------------------------------
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.

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