Core: Make removal of delete files optional in manifest filtering #4370
Conversation
| dropPartitions.contains(file.specId(), file.partition()) || | ||
| (isDelete && entry.sequenceNumber() > 0 && entry.sequenceNumber() < minSequenceNumber); | ||
|
|
||
| boolean nonMatchingDeleteFile = !file.content().equals(FileContent.DATA) && !evaluator.rowsMustMatch(file); |
There was a problem hiding this comment.
We should avoid extra calls to evaluator if possible. Also, there was isDelete var already defined.
| reader.entries().forEach(entry -> { | ||
| F file = entry.file(); | ||
| boolean fileDelete = deletePaths.contains(file.path()) || | ||
| boolean markedForDelete = deletePaths.contains(file.path()) || |
There was a problem hiding this comment.
Renamed for consistency.
| manifest.path(), wrapper.get()); | ||
| duplicateDeleteCount += 1; | ||
| if (allRowsMatch) { | ||
| writer.delete(entry); |
There was a problem hiding this comment.
Simply moved the existing logic under if now.
| .commit(); | ||
|
|
||
| validateTableFiles(table, FILE_DAY_2, FILE_DAY_2_MODIFIED); | ||
| validateTableDeleteFiles(table, FILE_DAY_2_POS_DELETES); |
There was a problem hiding this comment.
This would previously fail as we should remove one delete file and one should be kept.
| hasDeletedFiles = allRowsMatch; | ||
|
|
||
| if (failAnyDelete) { | ||
| throw new DeleteException(reader.spec().partitionToPath(file.partition())); |
There was a problem hiding this comment.
In failAnyDelete case, this would trigger if we have a deleteFile but rows do not match. Probably this never happens in delete call, but wonder if we need to update the condition (failAnyDelete && allRowsMatch).
There was a problem hiding this comment.
Good catch. I'll fix.
| table.newOverwrite() | ||
| .overwriteByRowFilter(EXPRESSION_DAY_2_ANOTHER_ID_RANGE) | ||
| .addFile(FILE_DAY_2_MODIFIED) | ||
| .validateFromSnapshot(baseSnapshot.snapshotId()) |
There was a problem hiding this comment.
Optional: this validate from snapshot seems to be a bit useless in reproducing the problem, especially as there should be no other transactions after this base here, would it be better to remove all the validateXX flags to simplify the test?
There was a problem hiding this comment.
Good call, will simplify.
szehon-ho
left a comment
szehon-ho
left a comment
There was a problem hiding this comment.
Yep, new changes look good to me.
|
Thanks, @szehon-ho! |
…ache#4370) (cherry picked from commit ab1a3a8)
2 participants
This PR is a follow-up to #4304. In that PR, we updated the method that checks whether a manifest has an entry to be deleted. However, we did not update the method that actually does the manifest filtering. As a consequence, the test added in this PR would fail with a runtime exception as one delete file should be removed and one kept. In addition, we should not introduce any extra calls to
evaluatoras they are expensive.Overall, the problem is that whenever we overwrite data by filter, we should not fail operations when it is not possible to remove delete files. Removal of delete files is optional and is an optimization we should perform if possible.