xiaoxuandev commented on code in PR #17764:
URL: https://github.com/apache/iceberg/pull/17764#discussion_r3848221821
##########
core/src/main/java/org/apache/iceberg/DeleteFileIndex.java:
##########
@@ -485,6 +486,24 @@ Builder ignoreResiduals() {
return this;
}
+ /**
+ * Indexes deletion vectors as file-scoped position deletes, allowing a
data file to have more
+ * than one DV.
+ *
+ * <p>A data file has at most one live DV in a single snapshot, so by
default {@link #build()}
+ * rejects a second DV for the same data file. That invariant does not
hold across snapshots,
+ * and commit validation indexes delete manifests from every snapshot in
the validation window,
+ * so it requires an index that accepts them.
+ *
+ * <p>The resulting index only answers whether deletes exist for a data
file, not which deletes
+ * apply to it: {@code forDataFile} no longer suppresses partition-scoped
position deletes when
+ * a DV is present. Scans must use the default so that corrupt live
metadata is still detected.
+ */
Review Comment:
Thanks, good catch, you're right that both DVs show up in that list.
Two things worth noting: pre-fix this scenario had no message at all —
`Can't index multiple DVs` was thrown before `isEmpty()`. The error path is
untouched; only where DVs get indexed changed. And it isn't DV-specific:
`posDeletesByPath` has always held multiple entries per data file across the
window, including ones later removed, so v2 file-scoped position deletes report
the same way today.
I'm hesitant to go with filtering for two reasons. The list answers "what
was added since your starting snapshot", so trimming it to live files would
hide that two writers touched the file, not one. And liveness isn't derivable
from the window: a removal is recorded only in the manifest written by the
removing snapshot, and the window skips `replace` snapshots — so a superseded
DV still looks live there. Filtering would need fresh reads of the parent's
delete manifests, a different source than the validation uses.
So I went with your javadoc suggestion for now, on both overloads and worded
to hold for v2 deletes too.
Happy to revisit if you think the message still needs trimming, though it
would probably want to cover v2 position deletes as well, which likely makes it
its own change.
--
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]
