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: [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]
