JeonDaehong commented on code in PR #3433:
URL: https://github.com/apache/iceberg-rust/pull/3433#discussion_r4259900026
##########
crates/iceberg/src/arrow/delete_file_loader.rs:
##########
@@ -66,13 +66,45 @@ impl BasicDeleteFileLoader {
pub(crate) async fn parquet_to_batch_stream(
&self,
data_file_path: &str,
- file_size_in_bytes: u64,
+ file_size_in_bytes: Option<u64>,
key_metadata: Option<&[u8]>,
) -> Result<ArrowRecordBatchStream> {
/*
Essentially a super-cut-down ArrowReader. We can't use ArrowReader
directly
as that introduces a circular dependency.
*/
+ // `None` means the task did not carry a size (see
`FileScanTaskDeleteFile`).
+ // Resolve it when the file is first loaded.
+ let file_size_in_bytes = match file_size_in_bytes {
+ Some(file_size_in_bytes) => file_size_in_bytes,
+ None => {
+ self.file_io
+ .new_input(data_file_path)?
+ .metadata()
+ .await
+ .map_err(|error| {
+ // Keep the storage error's kind and retryability.
+ Error::new(
+ error.kind(),
+ format!("Failed to stat delete file
'{data_file_path}'"),
+ )
+ .with_retryable(error.retryable())
+ .with_source(error)
+ })?
+ .size
+ }
+ };
+ // Reading a Parquet footer needs at least its last eight bytes. Reject
+ // a smaller size rather than read the file as if it had no deletes.
+ if file_size_in_bytes < 8 {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Delete file '{data_file_path}' has a size of
{file_size_in_bytes} bytes, \
+ below the Parquet footer minimum"
+ ),
+ ));
+ }
Review Comment:
Agreed, removed in b2981e2ea. The test now checks Parquet's `file size of N
is less than footer` error for `None` on an empty file, `Some(0)` and `Some(4)`.
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -366,8 +366,14 @@ pub struct FileScanTaskDeleteFile {
/// The delete file path
file_path: String,
- /// The total size of the delete file in bytes, from the manifest entry.
- file_size_in_bytes: u64,
+ /// The total size of the delete file in bytes. Tasks planned from
manifest entries carry
+ /// the recorded size. An externally constructed task may leave it unset
when the size is
+ /// unavailable; the Parquet delete-file loader then resolves it with one
metadata request
+ /// when the file is first loaded. Deletion vectors do not use it.
Review Comment:
Added in b2981e2ea, with a link to apache/iceberg#12554.
--
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]
