fix(scan): validate deletion vector delete tasks

[linhongyu510]

Summary

• validate deletion vector invariants when FileScanTaskDeleteFile is built
• migrate delete-file fixtures and callers to the validated builder result shape
• preserve DataInvalid errors for malformed manifests and deserialized scan plans instead of panicking
• remove duplicate single-task deletion-vector structural checks from the loader and delete-file index

Breaking change

• FileScanTaskDeleteFile fields are now private and exposed through read-only getters; downstream direct field access must migrate to the corresponding getter. This should be called out in the 0.11 release notes.

Testing

• cargo test -p iceberg scan::task::tests --lib (16 passed)
• cargo test -p iceberg delete_file_index::tests --lib (23 passed)
• cargo test -p iceberg arrow::caching_delete_file_loader::tests --lib (24 passed)
• cargo clippy -p iceberg --all-targets --all-features -- -D warnings
• cargo fmt --all -- --check
• git diff --check

Closes #3135.

[linhongyu510]

CI follow-up pushed in 8ff9d12: removed the internal design note that failed the Apache license-header check, made the helper private, and updated the validated builder public API baseline. Verified locally with 15 targeted scan tests, rustfmt, clippy -D warnings, LICENSE/NOTICE checks, and cargo-public-api 0.51.0 output comparison.

[linhongyu510]

Addressed the review in 0228cd4: all FileScanTaskDeleteFile fields are private, with read-only getters used by internal callers; the public API baseline is updated. Verified 66 focused tests, workspace fmt, Clippy with -D warnings, public API checks, and git diff --check.

[blackmwk]

1. I think this check should also apply to non deletion vector?
2. Why not make offset usize?

[blackmwk]

See above

[blackmwk]

I think this should also apply to non deletion vector? Also why not remove the Option

[mbutrovich]

I noticed this PR after opening #3180. I'll leave mine open for now as a reference, but we can try to get #3145 over the finish line first.

The two converge on the same mechanism (build_method(into = Result<..>) + validate() + is_deletion_vector()), so the differences are small. Two things that might be worth pulling across either way:

• The #3035 naming nit (r3919759548), renaming pos_deletes_by_path to pos_deletes_by_referenced_data_file, isn't covered here. It's independent of #3135 and could go separately.
• Making the fields private is a bigger breaking change for downstream than the validation itself. Would it be worth calling out explicitly in the description, so it shows up in the 0.11 release notes?

[mbutrovich]

I may be misreading the ordering here, but is the "validated" premise true at this point? PopulatedDeleteFileIndex::new consumes DeleteFileContext { manifest_entry, .. } fed straight from manifest scanning, and the conversion to FileScanTaskDeleteFile (and so build()) happens later, in get_deletes_for_data_file. If that's right, a manifest with a Puffin position-delete entry missing referenced_data_file would panic here, where it previously returned DataInvalid.

Relatedly, test_deletion_vector_missing_referenced_data_file_is_rejected is removed in this PR. Was that deliberate, or fallout from the check moving?

One option: keep this one check returning DataInvalid, since the index needs the value as its map key and so can't defer it, and move only content_offset / content_size_in_bytes to the builder. Curious whether you considered that and preferred the expect.

[mbutrovich]

Same question about the guarantee, for a different reason: FileScanTaskDeleteFile derives plain Deserialize with no #[serde(try_from = ..)], unlike FileScanTask, which has one. So a task rebuilt from a serialized scan plan, the planner to worker path the key_metadata doc comment describes, never passes through build(). Does that leave these expects reachable on a malformed or hand-written plan?

Since the function already returns Result, keeping ok_or_else(..) here would cost little and degrade to DataInvalid instead of a panic. Alternatively, adding #[serde(try_from = ..)] to route deserialization through the same validation would make the expects sound, which seems like the stronger fix if it's not too much scope for this PR.

[linhongyu510]

Addressed the latest review in a27d15970:

• restored DataInvalid at index construction when a Puffin position delete lacks referenced_data_file; the regression test now exercises the manifest-to-index path
• replaced the infallible From<&DeleteFileContext> conversion with TryFrom and propagated builder errors through all delete-file lookup paths
• kept loader-side required-field checks for deserialized/hand-written scan plans, so malformed tasks return DataInvalid instead of panicking
• applied non-negative validation to any provided content_offset / content_size_in_bytes, including non-DV delete files
• kept the coordinate fields optional because ordinary Parquet delete files legitimately omit them; kept the signed manifest representation and checked conversion to u64 at the read boundary
• updated the PR description to call out private fields as a 0.11 downstream breaking change

I left the independent pos_deletes_by_referenced_data_file rename out of this bug fix to avoid expanding scope.

Verification: 63 focused tests passed; workspace fmt, all-target/all-feature Clippy with -D warnings, and git diff --check passed.