Re: [PR] Core: Detect concurrently added DVs in RewriteFiles [iceberg]

["JeonDaehong (via GitHub)" <gi...@apache.org>]

JeonDaehong commented on code in PR #18224:
URL: https://github.com/apache/iceberg/pull/18224#discussion_r4177166286


##########
core/src/main/java/org/apache/iceberg/BaseRewriteFiles.java:
##########
@@ -139,6 +140,8 @@ protected void validate(TableMetadata base, Snapshot parent) {
       // files
       validateNoNewDeletesForDataFiles(base, startingSnapshotId, replacedDataFiles, parent);
     }
+
+    validateAddedDVs(base, startingSnapshotId, Expressions.alwaysTrue(), parent);

Review Comment:
   Thanks for the fix! I have one question about callers that don’t set `validateFromSnapshot`.
   
   I ran `TestSnapshot#testCachedDeleteFiles` without adding the `validateFromSnapshot` line from this PR. The test passes on `main`, but fails on this PR for v3/v4 with:
   
   `Found concurrently added DV for ...`
   
   It looks like the DV being replaced is treated as concurrently added because, without a starting snapshot, the validation checks the entire history.
   
   This means that existing code that replaces DVs via `newRewrite()` without setting `validateFromSnapshot` would start failing with this change.
   
   Is this behavior intentional? For example, is the intention for `newRewrite()` to behave consistently with `RowDelta` in this regard?



-- 
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-unsubscribe@iceberg.apache.org

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


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