danielcweeks commented on code in PR #12250:
URL: https://github.com/apache/iceberg/pull/12250#discussion_r1958620415


##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewritePositionDeleteFilesSparkAction.java:
##########
@@ -140,6 +148,33 @@ public RewritePositionDeleteFiles.Result execute() {
     }
   }
 
+  private boolean hasDeletionVectors() {

Review Comment:
   I don't feel like we're scanning for the right thing here.  Shouldn't we be 
checking to see if there are any positional delete files here (not puffin DVs)?
   
   If this is run on a V3 table, it shouldn't matter whether there are already 
DV files, only that that there are positional delete files that need to be 
rewritten.  This appears to assume that if there is a single DV, that 
everything has already been rewritten, which is not the case.  PosDel and DVs 
can coexist when a table is upgraded to v3.
   
   That said, this should probably be called something like 
`requiresDVRewrite()`



-- 
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: issues-unsubscr...@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org

Reply via email to