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. It also feels a bit
dangerous to expose this for DVs, granted it's not pbulic, still want to
prevent misuse.
I think the right way to fix this is in
MergingSnapshotProducer#validateNoNewDeletesForDataFiles to build a
DeleteFileIndex _per_ concurrently added snapshot. For every data file we're
validating we go through each delete index and do the validation. @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). Each index is bound to how many delete files
there are.
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]
