amogh-jahagirdar commented on code in PR #16957:
URL: https://github.com/apache/iceberg/pull/16957#discussion_r3831725344
##########
core/src/main/java/org/apache/iceberg/DeleteFileIndex.java:
##########
@@ -211,10 +226,34 @@ private DeleteFile findDV(long seq, DataFile dataFile) {
"DV data sequence number (%s) must be greater than or equal to data
file sequence number (%s)",
dv.dataSequenceNumber(),
seq);
+ validatePartitionMatch(dv, dataFile);
}
return dv;
}
+ private void validatePartitionMatch(DeleteFile deleteFile, DataFile
dataFile) {
+ ValidationException.check(
+ deleteFile.specId() == dataFile.specId(),
+ "Mismatched partition specs (%s, %s) for delete file %s and data file
%s:"
+ + " metadata is corrupted",
+ deleteFile.specId(),
Review Comment:
>we see it in existing DV's.
Yeah I can definitley get behind commit time validation, like we do when
merging potential duplicate DVs but I wasn't sure yet about read time. I'd say
+1 now. I think the change has good intent for inferring a bad table state. I
largely just wanted to reason about if there was any legitimate case where we'd
be improperly breaking reads, but no I don't think that's the case. Every kind
of compaction out there would be required to apply the deletes when producing
new files (even if the files have a new spec ID). I think this is a case where
having the implementation be stricter than what's in the spec is a good idea.
In case of a mismatch, it'll force readers to repair their table before moving
forward, which is a good thing because they would've been ignoring deletes this
time.
##########
core/src/main/java/org/apache/iceberg/DeleteFileIndex.java:
##########
@@ -211,10 +226,34 @@ private DeleteFile findDV(long seq, DataFile dataFile) {
"DV data sequence number (%s) must be greater than or equal to data
file sequence number (%s)",
dv.dataSequenceNumber(),
seq);
+ validatePartitionMatch(dv, dataFile);
}
return dv;
}
+ private void validatePartitionMatch(DeleteFile deleteFile, DataFile
dataFile) {
+ ValidationException.check(
+ deleteFile.specId() == dataFile.specId(),
+ "Mismatched partition specs (%s, %s) for delete file %s and data file
%s:"
+ + " metadata is corrupted",
+ deleteFile.specId(),
Review Comment:
>we see it in existing DV's.
Yeah I can definitley get behind commit time validation, like we do when
merging potential duplicate DVs but I wasn't sure yet about read time. I'd say
+1 now. I think the change has good intent for inferring a bad table state. I
largely just wanted to reason about if there was any legitimate case where we'd
be improperly breaking reads, but no I don't think that's the case. Every kind
of compaction out there would be required to apply the deletes when producing
new files (even if the files have a new spec ID). I think this is a case where
having the implementation be stricter than what's in the spec is a good idea.
In case of a mismatch, it'll force readers to repair their table before moving
forward, which is a good thing because they would've been ignoring deletes this
time.
--
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]
