aokolnychyi commented on a change in pull request #1080:
URL: https://github.com/apache/iceberg/pull/1080#discussion_r433987741
##########
File path: api/src/main/java/org/apache/iceberg/Snapshot.java
##########
@@ -64,13 +64,25 @@
long timestampMillis();
/**
- * Return the location of all manifests in this snapshot.
- * <p>
- * The current table is made of the union of the data files in these
manifests.
+ * Return all {@link ManifestFile} instances for either data or delete
manifests in this snapshot.
+ *
+ * @return a list of ManifestFile
+ */
+ List<ManifestFile> allManifests();
Review comment:
This seem like a breaking change to the public API. Technically, the old
description already assumed a list of all manifests. Do we think it is better
to change it now to avoid confusion and misuse?
##########
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:
Shouldn't this table return all manifests?
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
