amogh-jahagirdar commented on code in PR #17764:
URL: https://github.com/apache/iceberg/pull/17764#discussion_r3873394949
##########
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.
+ */
+ Builder indexDVsAsPositionDeletes() {
Review Comment:
I get why this works for the MergingSnapshotProducer validation case but I
feel like it's at the wrong abstraction imo. DeleteFileIndex shouldn't really
expose ways to allow callers to express a different way of indexing things, and
this option only exists just for our validation case.
I think the right way to fix this is in
MergingSnapshotProducer#validateNoNewDeletesForDataFiles to build a
DeleteFileIndex for concurrently added snapshots. For every data file we're
validating we go through each delete index and do the validation. Does this
make sense @xiaoxuandev , let me know if that makes sense or not.
The index is already bound to only added delete manifests for the
concurrently added snapshots for this validation case (N snapshots added
concurrnetly, N delete indices). I think in general we expect the number of
concurrently added snapshots to be slim especially for batch cases; the worst
case is a ton of streaming upsert snapshots concurrently, and even then it's no
additional I/O cost compared to what we do today, it's rather CPU bound hashing
etc where we're doing more lookups than before, but I think we can always
optimize that later.
--
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]
