linhongyu510 commented on code in PR #3145:
URL: https://github.com/apache/iceberg-rust/pull/3145#discussion_r4054378444
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -400,7 +404,145 @@ pub struct FileScanTaskDeleteFile {
#[serde(default)]
#[serde(skip_serializing_if = "Option::is_none")]
#[builder(default)]
- pub key_metadata: Option<Box<[u8]>>,
+ key_metadata: Option<Box<[u8]>>,
+}
+
+impl FileScanTaskDeleteFile {
+ /// Returns the delete file path.
+ pub fn file_path(&self) -> &str {
+ &self.file_path
+ }
+
+ /// Returns the total size of the delete file in bytes.
+ pub fn file_size_in_bytes(&self) -> u64 {
+ self.file_size_in_bytes
+ }
+
+ /// Returns the delete file content type.
+ pub fn file_type(&self) -> DataContentType {
+ self.file_type
+ }
+
+ /// Returns the delete file format.
+ pub fn file_format(&self) -> DataFileFormat {
+ self.file_format
+ }
+
+ /// Returns the partition spec id.
+ pub fn partition_spec_id(&self) -> i32 {
+ self.partition_spec_id
+ }
+
+ /// Returns the equality field ids for an equality delete file.
+ pub fn equality_ids(&self) -> Option<&[i32]> {
+ self.equality_ids.as_deref()
+ }
+
+ /// Returns the referenced data file path.
+ pub fn referenced_data_file(&self) -> Option<&str> {
+ self.referenced_data_file.as_deref()
+ }
+
+ /// Returns the deletion vector blob offset.
+ pub fn content_offset(&self) -> Option<i64> {
+ self.content_offset
+ }
+
+ /// Returns the deletion vector blob size in bytes.
+ pub fn content_size_in_bytes(&self) -> Option<i64> {
+ self.content_size_in_bytes
+ }
+
+ /// Returns the number of records in the delete file.
+ pub fn record_count(&self) -> Option<u64> {
+ self.record_count
+ }
+
+ /// Returns the key metadata for the encrypted delete file.
+ pub fn key_metadata(&self) -> Option<&[u8]> {
+ self.key_metadata.as_deref()
+ }
+
+ fn is_deletion_vector(&self) -> bool {
+ self.file_type == DataContentType::PositionDeletes
+ && self.file_format == DataFileFormat::Puffin
+ }
+
+ fn validate(&self) -> Result<()> {
+ if !self.is_deletion_vector() {
+ return Ok(());
+ }
+
+ if self.referenced_data_file.is_none() {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "deletion vector {} is missing referenced_data_file",
+ self.file_path
+ ),
+ ));
+ }
+
+ match self.content_offset {
+ None => {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "deletion vector {} is missing content_offset",
+ self.file_path
+ ),
+ ));
+ }
+ Some(offset) if offset < 0 => {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "deletion vector {} has negative content_offset {}",
+ self.file_path, offset
+ ),
+ ));
+ }
+ Some(_) => {}
+ }
+
+ match self.content_size_in_bytes {
+ None => {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "deletion vector {} is missing content_size_in_bytes",
+ self.file_path
+ ),
+ ));
+ }
+ Some(size) if size < 0 => {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "deletion vector {} has negative content_size_in_bytes
{}",
+ self.file_path, size
+ ),
+ ));
+ }
+ Some(_) => {}
+ }
+
+ if self.record_count.is_none() {
Review Comment:
You were right, and "this is not resolved" was a fair call — my earlier
attempt only applied the negative check to non-DV files while leaving the
`Option` in place, which addressed half of it.
Both halves are done now in `9f5cbbf77`, and making the field `u64` is what
resolved them together:
**Applies to every delete file.** The `i64 -> u64` conversion happens once
in `TryFrom<&DeleteFileContext>`, where the manifest value actually enters the
task:
```rust
let to_offset = |value: Option<i64>, field: &str| -> Result<Option<u64>> {
... };
```
That runs for every delete-file kind before the content type is even
consulted, so a negative offset on a Parquet position-delete or an
equality-delete is rejected the same way as on a deletion vector. The previous
version had to special-case this inside `validate()`; now there is no case to
special-case.
**The `Option` stays, deliberately.** I looked at removing it and it is not
safe: the field is genuinely absent for non-DV delete files.
`DataFile::content_offset` is `Option<i64>` in the manifest, and
`test_delete_file_builder_accepts_non_dv_delete_without_dv_fields` covers
position/equality deletes that carry none of these fields. Making it
non-optional would force a sentinel — and `0` is a legitimate offset, as
`positional_deletes.rs` exercises with `with_content_offset(Some(0))`. So
`Option` distinguishes "absent" from "zero", which a plain `u64` could not.
`Option<u64>` therefore encodes exactly the two real states: absent, or a
valid unsigned offset. Negative is gone from the type.
Test coverage moved with the check —
`test_delete_context_rejects_negative_coordinates_for_non_dv` now drives a
Parquet position-delete manifest entry through the conversion, which is the
non-DV path you were asking about.
--
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]
