amogh-jahagirdar commented on code in PR #8860:
URL: https://github.com/apache/iceberg/pull/8860#discussion_r1362567101
##########
core/src/main/java/org/apache/iceberg/BaseFile.java:
##########
@@ -460,6 +460,16 @@ public ByteBuffer keyMetadata() {
@Override
public List<Long> splitOffsets() {
+ if (splitOffsets == null || splitOffsets.length == 0) {
+ return null;
+ }
+
+ // If the last split offset is past the file size this means the split
offsets are corrupted and
Review Comment:
Do you mean throwing at read time (i.e. instead of returning null, we
throw)? If so, I don't think we want to do that because that's essentially
unnecessarily breaking future readers and split offsets are optional anyways;
this approach takes the stance of detecting the corruption and making sure read
logic doesn't leverage the corrupted metadata.
If you mean throwing at the time of writing the manifest entry (a
precondition check in the constructor of `BaseFile`), I went back and forth on
this but I think the problem there is let's say someone upgrades. When some
process is performed which rewrites a set of files (including some corrupted
entries) it would fail due to the precondition. The benefit is it would prevent
spreading the previous corruption which is nice, but at the cost of failing
operations. Considering again the corrupted split offsets will be ignored at
read time anyways, failing at write time seems needless.
To prevent spreading previous corrupted state, at the time of writing the
manifest if the corruption is detected the split offsets could be recomputed (a
sort of "fix-up" process). This requires more investigation though, not sure
how feasible it is and the perf implications (e.g. for Parquet we'd need to go
through the block metadata again)
let me know what you think!
--
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]
