gaborkaszab commented on code in PR #17523:
URL: https://github.com/apache/iceberg/pull/17523#discussion_r4478560708
##########
core/src/main/java/org/apache/iceberg/BaseSnapshot.java:
##########
@@ -68,6 +69,38 @@ class BaseSnapshot implements Snapshot {
Long firstRowId,
Long addedRows,
String keyId) {
+ this(
+ sequenceNumber,
+ snapshotId,
+ parentId,
+ timestampMillis,
+ operation,
+ summary,
+ schemaId,
+ manifestList,
+ null,
+ firstRowId,
+ addedRows,
+ keyId);
+ }
+
+ BaseSnapshot(
Review Comment:
Aren't these constructors asymmetric? We have now a variation that accepts
manifest list only, and another one that accepts both manifest list and root
manifest. To restore symmetry, we either need a third one accepting root
manifest only, or simply having the one that accepts both without the other 2
variations. I would personally go for the second option.
##########
core/src/main/java/org/apache/iceberg/SnapshotParser.java:
##########
@@ -87,11 +88,16 @@ static void toJson(Snapshot snapshot, JsonGenerator
generator) throws IOExceptio
// write just the location. manifests should not be embedded in JSON
along with a list
generator.writeStringField(MANIFEST_LIST, manifestList);
} else {
- // embed the manifest list in the JSON, v1 only
- JsonUtil.writeStringArray(
- MANIFESTS,
- Iterables.transform(snapshot.allManifests(DUMMY_FILE_IO),
ManifestFile::path),
- generator);
+ String rootManifest = snapshot.rootManifestLocation();
+ if (rootManifest != null) {
Review Comment:
This is an "if - else { if - else}" structure. I think we can use a cleaner
"if - else if - else" non-nested structure by adding `String rootManifest =
snapshot.rootManifestLocation()` above the if.
##########
core/src/main/java/org/apache/iceberg/SnapshotParser.java:
##########
@@ -176,9 +182,11 @@ static Snapshot fromJson(JsonNode node) {
String keyId = JsonUtil.getStringOrNull(KEY_ID, node);
- if (node.has(MANIFEST_LIST)) {
- // the manifest list is stored in a manifest list file
- String manifestList = JsonUtil.getString(MANIFEST_LIST, node);
+ String manifestList = JsonUtil.getStringOrNull(MANIFEST_LIST, node);
+ String rootManifest = JsonUtil.getStringOrNull(ROOT_MANIFEST, node);
+
+ if (manifestList != null || rootManifest != null) {
+ // manifest-list is used by v1-v3 snapshots, root-manifest by v4
snapshots
return new BaseSnapshot(
Review Comment:
This seems another reason why we should drop the manifest list only
constructor.
##########
core/src/main/java/org/apache/iceberg/FileCleanupStrategy.java:
##########
@@ -92,9 +92,12 @@ public abstract void cleanFiles(
ManifestFile.DELETED_FILES_COUNT.fieldId()));
protected CloseableIterable<ManifestFile> readManifests(Snapshot snapshot) {
- if (snapshot.manifestListLocation() != null) {
+ String location = snapshot.manifestListLocation();
+ if (location != null) {
return InternalData.read(
- FileFormat.AVRO,
fileIO.newInputFile(snapshot.manifestListLocation()))
+ FileFormat.AVRO,
+ ManifestLists.newInputFile(
+ fileIO, new BaseFileWithEncryptedKey(location, null,
snapshot.keyId(), null)))
Review Comment:
I'm not entirely sure about the scope of this PR now. Based on the title I
have the impression that we introduce support for the new root-manifest field
to `Snapshot` and its parsers. Here we also do some other stuff:
1) Route `InputFile` creation through the caching `ManifestLists` path.
2) Introduce the more generic abstraction of `ManifestListFile` in order to
be able to call `ManifestLists.newInputFile`.
Currently, I don't think that any of the above is the scope of this PR. Then
we could remove the dependency on https://github.com/apache/iceberg/pull/17545
too.
##########
core/src/main/java/org/apache/iceberg/SnapshotParser.java:
##########
@@ -87,11 +88,16 @@ static void toJson(Snapshot snapshot, JsonGenerator
generator) throws IOExceptio
// write just the location. manifests should not be embedded in JSON
along with a list
generator.writeStringField(MANIFEST_LIST, manifestList);
} else {
- // embed the manifest list in the JSON, v1 only
- JsonUtil.writeStringArray(
- MANIFESTS,
- Iterables.transform(snapshot.allManifests(DUMMY_FILE_IO),
ManifestFile::path),
- generator);
+ String rootManifest = snapshot.rootManifestLocation();
+ if (rootManifest != null) {
+ generator.writeStringField(ROOT_MANIFEST, rootManifest);
+ } else {
Review Comment:
I think the order of the checks is off. Previously we did "if not V2 then
V1". Shouldn't we check V3 root manifest first, then V2 manifest list, then V1?
##########
core/src/main/java/org/apache/iceberg/SnapshotsTable.java:
##########
@@ -37,7 +37,8 @@ public class SnapshotsTable extends BaseMetadataTable {
Types.NestedField.optional(
6,
"summary",
- Types.MapType.ofRequired(7, 8, Types.StringType.get(),
Types.StringType.get())));
+ Types.MapType.ofRequired(7, 8, Types.StringType.get(),
Types.StringType.get())),
Review Comment:
I recall @rdblue is in favor of not implementing metadata table support
right away as we might want to re-consider how they should look like in V4.
I know I've commented before not to add coverage for metadata table in this
PR, and I still think this way.
--
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]
