linhongyu510 commented on PR #3145: URL: https://github.com/apache/iceberg-rust/pull/3145#issuecomment-5744683464
Pushed `9f5cbbf77`, which answers all of the open threads from @blackmwk and @mbutrovich. They turned out to have a single root fix, so summarizing here rather than only thread-by-thread. **Making `content_offset` / `content_size_in_bytes` `Option<u64>` resolves five findings at once:** | finding | how `u64` resolves it | |---|---| | "Why not change it to `u64`?" ×2 | directly | | "This duplicates a lot." | the duplicated block existed only to phrase two negative-value errors; both are now unrepresentable and deleted | | "should also apply to non deletion vector?" | the `i64 -> u64` conversion runs once at the manifest boundary, before content type is consulted | | "does that leave these `expect`s reachable on a hand-written plan?" | serde rejects a negative during deserialization, so a corrupted scan plan fails before any loader code runs | `validate()` went from ~70 lines to ~25, with four `Error::new` sites collapsed to one. Downstream, `validate_deletion_vector_task` lost both `u64::try_from` conversions. **The `expect`s @mbutrovich flagged are gone** — both `delete_file_index.rs` and `caching_delete_file_loader.rs` use `ok_or_else` and degrade to `DataInvalid`. `test_deletion_vector_missing_referenced_data_file_is_rejected` was removed as fallout from that change, not deliberately; it is restored, and both entry points now have their own regression. **Tests moved with the checks rather than being deleted.** The negative cases could no longer be expressed at the builder (`Some(-1)` stopped compiling), so they now drive real manifest entries through the conversion — including the non-DV case. The two loader tests assert serde refuses the value. ``` cargo test -p iceberg --lib 1683 passed, 0 failed cargo fmt --all -- --check clean cargo clippy -p iceberg --all-targets -- -D warnings clean ``` `public-api.txt` updated for the two getter signatures. One thing I deliberately did **not** do, flagged in the thread: `#[serde(try_from = ..)]` would also move the *presence* checks into deserialization. I left it out to keep the scope contained — those cases still produce `DataInvalid`, just from the loader rather than the deserializer. Happy to fold it in if you would prefer it complete in one change. -- 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]
