Core: Fix deleted data files validation in OverwriteFiles #4303
Conversation
aokolnychyi
force-pushed
the
fix-overwrite-files
branch
from
1bda55e to
ff4e42d
Compare
|
@rdblue @szehon-ho @RussellSpitzer, could you take a look? |
| private Expression conflictDetectionFilter = null; | ||
| private boolean validateNewDataFiles = false; | ||
| private boolean validateNewDeleteFiles = false; | ||
| private boolean validateNewDeletes = false; |
There was a problem hiding this comment.
Cause we not only validate added delete files but also deleted data files. We have two types of deletes to cover.
| if (rowFilter() != Expressions.alwaysFalse()) { | ||
| Expression filter = conflictDetectionFilter != null ? conflictDetectionFilter : rowFilter(); | ||
| validateNoNewDeleteFiles(base, startingSnapshotId, filter); | ||
| validateDeletedDataFiles(base, startingSnapshotId, filter); |
| } | ||
|
|
||
| @Test | ||
| public void testConcurrentConflictingDataFileDeleteOverwriteByFilter() { |
There was a problem hiding this comment.
This test would previously fail as the commit would succeed.
There was a problem hiding this comment.
The error here is because file 2 is both modified and deleted correct?
There was a problem hiding this comment.
Yeah, we concurrently removed data from a partition what we are trying to overwrite.
|
|
||
| Snapshot firstSnapshot = table.currentSnapshot(); | ||
|
|
||
| OverwriteFiles overwrite = table.newOverwrite() |
There was a problem hiding this comment.
While this one is alright because file 2 is modified, but file 1 is deleted.
|
+1 from me too, glad we found and fixed it! |
|
Thanks, @RussellSpitzer @szehon-ho! Merged to unblock Szehon's PR. |
(cherry picked from commit cceef9f)
3 participants
This PR fixes the deleted data files validation for overwrites by filter in
OverwriteFiles. Right now, we allow removal of data files in partitions we overwrite, which, I think violates the assumption.Found by @szehon-ho in #4293.