feat(scan): optionally retain column statistics on FileScanTask by anuragmantri · Pull Request #3424 · apache/iceberg-rust

anuragmantri · 2026-10-12T22:55:36Z

Which issue does this PR close?

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. Names must be primitive fields, which may be nested; a struct, list, map or metadata column name fails build(), because those have no statistics of their own. Each planned task then carries a new FileScanTaskColumnStats with the file's column sizes, value, null and NaN counts, and lower and upper bounds for only those field ids. The count and bound 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. Bounds serialize as their type plus the spec's single-value binary encoding, as the DataFile JSON serde does, because the JSON encoding of single values does not round-trip every float (#3409). 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 returns no stats and an empty request returns empty 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 and metadata columns fail. Unit tests check that a nested primitive name resolves and a struct name fails. Another test writes the manifest with a column as int while the table says long, and checks the bound stays an int. The serde round trip with all optional fields covers column sizes and string, decimal, non-exact float and infinite 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, name validation and serialization, and each change made a test fail.

Reviewers may want to look at two choices. Nested primitive field names are accepted, unlike select, because manifests record stats for nested leaves. An empty column list gives each task empty statistics, as select_empty does.

AI Disclosure

I used Claude Code (Claude Opus 5.5) to write this change and its tests, and I reviewed it manually.

Add TableScanBuilder::with_column_stats, mirroring Java's Scan.includeColumnStats. The named columns resolve to field ids at build time with the scan's case sensitivity, and each planned FileScanTask carries a FileScanTaskColumnStats holding the data file's value, null and NaN counts and lower and upper bounds for only those field ids. Retention is opt-in: by default column_stats is None, nothing is copied from the manifest entry, and the field is omitted when a task is serialized, so serialized tasks are unchanged.

JeonDaehong

Thanks, this will be useful for #3343.

One thing I hit while round-tripping the new stats through serde: a float/double bound of ±Infinity (or NaN) serializes to "literal":null with serde_json, and deserializing it fails with None value at line 1 .... Every other primitive type round-trips (bool through decimal, including fixed, uuid and timestamp_ns).

±Infinity is a valid bound: the spec only excludes NaN, and Java's FloatFieldMetrics keeps infinite values as bounds. So a task for a file containing an infinite float would serialize but not deserialize. The root cause is Datum's serde rather than this PR, but this PR is what puts file bounds into serialized tasks. Could we add an infinite-bound case to the serde test and either fix it in Datum or document it?

Bounds went through Datum's JSON encoding, which writes non-finite floats as null and rejects many finite float values when reading them back, so a task with such a bound serialized but failed to deserialize. Serialize each bound as its type and the spec's single-value binary encoding, as the DataFile JSON serde already does, so every bound round-trips exactly.

comphead · 2026-10-13T20:30:32Z

    partition_spec: Option<PartitionSpecRef>,
    unified_partition_type: Option<Arc<StructType>>,
    sort_orders: Arc<HashMap<i64, SortOrderRef>>,
+    column_stats_field_ids: Option<Arc<Vec<i32>>>,


should ids be u32?

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.

comphead · 2026-10-13T20:32:50Z


+    /// 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);


I'd prob prefer a typed structure instead of tuple type.... unless tuple type is cheaper than struct

nvm, its for test

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.

comphead · 2026-10-13T20:34:33Z

    }
 }

+/// Column statistics of a [`FileScanTask`]'s data file, from its manifest entry.


I feel the comments can be concised here

Trimmed in 5345a45 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.

comphead

Overall the opt-in design looks sound and matches Java's includeColumnStats for the maps it copies. I found no correctness issues. A few small points below.

Question: Java's BaseFile(copy, requestedColumnIds) also keeps column_sizes for the requested ids. Is leaving it out here intentional, for example because FileMetrics in #3347 does not carry it? If so, a sentence in the FileScanTaskColumnStats docs would help.

comphead · 2026-10-13T20:38:10Z

+
+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> {


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.

Done in 5345a45. An empty map now returns before any lookups, and the result is sized with field_ids.len().min(map.len()).

comphead · 2026-10-13T20:38:10Z

        .collect()
 }

+fn collect_column_stats_field_ids(


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.

Done in 5345a45. 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.

comphead · 2026-10-13T20:38:10Z

    partition_spec: Option<PartitionSpecRef>,
    unified_partition_type: Option<Arc<StructType>>,
    sort_orders: Arc<HashMap<i64, SortOrderRef>>,
+    column_stats_field_ids: Option<Arc<Vec<i32>>>,


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.

Switched to Option<Arc<[i32]>> in all three contexts in 5345a45.

comphead · 2026-10-13T20:38:10Z

+    }
+
+    #[tokio::test]
+    async fn test_plan_files_retains_requested_column_stats() {


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.

Added test_collect_column_stats_field_ids_resolves_nested_field in 5345a45, which resolves s.f to the struct child's field id.

anuragmantri

@JeonDaehong thanks for catching this. I reproduced it, and it's a little wider than infinite bounds: Datum's JSON path also rejects many finite float values, such as Datum::float(100.1), because the float conversion compares against an absolute f32::EPSILON. That's the same root cause as #3409.

In e0f3919, bounds now serialize as their type plus the spec's single-value binary encoding, as the DataFile JSON serde already does, so every bound round-trips exactly. The serde test now covers a float of 100.1 and infinite float and double bounds. Partition values and predicate literals still go through the JSON path, so I've left those to #3409.

anuragmantri · 2026-10-13T20:34:34Z

    partition_spec: Option<PartitionSpecRef>,
    unified_partition_type: Option<Arc<StructType>>,
    sort_orders: Arc<HashMap<i64, SortOrderRef>>,
+    column_stats_field_ids: Option<Arc<Vec<i32>>>,


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.

anuragmantri · 2026-10-13T20:34:35Z


+    /// 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);


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.

Retain column_sizes for the requested ids too, matching Java's BaseFile(copy, requestedColumnIds). Share the column-name lookup and its error with column selection, keep the requested names as an Option like column_names so an empty list yields empty statistics, and pass the ids as Arc<[i32]>. Skip work for an empty map when filtering, trim the struct docs, and cover nested field names.

anuragmantri

@comphead thanks for the review. Leaving out column_sizes wasn't intentional. I had limited the maps to what FileMetrics reads, but there's no reason to differ from Java, so 5345a45 also keeps column_sizes for the requested ids. The other points are addressed inline.

anuragmantri · 2026-10-13T20:49:52Z

    }
 }

