mbutrovich commented on code in PR #3145:
URL: https://github.com/apache/iceberg-rust/pull/3145#discussion_r4065888486
##########
crates/iceberg/src/arrow/caching_delete_file_loader.rs:
##########
@@ -1744,64 +1756,32 @@ mod tests {
)
}
+ // A negative byte offset cannot be deserialized at all now that these
fields are
+ // u64, so a hand-written or corrupted scan plan is rejected before any
loader code
+ // runs. That is strictly stronger than the previous runtime check, which
only fired
+ // once the task reached this function.
Review Comment:
Could these comments describe the code as it is now? This one and a few
others explain what changed in this PR ("now that these fields are u64",
"strictly stronger than the previous runtime check"). A reader of the file
later has no "previous" to compare against. That history fits better in the
commit message or the PR description. The same applies to:
- `crates/iceberg/src/arrow/caching_delete_file_loader.rs` lines 389-391
- `crates/iceberg/src/scan/task.rs` lines 631-634
- `crates/iceberg/src/scan/task.rs` lines 1011-1013
- `crates/iceberg/src/scan/task.rs` lines 1030-1031
In most of these, the code reads clearly enough that the comment can be
dropped.
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -535,11 +566,116 @@ mod _serde {
}
}
+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<u64> {
+ self.content_offset
+ }
+
+ /// Returns the deletion vector blob size in bytes.
+ pub fn content_size_in_bytes(&self) -> Option<u64> {
+ 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<()> {
+ // Negative offsets/sizes are unrepresentable: both fields are u64,
and the
+ // conversion from the manifest entry's i64 rejects a negative there.
So the
+ // only thing left to check is presence, and only for deletion vectors
--
+ // other delete files legitimately carry none of these.
+ if !self.is_deletion_vector() {
+ return Ok(());
+ }
+
+ let missing = if self.referenced_data_file.is_none() {
Review Comment:
`validate()` checks presence only for deletion vectors. The spec also has a
rule for every delete file: `content_size_in_bytes` is "required if
`content_offset` is present" ([spec.md
L743](https://github.com/apache/iceberg/blob/781a30f3c9b5c74dc15ad5e16f7266919f2cbfeb/format/spec.md?plain=1#L743)).
Right now a Parquet position delete with `content_offset` set and no size
builds without error. Since the negative-value conversion already covers every
kind, should `validate()` enforce this pairing for every kind too, with a test
for the non-DV case?
##########
crates/iceberg/src/arrow/delete_file_loader.rs:
##########
Review Comment:
`test_deletion_vector_builder_rejects_missing_referenced_data_file` and
`test_deletion_vector_builder_rejects_missing_coordinates` (line 1209) call
only `FileScanTaskDeleteFile::builder()`. They repeat
`test_delete_file_builder_rejects_dv_missing_referenced_data_file` and
`test_delete_file_builder_rejects_dv_missing_content_offset` in `scan/task.rs`,
and nothing in them involves the index.
What they replaced was the index-level test for missing coordinates. With
this PR that check fires in `get_deletes_for_data_file` through
`TryFrom<&DeleteFileContext>`, and no test covers that path anymore. It does
work at the head commit. I built a DV with `build_deletion_vector`, set
`content_offset = None`, and `get_deletes_for_data_file(&data_file, Some(0))`
returned `DataInvalid => deletion vector ... is missing content_offset`. Could
you replace these two tests with one that asserts through
`get_deletes_for_data_file`, so the index path is covered?
--
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]
