openinx commented on a change in pull request #2865:
URL: https://github.com/apache/iceberg/pull/2865#discussion_r677072072
##########
File path: core/src/main/java/org/apache/iceberg/BaseRewriteFiles.java
##########
@@ -85,4 +90,18 @@ public RewriteFiles rewriteFiles(Set<DataFile>
dataFilesToDelete, Set<DeleteFile
return this;
}
+
+ @Override
+ public RewriteFiles validateFromSnapshot(long snapshotId) {
+ this.startingSnapshotId = snapshotId;
+ return this;
+ }
+
+ @Override
+ protected void validate(TableMetadata base) {
+ if (replacedDataFiles.size() > 0) {
+ // if there are replaced data files, there cannot be any new row-level
deletes for those data files
+ validateNoNewDeletesForDataFiles(base, startingSnapshotId,
replacedDataFiles);
Review comment:
I think we should also handle the case :
* t1: table has FILE_A and EQ_DELETE_FILE_B;
* t2: RewriteFiles action to convert the EQ_DELETE_FILE_B to
POS_DELETE_FILE_C which hold the file_id and offset from FILE_A, just started
the txn but not commit;
* t3: DeleteFiles action committed the txn which deletes the FILE_A;
* t4: Try to commit the RewriteFiles action.
At timestamp `t4`, the RewriteFiles txn should be aborted because the data
file FILE_A which is relied by POS_DELETE_FILE_C has been deleted.
Do we plan to add this validation in the following PR, or did not consider
this case because we currently don't have introduced any RewriteFiles action
that converting equality delete files into positional delete files ?
--
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]