+/// Column statistics of a [`FileScanTask`]'s data file, from its manifest entry.


Trimmed in 5345a45 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.

anuragmantri · 2026-10-13T20:49:53Z

+
+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> {


Done in 5345a45. An empty map now returns before any lookups, and the result is sized with field_ids.len().min(map.len()).

anuragmantri · 2026-10-13T20:49:54Z

        .collect()
 }

+fn collect_column_stats_field_ids(


Done in 5345a45. 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.

anuragmantri · 2026-10-13T20:49:56Z

    partition_spec: Option<PartitionSpecRef>,
    unified_partition_type: Option<Arc<StructType>>,
    sort_orders: Arc<HashMap<i64, SortOrderRef>>,
+    column_stats_field_ids: Option<Arc<Vec<i32>>>,


Switched to Option<Arc<[i32]>> in all three contexts in 5345a45.

anuragmantri · 2026-10-13T20:49:57Z

+    }
+
+    #[tokio::test]
+    async fn test_plan_files_retains_requested_column_stats() {


Added test_collect_column_stats_field_ids_resolves_nested_field in 5345a45, which resolves s.f to the struct child's field id.

JeonDaehong · 2026-10-14T10:45:40Z

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.

JeonDaehong

LGTM. Read through the full diff: opt-in default keeps existing serialization unchanged, stats are filtered to the requested ids, and the binary bound encoding round-trips. Thanks @anuragmantri!

…mn-stats-f19b86d2 # Conflicts: # crates/iceberg/src/scan/mod.rs

JeonDaehong

A second pass after my approval, none of these are blocking. One thing I checked that works better than Java: Java's includeColumnStats(columns) resolves names with schema.findField, so it is always case-sensitive and throws a NullPointerException on an unknown name. Here names follow the scan's case sensitivity and an unknown name fails build() with a clear error.

JeonDaehong · 2026-10-16T06:41:03Z

+    ///
+    /// [`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(


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.

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.

JeonDaehong · 2026-10-16T06:41:03Z

+    /// [`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.


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.

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.

JeonDaehong · 2026-10-16T06:41:03Z

        .collect()
 }

+fn collect_column_stats_field_ids(


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.

Agreed. In fe1bf907a, a metadata column now fails with Metadata column _file has no column statistics, and the planning test asserts that message.

JeonDaehong · 2026-10-16T06:41:03Z

+///
+/// 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


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.

Added test_plan_files_keeps_bounds_typed_with_manifest_schema in fe1bf90. 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.

Struct, list and map fields have no statistics of their own, and metadata columns are not stored in data files, so requesting stats for them silently returned empty maps or failed with a misleading "not found" error. Fail build() with an error that says why. Also cover bounds that keep the manifest's type after a type promotion.

JeonDaehong · 2026-10-17T02:09:17Z

Thanks @anuragmantri, all four look good to me. I checked fe1bf90 and the scan tests pass. A with_all_column_stats() follow-up sounds fine.

JeonDaehong

Checked fe1bf90, all four look good. Thanks for adding the tests.

github-actions Bot added the datafusion label Oct 12, 2026

JeonDaehong reviewed Oct 13, 2026

View reviewed changes

comphead reviewed Oct 13, 2026

View reviewed changes

anuragmantri commented Oct 13, 2026

View reviewed changes

JeonDaehong approved these changes Oct 14, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/main' into file-scan-task-colu…

d689791

…mn-stats-f19b86d2 # Conflicts: # crates/iceberg/src/scan/mod.rs

JeonDaehong reviewed Oct 16, 2026

View reviewed changes

This was referenced Oct 16, 2026

Bound the sorted-scan merge by how many files' key ranges overlap apache/datafusion-iceberg#71

Open

[Epic] Sort-order reporting for iceberg-datafusion scans #3126

Open

JeonDaehong approved these changes Oct 17, 2026

View reviewed changes

Conversation

anuragmantri commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

Uh oh!

JeonDaehong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuragmantri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuragmantri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeonDaehong commented Oct 14, 2026

Uh oh!

JeonDaehong left a comment

Choose a reason for hiding this comment

Uh oh!

JeonDaehong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuragmantri commented Oct 12, 2026 •

edited

Loading