stevenzwu commented on code in PR #18225:
URL: https://github.com/apache/iceberg/pull/18225#discussion_r4086352640
##########
core/src/main/java/org/apache/iceberg/TrackingStruct.java:
##########
@@ -132,6 +151,38 @@ void inherit(long manifestSnapshotId, long
manifestSeqNumber) {
}
}
+ /**
+ * Assign the first row ID to the given next row ID if it is unassigned.
+ *
+ * <p>If the {@code nextRowId} is null, the first row ID will also be set to
null. This is used
+ * when reading snapshots from older format versions that do not have
assigned row IDs.
+ *
+ * @param nextRowId the next row ID to assign, or null when reading v2 or
earlier snapshots
+ * @return true if the first row ID is assigned a non-null value
+ */
+ boolean assignFirstRowId(Long nextRowId) {
+ if (null == nextRowId) {
+ // null manifest first row ID from pre-v3 upgrade path
+ // defensively set the first row ID for every entry to be null
+ this.firstRowId = null;
+ return false;
+ }
+
+ boolean isAdded = status == EntryStatus.ADDED;
+ // EXISTING will assign to handle existing files upgraded from pre-v3
+ boolean isExisting = status == EntryStatus.EXISTING;
+ // MODIFIED will assign to handle existing files upgraded from pre-v3 AND
modified in the first
+ // v4 commit when IDs are assigned
+ boolean isModified = status == EntryStatus.MODIFIED;
+
+ if ((isAdded || isExisting || isModified) && null == firstRowId) {
Review Comment:
can we simplify this to `if (isLive() && null == firstRowId)`
##########
core/src/main/java/org/apache/iceberg/V4ManifestReader.java:
##########
@@ -130,7 +143,16 @@ public CloseableIterator<TrackedFile> iterator() {
private TrackedFile applyInheritance(TrackedFile file) {
// the reader uses TrackingStruct to read tracking so this cast is safe
TrackingStruct tracking = (TrackingStruct) file.tracking();
- tracking.inherit(manifest.snapshotId(), manifest.sequenceNumber());
+ if (isUncommitted) {
+ // uncommitted files cannot have a sequence number or assign first row ID
+ tracking.inherit(manifest.snapshotId());
+ } else {
+ tracking.inherit(manifest.snapshotId(), manifest.sequenceNumber());
+ if (tracking.assignFirstRowId(nextRowId)) {
Review Comment:
A v4 root can mix inline data files, v4 data manifests, legacy data
manifests, and legacy delete manifests.
This code snippet captures what I was thinking.
```java
if (isUncommitted) {
tracking.inherit(manifest.snapshotId());
} else {
tracking.inherit(manifest.snapshotId(), manifest.sequenceNumber());
switch (file.contentType()) {
case DATA -> {
if (tracking.assignFirstRowId(nextRowId)) {
this.nextRowId += file.recordCount();
}
}
case DATA_MANIFEST -> {
if (tracking.assignFirstRowId(nextRowId)) {
ManifestInfo info = file.manifestInfo();
this.nextRowId +=
info.addedRowsCount() + info.existingRowsCount() +
info.modifiedRowsCount();
}
}
default -> {
// delete manifests: do not assign or advance
}
}
}
```
`assignFirstRowId` still decides from null vs already-set (and live vs
`DELETED`/`REPLACED`).
BTW, I have PR to add the modified files and rows counts to the
ManifestInfo: https://github.com/apache/iceberg/pull/18212
--
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]
