linhongyu510 commented on code in PR #3145:
URL: https://github.com/apache/iceberg-rust/pull/3145#discussion_r4054382189
##########
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:
This was the sharpest of the findings, and it is now fixed at the level you
pointed at rather than only in the loader.
You were right on both counts: `FileScanTaskDeleteFile` derives plain
`Deserialize` with no `#[serde(try_from = ..)]`, unlike `FileScanTask` — so a
task rebuilt from a serialized scan plan on the planner→worker path never
passes through `build()`, and the `expect`s were reachable on a malformed or
hand-written plan.
**The `expect`s are gone**; the loader uses `ok_or_else(..)` and degrades to
`DataInvalid`, which was your minimal suggestion.
But you also named the stronger fix, and the `u64` change @blackmwk asked
for above turned out to deliver most of it for free. Now that `content_offset`
/ `content_size_in_bytes` are `Option<u64>`, **serde itself rejects a
negative** — a corrupted plan fails during deserialization, before any loader
code runs. No `try_from` attribute needed for that class of value, and it
cannot be bypassed by a future caller who forgets to validate.
Pinned directly, since the guarantee now lives in the deserializer:
```rust
#[test]
fn test_deserializing_negative_content_offset_is_rejected() {
let mut task = serde_json::to_value(valid_dv_task()).unwrap();
task["content_offset"] = serde_json::json!(-1);
let err =
serde_json::from_value::<FileScanTaskDeleteFile>(task).unwrap_err();
assert!(err.to_string().contains("invalid value"));
}
```
(plus the same for `content_size_in_bytes`). These replace the two old tests
that had to deserialize a bad value successfully and then catch it at runtime —
that is no longer possible, which is the point.
**What that does not cover, to be precise:** the *presence* requirements
(`referenced_data_file`, `record_count`, and the two coordinates on a DV) are
still only enforced by `build()`, so a hand-written plan omitting them is
caught by the loader's `ok_or_else` rather than by serde. `DataInvalid`, not a
panic — but later than the range check. Adding `#[serde(try_from = ..)]` would
close that too; I have left it out of this PR since it widens the scope, and
the failure mode is now a clean error either way. Happy to add it here if you
would rather it be complete in one change.
`cargo test -p iceberg --lib`: **1683 passed, 0 failed**.
--
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]
