mbutrovich opened a new pull request, #3180: URL: https://github.com/apache/iceberg-rust/pull/3180
## Which issue does this PR close? - Closes #3135. ## What changes are included in this PR? Review follow-ups from #3035, per @CTTY's feedback. - `FileScanTaskDeleteFile::build()` now validates deletion vectors. A deletion vector is a `PositionDeletes` entry stored as `Puffin`, and must carry `referenced_data_file`, `content_offset`, `content_size_in_bytes` and `record_count`, with non-negative Puffin coordinates. This follows `FileScanTask`, which already validates in `build()` via `build_method(into = Result<FileScanTask>)`. - `deletion_vector_coordinates()` is the single source of truth for those fields: `validate()` calls it at build time and `CachingDeleteFileLoader` at read time, so the loader's `validate_deletion_vector_task` is removed rather than duplicated. - `DeleteFileIndex` no longer checks `content_offset` / `content_size_in_bytes`; those now surface when a matched entry is converted into a scan task. It still requires `referenced_data_file`, the key it indexes deletion vectors by, which cannot be deferred. `From<&DeleteFileContext>` becomes `TryFrom`, and `get_deletes_for_data_file` propagates the error. - Also addresses a naming nit not tracked in #3135 ([review comment](https://github.com/apache/iceberg-rust/pull/3035#discussion_r3919759548)): `pos_deletes_by_path` is renamed to `pos_deletes_by_referenced_data_file`, for consistency with `dvs_by_referenced_data_file`. Breaking change: `FileScanTaskDeleteFile::builder().build()` now returns `Result<FileScanTaskDeleteFile>`. This is inherent to validating in `build()`, and matches `FileScanTask`. Deletion vector read behavior is otherwise unchanged, as is position and equality delete handling. ## Are these changes tested? Yes. - Unit tests in `task.rs` for each field the builder now rejects a deletion vector for, that a valid deletion vector builds and reports its coordinates, and that a Parquet position delete file is not held to those requirements. The first seven replace the `test_validate_deletion_vector_task_*` tests in `caching_delete_file_loader.rs`, which covered the same cases against the removed helper; the cardinality tests there are unchanged, since that check still belongs to the loader. - `test_deletion_vector_missing_coordinates_is_rejected` in `delete_file_index.rs` now asserts through `get_deletes_for_data_file`, where the check fires after the move. ## AI Disclosure Developed with the help of Claude Code, but I understand and support these changes. -- 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]
