CTTY commented on code in PR #3035:
URL: https://github.com/apache/iceberg-rust/pull/3035#discussion_r3919930554
##########
crates/iceberg/src/delete_file_index.rs:
##########
@@ -146,31 +180,83 @@ impl PopulatedDeleteFileIndex {
/// Creates a new populated delete file index from a list of delete file
contexts, which
/// allows for fast lookup when determining which delete files apply to a
given data file.
///
- /// 1. Position deletes that reference a single data file, either through
the
+ /// 1. A V3 deletion vector (a `PositionDeletes` entry stored as `Puffin`)
is indexed by the
+ /// `referenced_data_file` field, which the spec requires for deletion
vectors.
+ /// Fails if two deletion vectors reference the same data file: the
spec allows at most
+ /// one deletion vector per data file per snapshot.
+ /// 2. Other position deletes that reference a single data file, either
through the
/// `referenced_data_file` field or through equal `file_path` column
bounds,
/// are indexed by that data file's path.
- /// 2. All other position deletes are indexed by the partition extracted
from
+ /// 3. All other position deletes are indexed by the partition extracted
from
/// their manifest entry.
- /// 3. Equality deletes stored with an unpartitioned spec are applied as
global
+ /// 4. Equality deletes stored with an unpartitioned spec are applied as
global
/// deletes, per the spec. All other equality deletes are indexed by
partition.
- fn new(files: Vec<DeleteFileContext>) -> PopulatedDeleteFileIndex {
+ fn new(files: Vec<DeleteFileContext>) -> Result<PopulatedDeleteFileIndex> {
let mut eq_deletes_by_partition: HashMap<Struct,
Vec<Arc<DeleteFileContext>>> =
HashMap::default();
let mut pos_deletes_by_partition: HashMap<Struct,
Vec<Arc<DeleteFileContext>>> =
HashMap::default();
let mut pos_deletes_by_path: HashMap<String,
Vec<Arc<DeleteFileContext>>> =
HashMap::default();
+ let mut dvs_by_referenced_data_file: HashMap<String,
Arc<DeleteFileContext>> =
+ HashMap::default();
let mut global_equality_deletes: Vec<Arc<DeleteFileContext>> = vec![];
- files.into_iter().for_each(|ctx| {
+ for ctx in files {
let arc_ctx = Arc::new(ctx);
- let partition = arc_ctx.manifest_entry.data_file().partition();
+ let data_file = arc_ctx.manifest_entry.data_file();
+ let partition = data_file.partition();
match arc_ctx.manifest_entry.content_type() {
DataContentType::PositionDeletes => {
- if let Some(path) =
referenced_data_file(arc_ctx.manifest_entry.data_file()) {
+ // A deletion vector is a position delete stored as a
Puffin blob. The file
+ // format is what distinguishes it from a position delete
parquet file.
+ if data_file.file_format() == DataFileFormat::Puffin {
+ // The spec requires referenced_data_file,
content_offset and
+ // content_size_in_bytes on a deletion vector, so a
missing one is a
+ // malformed manifest entry, not an ordinary position
delete to fall back
+ // on.
+ let Some(path) = data_file.referenced_data_file() else
{
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "deletion vector {} is missing
referenced_data_file",
+ arc_ctx.manifest_entry.file_path()
+ ),
+ ));
+ };
+
+ if data_file.content_offset().is_none()
+ || data_file.content_size_in_bytes().is_none()
+ {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "deletion vector {} is missing
content_offset or content_size_in_bytes",
+ arc_ctx.manifest_entry.file_path()
+ ),
+ ));
+ }
Review Comment:
These validations should be moved to
FileScanTaskDeleteFile::builder()::build()
I think it would be good to include this in 0.11 and would be happy if we
can address this in a follow up
##########
crates/iceberg/src/delete_file_index.rs:
##########
@@ -46,7 +57,10 @@ struct PopulatedDeleteFileIndex {
eq_deletes_by_partition: HashMap<Struct, Vec<Arc<DeleteFileContext>>>,
pos_deletes_by_partition: HashMap<Struct, Vec<Arc<DeleteFileContext>>>,
pos_deletes_by_path: HashMap<String, Vec<Arc<DeleteFileContext>>>,
- // TODO: Deletion Vector support
+ // V3 deletion vectors, keyed by the data file they apply to
(referenced_data_file). At most
+ // one exists per data file per snapshot, and when one applies it
supersedes any position
+ // delete files for that data file, partition-scoped or path-scoped alike.
+ dvs_by_referenced_data_file: HashMap<String, Arc<DeleteFileContext>>,
Review Comment:
nit: we can improve the naming consistency here by updating
pos_deletes_by_path to pos_deletes_by_referenced_data_file
##########
crates/iceberg/src/arrow/caching_delete_file_loader.rs:
##########
@@ -299,6 +330,137 @@ impl CachingDeleteFileLoader {
}
}
+ /// Validates a deletion-vector task and returns what the read needs as
typed values:
+ /// `(start, len, referenced data file path, expected cardinality)`.
+ ///
+ /// The spec requires `referenced_data_file`, `content_offset` and
`content_size_in_bytes` on
+ /// a deletion vector, and a deletion vector is always built from a
manifest entry, so it
+ /// always carries `record_count`. A missing one is a manifest-entry
inconsistency rather
+ /// than an I/O failure.
+ ///
+ /// Equality and ordinary position deletes have no equivalent validation
in this loader: a
+ /// malformed equality/position delete file fails loudly when the Parquet
reader can't open
+ /// it. A deletion vector's coordinates instead drive a raw byte-range
read with no format
+ /// to fail against, so a bad coordinate would otherwise decode silently
into the wrong (or
+ /// no) deletes, per the same corrupted-blob concern Iceberg-Java
validates in
+ /// `BitmapPositionDeleteIndex.deserializeBitmap`.
+ fn validate_deletion_vector_task(
Review Comment:
We should validate this when building `FileScanTaskDeleteFile`.
```
isDv == FileScanTaskDeleteFile.content_type == PosDel &&
FileScanTaskDeleteFile.file_format == puffin
fn build() {
if (isDv) { validate_deletion_vector_task() }
}
```
--
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]
