amogh-jahagirdar commented on code in PR #16263:
URL: https://github.com/apache/iceberg/pull/16263#discussion_r3222247033
##########
core/src/main/java/org/apache/iceberg/ManifestReader.java:
##########
@@ -417,14 +417,9 @@ public ManifestEntry<F> apply(ManifestEntry<F> entry) {
}
};
} else {
- // data file's first_row_id is null when the manifest's first_row_id is
null
- return entry -> {
- if (entry.file() instanceof BaseFile) {
- ((BaseFile<?>) entry.file()).setFirstRowId(null);
- }
-
- return entry;
- };
+ // Preserve the source entry’s first row ID even if the manifest hasn’t
assigned one since it
+ // may be EXISTING
+ return Function.identity();
Review Comment:
Ok, I think I get the rationale for the defensive null assignment now.
Preserving the entry made the upgrade test cases work because those will
anyways have null first row IDs but this is to be defensive against someone
producing older manifests with a first row ID somehow?
In either case, I've updated to pass in a committed flag to a pakcage
private ManifestFiles.read API , and if it's true we do the null assignment,
and if not, we preserve the entry as it is.
@RussellSpitzer I think you're saying we should add a Preconditions check
for the committed case that the firstRowId is indeed null. I can get behind
that, that seemed to be what @rdblue was implying in our offline conversation,
if that's our expectation we should have a precondition check.
--
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]
