mbutrovich commented on code in PR #3433:
URL: https://github.com/apache/iceberg-rust/pull/3433#discussion_r4444162359
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -366,8 +366,17 @@ 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 or the recorded size can't be trusted, such as position
delete files
+ /// rewritten by `rewrite_table_path` before Iceberg Java 1.12.0
+ /// (<https://github.com/apache/iceberg/issues/12554>). The Parquet
delete-file loader then
+ /// resolves it with one metadata request when the file is first loaded.
Deletion vectors
+ /// do not use it.
+ #[serde(default)]
+ #[serde(skip_serializing_if = "Option::is_none")]
+ #[builder(default, setter(strip_option(fallback =
with_file_size_in_bytes_opt)))]
Review Comment:
Could we keep the size required in the builder, and have callers that don't
know it pass `None` through `with_file_size_in_bytes_opt`? On `main` the field
[has no builder
default](https://github.com/apache/iceberg-rust/blob/896f9d6c50ff52fa9e43aec170d3f14bfb40b512/crates/iceberg/src/scan/task.rs#L369-L370),
so a builder chain that forgets the size fails to compile. With `default`
here, the same chain compiles and each delete file costs one extra metadata
request at read time, which is easy to miss in an engine that builds its own
tasks.
Removing `default` keeps the `with_file_size_in_bytes_opt` fallback and
doesn't change the generated builder's type in `public-api.txt`. The only
callers that need updating are the two test builders that omit the size, in
[`test_delete_file_without_size_round_trips_through_serde`](https://github.com/apache/iceberg-rust/blob/514ee8316ee7578dbcac345b68e6cd6c4e088670/crates/iceberg/src/arrow/caching_delete_file_loader.rs#L850-L857)
and
[`without_size`](https://github.com/apache/iceberg-rust/blob/514ee8316ee7578dbcac345b68e6cd6c4e088670/crates/iceberg/src/arrow/caching_delete_file_loader.rs#L1247-L1250),
which would each add `.with_file_size_in_bytes_opt(None)`. The builder line in
the description would change to match.
```suggestion
/// The total size of the delete file in bytes. Tasks planned from
manifest entries carry
/// the recorded size. An externally constructed task may pass `None`
when the size is
/// unavailable or the recorded size can't be trusted, such as position
delete files
/// rewritten by `rewrite_table_path` before Iceberg Java 1.12.0
/// (<https://github.com/apache/iceberg/issues/12554>). The Parquet
delete-file loader then
/// resolves it with one metadata request when the file is first loaded.
Deletion vectors
/// do not use it.
#[serde(default)]
#[serde(skip_serializing_if = "Option::is_none")]
#[builder(setter(strip_option(fallback = with_file_size_in_bytes_opt)))]
```
--
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]
