amogh-jahagirdar commented on code in PR #18225:
URL: https://github.com/apache/iceberg/pull/18225#discussion_r4094010949
##########
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());
Review Comment:
Minor: Could go either way on this, but do we really need the new `inherit`?
Since it's package private and we already have the comment , I feel like we
could just change the existing inherit to be (long, Long) and then pass in an
explicit null for the sequence number. Then it just becomes a one liner instead
of the if/else. And with the comment, I think the meaning of null is pretty
clear.
```
tracking.inherit(manifest.snapshotId(), isUncommitted ? null :
manifest.sequenceNumber());
```
--
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]
