JeonDaehong commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4291636612
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -211,6 +232,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 may name
+ /// nested fields. By default, tasks carry no statistics.
+ ///
+ /// [`build`](Self::build) fails if a name matches no field, or more than
one field when the
+ /// scan is case-insensitive.
+ pub fn with_column_stats(
Review Comment:
Java has a no-argument `includeColumnStats()` that keeps the stats of every
column (`BaseFile` copies all maps when `requestedColumnIds` is null). Here the
only way to get all of them is to list every leaf name. Is that intended? If
engines need it, a `with_all_column_stats()` could come in a follow-up.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -211,6 +232,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 may name
+ /// nested fields. By default, tasks carry no statistics.
Review Comment:
Naming a struct (or list/map) column resolves to the parent's field id,
which never has stats, so `with_column_stats(["s"])` gives empty maps without
an error. I checked: `s: struct<f>` resolves to `[1]` and the stats come back
empty while `f` (id 2) has them. Java behaves the same, so I'd either reject
non-primitive fields at `build()` or say here that only leaf names return
statistics.
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -760,6 +826,106 @@ impl From<FileScanTaskDeleteFile> for
Result<FileScanTaskDeleteFile> {
}
}
+/// Column statistics of a [`FileScanTask`]'s data file, keyed by field id and
limited to the
+/// columns requested for the scan.
+///
+/// A requested column missing from a map has unknown statistics of that kind,
for example when
+/// the table's metrics mode records no bounds. Bounds are inclusive even when
truncated, and are
+/// typed with the schema the manifest was written with, so after type
promotion an `int` bound
Review Comment:
This promises that after type promotion an `int` bound can describe a `long`
column, which is a useful thing for callers to know, but no test pins it. A
manifest written with an `int` column, read after promoting it to `long`,
asserting the bound is still `Datum::int`, would keep it true.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -103,6 +107,21 @@ fn collect_scan_field_ids(
.collect()
}
+fn collect_column_stats_field_ids(
Review Comment:
nit, optional: `select` accepts metadata columns like `_file`, but here they
fail with `Column _file not found in table`, which reads like a typo in the
name. A message saying metadata columns have no statistics would be clearer.
--
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]
