gaborkaszab commented on code in PR #18884:
URL: https://github.com/apache/iceberg/pull/18884#discussion_r4542736225
##########
core/src/main/java/org/apache/iceberg/SnapshotParser.java:
##########
@@ -176,7 +182,62 @@ static Snapshot fromJson(JsonNode node) {
String keyId = JsonUtil.getStringOrNull(KEY_ID, node);
- if (node.has(MANIFEST_LIST)) {
+ return newSnapshot(
+ node,
+ sequenceNumber,
+ snapshotId,
+ parentId,
+ timestamp,
+ operation,
+ summary,
+ schemaId,
+ firstRowId,
+ addedRows,
+ keyId);
+ }
+
+ private static Snapshot newSnapshot(
+ JsonNode node,
+ long sequenceNumber,
+ long snapshotId,
+ Long parentId,
+ long timestamp,
+ String operation,
+ Map<String, String> summary,
+ Integer schemaId,
+ Long firstRowId,
+ Long addedRows,
+ String keyId) {
+ if (node.has(ROOT_MANIFEST)) {
Review Comment:
Aren't these validated by `BaseSnapshot.withRootManifest()`? Seem redundant
for me.
##########
api/src/main/java/org/apache/iceberg/Snapshot.java:
##########
@@ -170,6 +170,16 @@ default Iterable<DeleteFile> removedDeleteFiles(FileIO io)
{
*/
String manifestListLocation();
+ /**
+ * Return the location of this snapshot's root manifest, or null if this
snapshot has no root
+ * manifest.
+ *
+ * @return the location of the root manifest for this Snapshot
+ */
+ default String rootManifestLocation() {
Review Comment:
This overlaps with @stevenzwu 's https://github.com/apache/iceberg/pull/17523
Reminds me, shouldn't we introduce all the new V4 snapshot fields in one PR?
Would require concluding on the [encryption
fields](https://github.com/apache/iceberg/pull/18503), but I don't think we are
far.
Doing them in one go might simplify the constructors around `Snapshot`.
##########
core/src/main/java/org/apache/iceberg/SnapshotParser.java:
##########
@@ -159,8 +163,10 @@ static Snapshot fromJson(JsonNode node) {
summary = builder.build();
// When the operation is not found, default to overwrite
- // to ensure that we can read the summary without raising an exception
- if (operation == null) {
+ // to ensure that we can read the summary without raising an exception.
Review Comment:
According to the spec, `operation` has been required in V2-V3 already and we
handled a missing `operation` gracefully here. The spec doesn't seem any
different for V4 in this aspect, what's the reason being stricter in the
implementation?
##########
core/src/main/java/org/apache/iceberg/FilePlanner.java:
##########
@@ -163,6 +170,14 @@ CloseableIterable<FileScanTask> planFiles() {
return CloseableIterable.transform(dataFiles, this::createTask);
}
+ private static void rejectColumnFiles(TrackedFile entry) {
+ List<ColumnFile> columnFiles = entry.columnFiles();
+ if (columnFiles != null && !columnFiles.isEmpty()) {
Review Comment:
Do we allow empty column files? Isn't it either null or has some items?
##########
core/src/main/java/org/apache/iceberg/BaseSnapshot.java:
##########
@@ -56,6 +57,41 @@ class BaseSnapshot implements Snapshot {
private transient List<DeleteFile> addedDeleteFiles = null;
private transient List<DeleteFile> removedDeleteFiles = null;
+ static BaseSnapshot withRootManifest(
Review Comment:
Is this function needed because the signature for the V3 Snapshot
constructor is the same as for the V4 constructor? I find this a bit messy
here, probably would help to introduce all the other new V4 Snapshot fields
together (see my other comment) and then this is not needed.
##########
core/src/main/java/org/apache/iceberg/V4ManifestReader.java:
##########
@@ -223,7 +223,10 @@ private boolean matchesPartition(TrackedFile trackedFile) {
private void incrementSkipCount(TrackedFile file) {
switch (file.contentType()) {
case DATA -> scanMetrics.skippedDataFiles().increment();
- case DATA_MANIFEST -> scanMetrics.skippedDataManifests().increment();
+ case DATA_MANIFEST -> {
Review Comment:
This seems independent of this PR. Separate one?
##########
core/src/main/java/org/apache/iceberg/SnapshotParser.java:
##########
@@ -176,7 +182,62 @@ static Snapshot fromJson(JsonNode node) {
String keyId = JsonUtil.getStringOrNull(KEY_ID, node);
- if (node.has(MANIFEST_LIST)) {
+ return newSnapshot(
+ node,
+ sequenceNumber,
+ snapshotId,
+ parentId,
+ timestamp,
+ operation,
+ summary,
+ schemaId,
+ firstRowId,
+ addedRows,
+ keyId);
+ }
+
+ private static Snapshot newSnapshot(
Review Comment:
I'm not sure what this new function adds to readability. I found it more
readable in the original form.
##########
core/src/main/java/org/apache/iceberg/SnapshotParser.java:
##########
@@ -176,7 +182,62 @@ static Snapshot fromJson(JsonNode node) {
String keyId = JsonUtil.getStringOrNull(KEY_ID, node);
- if (node.has(MANIFEST_LIST)) {
+ return newSnapshot(
+ node,
+ sequenceNumber,
+ snapshotId,
+ parentId,
+ timestamp,
+ operation,
+ summary,
+ schemaId,
+ firstRowId,
+ addedRows,
+ keyId);
+ }
+
+ private static Snapshot newSnapshot(
+ JsonNode node,
+ long sequenceNumber,
+ long snapshotId,
+ Long parentId,
+ long timestamp,
+ String operation,
+ Map<String, String> summary,
+ Integer schemaId,
+ Long firstRowId,
+ Long addedRows,
+ String keyId) {
+ if (node.has(ROOT_MANIFEST)) {
+ Preconditions.checkArgument(
+ !node.has(MANIFEST_LIST),
Review Comment:
the `MANIFEST_LISTS` branch didn't check for existence of `MANIFESTS` just
to fail. The former simply took precedence. Would be more simple if we did
that too here.
--
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]
