kinolaev commented on code in PR #15727:
URL: https://github.com/apache/iceberg/pull/15727#discussion_r3863569753
##########
core/src/main/java/org/apache/iceberg/DeleteFileIndex.java:
##########
@@ -220,38 +222,26 @@ private DeleteFile findDV(long seq, DataFile dataFile) {
}
DeleteFile dv = dvByPath.get(dataFile.location());
- if (dv != null) {
- ValidationException.check(
- dv.dataSequenceNumber() >= seq,
- "DV data sequence number (%s) must be greater than or equal to data
file sequence number (%s)",
- dv.dataSequenceNumber(),
- seq);
- validatePartitionMatch(dv, dataFile);
+ if (dv != null && (dv.dataSequenceNumber() < seq ||
!validatePartitionMatch(dv, dataFile))) {
+ return null;
}
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(),
- dataFile.specId(),
- deleteFile.location(),
- dataFile.location());
+ private boolean validatePartitionMatch(DeleteFile deleteFile, DataFile
dataFile) {
+ if (deleteFile.specId() != dataFile.specId()) {
+ // Mismatched partition specs for delete file and data file: metadata is
corrupted
+ return false;
+ }
if (partitionComparatorsBySpecId != null) {
Comparator<StructLike> partitionComparator =
partitionComparatorsBySpecId.get(deleteFile.specId());
- ValidationException.check(
- partitionComparator.compare(deleteFile.partition(),
dataFile.partition()) == 0,
- "Mismatched partition tuples (%s, %s) for delete file %s and data
file %s:"
- + " metadata is corrupted",
- deleteFile.partition(),
- dataFile.partition(),
- deleteFile.location(),
- dataFile.location());
+ if (partitionComparator.compare(deleteFile.partition(),
dataFile.partition()) != 0) {
+ // Mismatched partition tuples for delete file and data file: metadata
is corrupted
+ return false;
Review Comment:
@huaxingao, thank you for your persistence, and I apologize - I should have
read the full discussion instead of just the diff before leaving my previous
comment. I see the distinction now: a position delete file with a mismatched
partition indicates corruption, it is not just a dangling delete file.
Also, according to the spec, there is no such thing as a dangling DV because:
> When removing a data file, writers must also remove any deletion vector
that applies to that data file from delete manifests.
I checked, and this requirement is enforced by `ManifestFilterManager`.
Taking this into account, I think `RemoveDanglingDeletesSparkAction` should
only worry about spec-compliant dangling delete files. That is why I reverted
both validations in the core library and restricted the failing tests in
`TestRemoveDanglingDeleteAction` to format versions lower than 3
(32f758a17be73ccd00ac5d3dde1a97ccab1ee036).
Does it sound right to you?
--
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]
