mbutrovich commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4395060067
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -137,6 +138,12 @@ pub struct FileScanTask {
#[builder(default)]
sort_order: Option<SortOrderRef>,
+ /// The data file's column statistics for the columns requested with
+ ///
[`TableScanBuilder::with_column_stats`](crate::scan::TableScanBuilder::with_column_stats).
+ /// `None` if no statistics were requested.
+ #[builder(default)]
+ column_stats: Option<FileScanTaskColumnStats>,
Review Comment:
Could this be `Option<Box<FileScanTaskColumnStats>>`? Nothing is allocated
when the field is `None`, but the field reserves room for six `HashMap`s
inline, 288 bytes. So `FileScanTask` grows from 352 to 640 bytes on every scan,
including scans that never call `with_column_stats`. I measured that with
`size_of` on ba849a6 (the merge base) and fe1bf90. #3422 says the feature
"should add nothing when it isn't used".
Boxing keeps the field at 8 bytes. The getter can return
`self.column_stats.as_deref()`, and a `transform` on the builder setter keeps
its signature, so the public API stays the same:
```suggestion
#[builder(default, setter(transform = |stats:
Option<FileScanTaskColumnStats>| stats.map(Box::new)))]
column_stats: Option<Box<FileScanTaskColumnStats>>,
```
The serializer at line 574 would change to `value.column_stats.as_deref()`
too. The cost is one more allocation per task when stats are requested, next to
the six maps that path already builds.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -211,6 +248,27 @@ impl<'a> TableScanBuilder<'a> {
self
}
+ /// Retains the data file's column statistics for the named columns on
each planned
+ /// [`FileScanTask`], exposed as [`FileScanTask::column_stats`].
+ ///
+ /// Names resolve against the scan's schema with the scan's case
sensitivity and must name
+ /// primitive fields, which may be nested. By default, tasks carry no
statistics.
+ ///
+ /// [`build`](Self::build) fails if a name matches no field, matches more
than one field when
+ /// the scan is case-insensitive, or names a struct, list, map or metadata
column.
+ pub fn with_column_stats(
Review Comment:
Could `with_column_stats` get a doctest, like
[`as_of_time`](https://github.com/apache/iceberg-rust/blob/de0a6ac8471da43f63382d66fac27d7e56fd273e/crates/iceberg/src/scan/mod.rs#L232-L255)
on `main`? It's the entry point engines will copy, and an example that reads a
bound from `task.column_stats()` would show the field-id keying and the `None`
default in one place.
--
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]
