gaborkaszab commented on code in PR #17523:
URL: https://github.com/apache/iceberg/pull/17523#discussion_r3988846592
##########
core/src/main/java/org/apache/iceberg/AllManifestsTable.java:
##########
@@ -136,13 +136,13 @@ protected CloseableIterable<FileScanTask> doPlanFiles() {
Iterables.transform(
filteredSnapshots,
snap -> {
- if (snap.manifestListLocation() != null) {
+ if (snap.rootLocation() != null) {
return new ManifestListReadTask(
dataTableSchema,
io,
schema(),
specs,
- new BaseManifestListFile(snap.manifestListLocation(),
snap.keyId()),
+ new BaseFileWithKeyId(snap.rootLocation(), snap.keyId()),
Review Comment:
I think I missed this in the "generic interface"
[PR](https://github.com/apache/iceberg/pull/17545). Already covered there,
might worth another rebase with that.
##########
api/src/main/java/org/apache/iceberg/Snapshot.java:
##########
@@ -167,9 +167,32 @@ default Iterable<DeleteFile> removedDeleteFiles(FileIO io)
{
* Return the location of this snapshot's manifest list, or null if it is
not separate.
*
* @return the location of the manifest list for this Snapshot
+ * @deprecated since 1.12.0. Will be removed in 2.0.0; use {@link
#rootLocation()}, which returns
+ * the manifest list for v3 and earlier and the root manifest for v4+.
*/
+ @Deprecated
String manifestListLocation();
+ /**
+ * Returns the location of this snapshot's root metadata file — a manifest
list for v3 and
+ * earlier, or a root manifest for v4+.
+ *
+ * @return the location of the root file for this Snapshot
+ */
+ default String rootLocation() {
+ return manifestListLocation();
Review Comment:
Not entirely sure we should default to a deprecated function. Can't we throw
UOE and let the implementation override this?
##########
core/src/main/java/org/apache/iceberg/SnapshotsTable.java:
##########
@@ -94,13 +95,16 @@ public CloseableIterable<FileScanTask> planFiles() {
}
}
- private static StaticDataTask.Row snapshotToRow(Snapshot snap) {
+ static StaticDataTask.Row snapshotToRow(Snapshot snap) {
+ boolean adaptive = snap.formatVersion() !=
TableMetadata.UNREPORTED_FORMAT_VERSION;
Review Comment:
Snapshot.formatVersion() is always zero ATM if I'm not mistaken. This again
makes me think that the snapshots metadata table part of this PR should be
separate and taken care of when we actually have something v4 related to how
there.
##########
core/src/main/java/org/apache/iceberg/ReachableFileUtil.java:
##########
@@ -99,36 +99,57 @@ private static TableMetadata
findFirstExistentPreviousMetadata(
}
/**
- * Returns locations of manifest lists in a table.
- *
- * @param table table for which manifestList needs to be fetched
- * @return the location of manifest lists
+ * @deprecated since 1.12.0, will be removed in 1.13.0; use {@link
#rootLocations(Table)}. The
+ * method returns v4+ root manifest locations too, so the name no longer
matches its behavior.
*/
+ @Deprecated
public static List<String> manifestListLocations(Table table) {
- return manifestListLocations(table, null);
+ return rootLocations(table, null);
}
/**
- * Returns locations of manifest lists in a table.
- *
- * @param table table for which manifestList needs to be fetched
- * @param snapshotIds ids of snapshots for which manifest lists will be
returned
- * @return the location of manifest lists
+ * @deprecated since 1.12.0, will be removed in 1.13.0; use {@link
#rootLocations(Table, Set)}.
+ * The method returns v4+ root manifest locations too, so the name no
longer matches its
+ * behavior.
*/
+ @Deprecated
public static List<String> manifestListLocations(Table table, Set<Long>
snapshotIds) {
+ return rootLocations(table, snapshotIds);
+ }
+
+ /**
+ * Returns the root location for every snapshot in the table — a manifest
list for v3 and earlier,
+ * or a root manifest for v4+.
+ *
+ * @param table table whose root locations should be fetched
+ * @return the root location per snapshot
+ */
+ public static List<String> rootLocations(Table table) {
+ return rootLocations(table, null);
+ }
+
+ /**
+ * Returns the root location for each snapshot filtered by id — a manifest
list for v3 and
+ * earlier, or a root manifest for v4+.
+ *
+ * @param table table whose root locations should be fetched
+ * @param snapshotIds ids of snapshots to include, or null for every snapshot
+ * @return the root location per matching snapshot
+ */
+ public static List<String> rootLocations(Table table, Set<Long> snapshotIds)
{
Iterable<Snapshot> snapshots = table.snapshots();
if (snapshotIds != null) {
snapshots = Iterables.filter(snapshots, s ->
snapshotIds.contains(s.snapshotId()));
}
- List<String> manifestListLocations = Lists.newArrayList();
+ List<String> rootLocations = Lists.newArrayList();
Review Comment:
nit: I know this is a pure rename of stuff, but this could be nice and
simple with stream api
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -102,14 +102,14 @@ public static void dropTableData(FileIO io, TableMetadata
metadata) {
// Reads and deletes are done using
Tasks.foreach(...).suppressFailureWhenFinished to complete
// as much of the delete work as possible and avoid orphaned data or
manifest files.
- Set<String> manifestListsToDelete = Sets.newHashSet();
+ Set<String> snapshotFilesToDelete = Sets.newHashSet();
Set<ManifestFile> manifestsToDelete = Sets.newHashSet();
for (Snapshot snapshot : metadata.snapshots()) {
// add all manifests to the delete set because both data and delete
files should be removed
Iterables.addAll(manifestsToDelete, snapshot.allManifests(io));
- // add the manifest list to the delete set, if present
- if (snapshot.manifestListLocation() != null) {
- manifestListsToDelete.add(snapshot.manifestListLocation());
+ // add the top-level snapshot file (manifest list for v3, root manifest
for v4+) if present
Review Comment:
"(manifest list for v3, root manifest for v4+)" maybe we should't spell this
out everywhere. Here probably no point of this comment, enough to document what
rootLocation means where it's implemented but not at the callsite.
This applies for all the other similar comments across this PR.
--
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]
