You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "anuragmantri (via GitHub)" <gi...@apache.org> on 2026/10/12 22:55:39 UTC
[PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
anuragmantri opened a new pull request, #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424
## Which issue does this PR close?
- Closes #3422.
- Part of #3343.
- Related to #3126.
## What changes are included in this PR?
Planning drops a data file's column statistics when it builds a `FileScanTask`. Engines that want to order or merge files by key range, such as the sorted scan work in apache/datafusion-iceberg#55 and apache/datafusion-comet#5343, need per-file bounds on the sort columns. This PR lets a scan keep them, mirroring Java's `Scan.includeColumnStats`.
`TableScanBuilder::with_column_stats(columns)` takes column names and resolves them to field ids in `build()` with the scan's case sensitivity. An unknown name fails the same way `select` does, as does a name that matches more than one field when the scan is case-insensitive. Each planned task then carries a new `FileScanTaskColumnStats` with the file's value, null and NaN counts and lower and upper bounds for only those field ids. The maps have the same names and types as the crate-internal `FileMetrics` in #3347, so the two convert directly.
The feature is opt-in, following the concerns raised in #607. By default `FileScanTask::column_stats()` is `None`, planning copies nothing from the manifest entry, and serialization leaves the field out, so tasks without stats serialize exactly as they do today. Nothing from `key_metadata` or other `DataFile` fields is exposed.
The docs spell out what the statistics mean. Bounds are inclusive and may be truncated. Any of them can be missing. They describe the whole file, with no adjustment for deletes, the filter or splits. They are typed with the schema the manifest was written with, so after type promotion a bound can be an `int` on a `long` column.
Public API additions are `TableScanBuilder::with_column_stats`, `FileScanTask::column_stats` and the `FileScanTaskColumnStats` struct with its getters and builder.
## Are these changes tested?
Yes. A new fixture writes manifest entries with statistics on several columns, where one file has no statistics for `x`. The scan test checks that the default scan and an empty request return no stats, that requesting `x` and `dbl` keeps only those entries in every map while `x` stays missing for the second file, that case-insensitive resolution works, and that unknown names fail in both case modes. The serde round trip with all optional fields now includes string and decimal bounds, and a new test checks that a task without stats serializes no `column_stats` key and reads back as `None`. I also mutated the filtering, the default, case handling, unknown-name handling and serialization, and each change made a test fail.
Reviewers may want to look at two choices. Nested field names are accepted, unlike `select`, because manifests record stats for nested leaves. An empty column list is treated as not requested.
## AI Disclosure
I used Claude Code (Claude Opus 5.5) to write this change and its tests, and I reviewed it manually.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4395046068
##########
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.
Should planning convert the retained bounds to the scan schema's type instead, so that one scan returns one type per field id?
As I read it, a bound's type depends on which manifest holds the entry, not only on when the data file was written. A file listed in a manifest written before `y` was promoted gets `Datum::int`. A file listed in a manifest written after the promotion gets `Datum::long`. Java's merge append writes old entries into such manifests: [`ManifestMergeManager`](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/core/src/main/java/org/apache/iceberg/ManifestMergeManager.java#L195-L214) copies them with `writer.existing(entry)` into a writer whose spec is [rebound to the current schema](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/core/src/main/java/org/apache/iceberg/TableMetadata.java#L1105-L1107), and the bound bytes stay 4 bytes. The type changes on read because [`Datum::try_from_bytes`](https://github.com/apache/iceberg-rust/blob/de0a6ac8471da43f63382d66fac27d7e56fd273e/crates/iceberg/src/spec/values/datum.rs#L401-L408) wi
dens 4-byte values when the manifest schema says `long`. So one scan can hand out both types for the same column. [`Datum`'s `PartialOrd`](https://github.com/apache/iceberg-rust/blob/de0a6ac8471da43f63382d66fac27d7e56fd273e/crates/iceberg/src/spec/values/datum.rs#L178-L294) has no cross-type arms, so comparing them returns `None`. Ordering files by their bounds is the comparison a sorted scan makes.
I checked this on fe1bf90 with a scratch test that writes two manifests, one with `y: int` and one with `y: long`, and scans with `with_column_stats(["y"])`. The lower bounds come back as `Datum { type: Int, literal: Int(5) }` and `Datum { type: Long, literal: Long(6) }`, and `partial_cmp` between them returns `None`.
The spec's [promotion table](https://github.com/apache/iceberg/blob/6275107e38cc87a7d416ef231019ceb51556f196/format/spec.md?plain=1#L405-L417) describes decoding a bound against the current type. `Datum::to` already converts `int` to `long` and `float` to `double`, and decimals of different precision compare correctly today. `date` to `timestamp` (v3) would need its own conversion, or the bound could be dropped, since a missing bound already means unknown. If keeping the manifest's type is intended, what do you think about adding the two-manifest case to `test_plan_files_keeps_bounds_typed_with_manifest_schema` and saying in the docs that callers must convert bounds before comparing them across tasks?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4263868741
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -760,6 +826,96 @@ impl From<FileScanTaskDeleteFile> for Result<FileScanTaskDeleteFile> {
}
}
+/// Column statistics of a [`FileScanTask`]'s data file, from its manifest entry.
+///
+/// Each map is keyed by field id and holds only the columns requested for the scan. A
+/// requested column missing from a map has unknown statistics of that kind: metrics mode
+/// `none` records nothing for the column, while metrics mode `counts` and an all-null column
+/// record counts but no bounds.
+///
+/// Bounds are inclusive. A truncated lower bound is still less than or equal to every value in
+/// the file, and a truncated upper bound is still greater than or equal to every value. Bounds
+/// are typed with the schema the manifest was written with, so after type promotion a bound
+/// can be an `int` while the scan schema says `long`.
+///
+/// The statistics describe the whole data file: they are not adjusted for delete files, the
+/// scan's filter, or the byte range of a split task.
+#[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize, TypedBuilder)]
+#[builder(field_defaults(default, setter(prefix = "with_")))]
+pub struct FileScanTaskColumnStats {
+ /// Number of values per field id, including nulls and NaNs.
+ #[serde(default, skip_serializing_if = "HashMap::is_empty")]
+ value_counts: HashMap<i32, u64>,
+
+ /// Number of null values per field id.
+ #[serde(default, skip_serializing_if = "HashMap::is_empty")]
+ null_value_counts: HashMap<i32, u64>,
+
+ /// Number of NaN values per field id.
+ #[serde(default, skip_serializing_if = "HashMap::is_empty")]
+ nan_value_counts: HashMap<i32, u64>,
+
+ /// Inclusive lower bound per field id.
+ #[serde(
+ default,
+ skip_serializing_if = "HashMap::is_empty",
+ with = "crate::scan::task::_serde::bounds"
+ )]
+ lower_bounds: HashMap<i32, Datum>,
+
+ /// Inclusive upper bound per field id.
+ #[serde(
+ default,
+ skip_serializing_if = "HashMap::is_empty",
+ with = "crate::scan::task::_serde::bounds"
+ )]
+ upper_bounds: HashMap<i32, Datum>,
+}
+
+impl FileScanTaskColumnStats {
+ pub(crate) fn from_data_file(data_file: &DataFile, field_ids: &[i32]) -> Self {
+ fn retain<V: Clone>(map: &HashMap<i32, V>, field_ids: &[i32]) -> HashMap<i32, V> {
Review Comment:
Done in 5345a45c8. An empty map now returns before any lookups, and the result is sized with `field_ids.len().min(map.len())`.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -103,6 +103,26 @@ fn collect_scan_field_ids(
.collect()
}
+fn collect_column_stats_field_ids(
Review Comment:
Done in 5345a45c8. Both paths now share a `require_field_id` helper and its not-found error. I named it differently so it isn't confused with `Schema::field_id_by_name`. The requested names are now an `Option<Vec<String>>` like `column_names`, so an empty list means no columns and each task carries empty statistics, the same way `select_empty` works.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -1106,6 +1150,112 @@ mod tests {
);
}
+ /// One column's statistics: field id, value count, null count, NaN count, lower bound
+ /// and upper bound.
+ type ColumnStat = (i32, u64, u64, Option<u64>, Datum, Datum);
+
+ fn column_stats(columns: &[ColumnStat]) -> FileScanTaskColumnStats {
+ FileScanTaskColumnStats::builder()
+ .with_value_counts(columns.iter().map(|c| (c.0, c.1)).collect())
+ .with_null_value_counts(columns.iter().map(|c| (c.0, c.2)).collect())
+ .with_nan_value_counts(columns.iter().filter_map(|c| Some((c.0, c.3?))).collect())
+ .with_lower_bounds(columns.iter().map(|c| (c.0, c.4.clone())).collect())
+ .with_upper_bounds(columns.iter().map(|c| (c.0, c.5.clone())).collect())
+ .build()
+ }
+
+ async fn column_stats_by_file(scan: TableScan) -> Vec<Option<FileScanTaskColumnStats>> {
+ let mut tasks: Vec<_> = scan
+ .plan_files()
+ .await
+ .unwrap()
+ .try_collect()
+ .await
+ .unwrap();
+ tasks.sort_by(|a, b| a.data_file_path().cmp(b.data_file_path()));
+ tasks
+ .into_iter()
+ .map(|task| task.column_stats().cloned())
+ .collect()
+ }
+
+ #[tokio::test]
+ async fn test_plan_files_retains_requested_column_stats() {
Review Comment:
Added `test_collect_column_stats_field_ids_resolves_nested_field` in 5345a45c8, which resolves `s.f` to the struct child's field id.
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -760,6 +780,88 @@ impl From<FileScanTaskDeleteFile> for Result<FileScanTaskDeleteFile> {
}
}
+/// Column statistics of a [`FileScanTask`]'s data file, from its manifest entry.
Review Comment:
Trimmed in 5345a45c8 to what a caller needs: what a missing entry means, that bounds stay inclusive when truncated and are typed with the manifest's schema, and that the statistics cover the whole file.
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -52,6 +52,7 @@ pub(crate) struct ManifestFileContext {
partition_spec: Option<PartitionSpecRef>,
unified_partition_type: Option<Arc<StructType>>,
sort_orders: Arc<HashMap<i64, SortOrderRef>>,
+ column_stats_field_ids: Option<Arc<Vec<i32>>>,
Review Comment:
Switched to `Option<Arc<[i32]>>` in all three contexts in 5345a45c8.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "JeonDaehong (via GitHub)" <gi...@apache.org>.
JeonDaehong commented on PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#issuecomment-6228330953
Thanks @anuragmantri, all four look good to me. I checked fe1bf907a and the scan tests pass. A `with_all_column_stats()` follow-up sounds fine.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4263714984
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -1106,6 +1150,112 @@ mod tests {
);
}
+ /// One column's statistics: field id, value count, null count, NaN count, lower bound
+ /// and upper bound.
+ type ColumnStat = (i32, u64, u64, Option<u64>, Datum, Datum);
Review Comment:
nvm, its for test
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "JeonDaehong (via GitHub)" <gi...@apache.org>.
JeonDaehong commented on PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#issuecomment-6166574864
Thanks @anuragmantri! Storing bounds as the spec's binary encoding is a nice fix, since it now matches the DataFile serde. Good catch on finite floats like 100.1 too. Leaving partition values and literals to #3409 sounds right to me.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "JeonDaehong (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "alexanderbianchi (via GitHub)" <gi...@apache.org>.
alexanderbianchi commented on PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#issuecomment-6375664301
https://github.com/datafusion-contrib/datafusion-distributed/pull/789 Thanks for this work, I have a PR that'll use it. Will also apply to datafusion-iceberg.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "alexanderbianchi (via GitHub)" <gi...@apache.org>.
alexanderbianchi commented on PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#issuecomment-6452279099
@comphead is this PR in a state you are confident with approving after the iteration with @JeonDaehong? It's relevant for some datafusion work.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4263759122
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -103,6 +103,26 @@ fn collect_scan_field_ids(
.collect()
}
+fn collect_column_stats_field_ids(
Review Comment:
The resolve plus `Column {column_name} not found in table. Schema: {schema}` construction duplicates `collect_scan_field_ids`. A small `field_id_by_name(schema, name, case_sensitive) -> Result<i32>` used by both would keep the two error messages from drifting. Related: `column_names` is an `Option<Vec<String>>` and this is a `Vec<String>` where empty means none. An `Option` would follow the existing field and drop the `is_empty` special case.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -1106,6 +1150,112 @@ mod tests {
);
}
+ /// One column's statistics: field id, value count, null count, NaN count, lower bound
+ /// and upper bound.
+ type ColumnStat = (i32, u64, u64, Option<u64>, Datum, Datum);
+
+ fn column_stats(columns: &[ColumnStat]) -> FileScanTaskColumnStats {
+ FileScanTaskColumnStats::builder()
+ .with_value_counts(columns.iter().map(|c| (c.0, c.1)).collect())
+ .with_null_value_counts(columns.iter().map(|c| (c.0, c.2)).collect())
+ .with_nan_value_counts(columns.iter().filter_map(|c| Some((c.0, c.3?))).collect())
+ .with_lower_bounds(columns.iter().map(|c| (c.0, c.4.clone())).collect())
+ .with_upper_bounds(columns.iter().map(|c| (c.0, c.5.clone())).collect())
+ .build()
+ }
+
+ async fn column_stats_by_file(scan: TableScan) -> Vec<Option<FileScanTaskColumnStats>> {
+ let mut tasks: Vec<_> = scan
+ .plan_files()
+ .await
+ .unwrap()
+ .try_collect()
+ .await
+ .unwrap();
+ tasks.sort_by(|a, b| a.data_file_path().cmp(b.data_file_path()));
+ tasks
+ .into_iter()
+ .map(|task| task.column_stats().cloned())
+ .collect()
+ }
+
+ #[tokio::test]
+ async fn test_plan_files_retains_requested_column_stats() {
Review Comment:
The description calls out nested names as a deliberate difference from `select`, but no case here uses one. A nested leaf (for example a struct child, resolved by `field_id_by_name`) would pin that choice.
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -52,6 +52,7 @@ pub(crate) struct ManifestFileContext {
partition_spec: Option<PartitionSpecRef>,
unified_partition_type: Option<Arc<StructType>>,
sort_orders: Arc<HashMap<i64, SortOrderRef>>,
+ column_stats_field_ids: Option<Arc<Vec<i32>>>,
Review Comment:
`Option<Arc<Vec<i32>>>` is a double indirection. `Option<Arc<[i32]>>` reads the ids with one pointer hop and is built with `Arc::from(vec)`. Same for the field on `ManifestEntryContext` and `PlanContext`.
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -760,6 +826,96 @@ impl From<FileScanTaskDeleteFile> for Result<FileScanTaskDeleteFile> {
}
}
+/// Column statistics of a [`FileScanTask`]'s data file, from its manifest entry.
+///
+/// Each map is keyed by field id and holds only the columns requested for the scan. A
+/// requested column missing from a map has unknown statistics of that kind: metrics mode
+/// `none` records nothing for the column, while metrics mode `counts` and an all-null column
+/// record counts but no bounds.
+///
+/// Bounds are inclusive. A truncated lower bound is still less than or equal to every value in
+/// the file, and a truncated upper bound is still greater than or equal to every value. Bounds
+/// are typed with the schema the manifest was written with, so after type promotion a bound
+/// can be an `int` while the scan schema says `long`.
+///
+/// The statistics describe the whole data file: they are not adjusted for delete files, the
+/// scan's filter, or the byte range of a split task.
+#[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize, TypedBuilder)]
+#[builder(field_defaults(default, setter(prefix = "with_")))]
+pub struct FileScanTaskColumnStats {
+ /// Number of values per field id, including nulls and NaNs.
+ #[serde(default, skip_serializing_if = "HashMap::is_empty")]
+ value_counts: HashMap<i32, u64>,
+
+ /// Number of null values per field id.
+ #[serde(default, skip_serializing_if = "HashMap::is_empty")]
+ null_value_counts: HashMap<i32, u64>,
+
+ /// Number of NaN values per field id.
+ #[serde(default, skip_serializing_if = "HashMap::is_empty")]
+ nan_value_counts: HashMap<i32, u64>,
+
+ /// Inclusive lower bound per field id.
+ #[serde(
+ default,
+ skip_serializing_if = "HashMap::is_empty",
+ with = "crate::scan::task::_serde::bounds"
+ )]
+ lower_bounds: HashMap<i32, Datum>,
+
+ /// Inclusive upper bound per field id.
+ #[serde(
+ default,
+ skip_serializing_if = "HashMap::is_empty",
+ with = "crate::scan::task::_serde::bounds"
+ )]
+ upper_bounds: HashMap<i32, Datum>,
+}
+
+impl FileScanTaskColumnStats {
+ pub(crate) fn from_data_file(data_file: &DataFile, field_ids: &[i32]) -> Self {
+ fn retain<V: Clone>(map: &HashMap<i32, V>, field_ids: &[i32]) -> HashMap<i32, V> {
Review Comment:
This runs once per manifest entry and builds five maps. `collect()` over `filter_map` has a lower size hint of 0, so each map regrows. Consider `HashMap::with_capacity(field_ids.len().min(map.len()))` and returning early when `map` is empty (metrics mode `none` is common), so the empty case does no work.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4263709650
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -1106,6 +1150,112 @@ mod tests {
);
}
+ /// One column's statistics: field id, value count, null count, NaN count, lower bound
+ /// and upper bound.
+ type ColumnStat = (i32, u64, u64, Option<u64>, Datum, Datum);
Review Comment:
I'd prob prefer a typed structure instead of tuple type.... unless tuple type is cheaper than struct
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4299950544
##########
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:
Yes, that's intended for now. Naming the columns keeps the default cheap, which was the concern about task size in #607, and the engines I have in mind only need the sort columns. If one needs every column, a `with_all_column_stats()` is a small 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:
Good catch. In fe1bf907a, `build()` rejects a struct, list or map name with `Column s is not a primitive field and has no column statistics`, and the doc now says names must be primitive fields. `test_collect_column_stats_field_ids_rejects_struct_field` covers it.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -103,6 +107,21 @@ fn collect_scan_field_ids(
.collect()
}
+fn collect_column_stats_field_ids(
Review Comment:
Agreed. In fe1bf907a, a metadata column now fails with `Metadata column _file has no column statistics`, and the planning test asserts that message.
##########
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:
Added `test_plan_files_keeps_bounds_typed_with_manifest_schema` in fe1bf907a. It writes the manifest with `y` as an `int` while the table schema says `long`, and checks the bounds come back as `Datum::int`. This holds because the manifest reader parses bounds with the schema stored in the manifest.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4263688345
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -52,6 +52,7 @@ pub(crate) struct ManifestFileContext {
partition_spec: Option<PartitionSpecRef>,
unified_partition_type: Option<Arc<StructType>>,
sort_orders: Arc<HashMap<i64, SortOrderRef>>,
+ column_stats_field_ids: Option<Arc<Vec<i32>>>,
Review Comment:
should ids be u32?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4263726289
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -52,6 +52,7 @@ pub(crate) struct ManifestFileContext {
partition_spec: Option<PartitionSpecRef>,
unified_partition_type: Option<Arc<StructType>>,
sort_orders: Arc<HashMap<i64, SortOrderRef>>,
+ column_stats_field_ids: Option<Arc<Vec<i32>>>,
Review Comment:
I kept `i32` to match how field ids are typed everywhere else: the spec defines them as `int`, and `NestedField::id`, the `DataFile` stats maps this reads from, `FileScanTask::project_field_ids` and `FileMetrics` in #3347 all use `i32`. Switching here would mean converting at each of those boundaries.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -1106,6 +1150,112 @@ mod tests {
);
}
+ /// One column's statistics: field id, value count, null count, NaN count, lower bound
+ /// and upper bound.
+ type ColumnStat = (i32, u64, u64, Option<u64>, Datum, Datum);
Review Comment:
Right, it's only a test helper, so each column's expected stats fit on one line. Happy to switch to a small struct if that reads better.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4263726171
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -760,6 +780,88 @@ impl From<FileScanTaskDeleteFile> for Result<FileScanTaskDeleteFile> {
}
}
+/// Column statistics of a [`FileScanTask`]'s data file, from its manifest entry.
Review Comment:
I feel the comments can be concised here
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(scan): optionally retain column statistics on FileScanTask [iceberg-rust]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on code in PR #3424:
URL: https://github.com/apache/iceberg-rust/pull/3424#discussion_r4395387005
##########
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:
Added. It is compile-only, because there is no table with data files to run it against.
##########
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:
Agreed, and thanks for the trace through `ManifestMergeManager`. Planning now converts bounds to the column's type in the scan schema. The two-manifest case is a test.
A decimal is converted only to a wider precision at the same scale. Any other bound that differs in type is dropped, so it reads as unknown.
`date` to `timestamp` never gets this far. `Datum::try_from_bytes` fails on the 4-byte bound, so the manifest read errors before planning. That seems like a separate issue?
##########
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:
Done as you suggested. The setter and getter signatures are unchanged.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org