mbutrovich commented on code in PR #3145:
URL: https://github.com/apache/iceberg-rust/pull/3145#discussion_r4065882767
##########
crates/iceberg/src/arrow/caching_delete_file_loader.rs:
##########
@@ -347,46 +346,26 @@ impl CachingDeleteFileLoader {
fn validate_deletion_vector_task(
task: &FileScanTaskDeleteFile,
) -> Result<(u64, u64, String, u64)> {
- let content_offset = task.content_offset.ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!(
- "deletion vector {} is missing content_offset",
- task.file_path
- ),
- )
- })?;
- let content_size = task.content_size_in_bytes.ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!(
- "deletion vector {} is missing content_size_in_bytes",
- task.file_path
- ),
- )
- })?;
- let data_file_path = task.referenced_data_file.clone().ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!(
- "deletion vector {} is missing referenced_data_file",
- task.file_path
- ),
- )
- })?;
- let record_count = task.record_count.ok_or_else(|| {
- Error::new(
- ErrorKind::DataInvalid,
- format!("deletion vector {} is missing record_count",
task.file_path),
- )
- })?;
+ let content_offset = task
+ .content_offset()
+ .expect("validated deletion vector must have content_offset");
Review Comment:
Yes, please add it here. #3135 asks for `FileScanTaskDeleteFile` to be
validated at construction, and deserialization is one of the ways it gets
constructed. Right now a deletion vector with no `referenced_data_file`,
`content_offset`, `content_size_in_bytes`, or `record_count` deserializes
without error. The PR's own
`test_validate_deserialized_deletion_vector_rejects_missing_fields` builds
exactly that task. The same thing happens one level up. `FileScanTask`
deserializes through
[`TryFrom<FileScanTaskSerde>`](https://github.com/apache/iceberg-rust/blob/9f5cbbf7770a6dae775e123ea83d666b3cde529e/crates/iceberg/src/scan/task.rs#L526-L557),
which passes `value.deletes` to `with_deletes` without validating the entries.
`FileScanTask` got the same treatment in two steps. #3131 made `build()`
validate, and #3091 routed deserialization through the builder with
[`#[serde(try_from =
"...FileScanTaskSerde")]`](https://github.com/apache/iceberg-rust/blob/9f5cbbf7770a6dae775e123ea83d666b3cde529e/crates/iceberg/src/scan/task.rs#L36).
The delete file can follow the same pattern with a mirror struct in `mod
_serde` that goes through `builder().build()`. After that, every
`FileScanTaskDeleteFile` has passed `validate()`.
`validate_deletion_vector_task` would then repeat the four checks in
`validate()` (same fields, same messages) without doing anything new. One
`pub(crate)` method on `FileScanTaskDeleteFile` that returns the coordinates as
a `Result`, called by both `validate()` and the loader, would leave one copy of
those checks. The loader could also call `is_deletion_vector()` at line 269
instead of checking `file_format()` on its own.
--
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]
