bench(reader): add ArrowReader benchmark harness

[viirya]

Which issue does this PR close?

• Closes Add an in-repo benchmark harness for ArrowReader #2557.

What changes are included in this PR?

Adds a criterion-based benchmark harness for the ArrowReader at crates/iceberg/benches/arrow_reader.rs. Until now there were no in-repo reader benchmarks, so every performance claim in the perf epic (#2172) had to be validated against external workloads. This harness writes Parquet files to a local temp dir and reads them back through the normal FileIO path, measuring the per-FileScanTask overhead that dominates scans of tables with many small files. Because it runs on the local FS, it isolates CPU / per-task work rather than network latency.

Benchmark groups, chosen to map onto the epic's code paths:

• many_small_files — scans 16/64/256 small files, reporting files/sec so per-file overhead is directly visible.
• concurrency — a fixed corpus read at concurrency 1/4/16, exercising both the single-concurrency fast path and the buffered/flattened multi-task path.
• migrated_table — files without embedded field IDs read via name mapping, isolating the migrated-table schema-resolution cost (perf(reader): Avoid second create_parquet_record_batch_stream_builder() call for migrated tables #2176 path).
• same_file_splits — one multi-row-group file read as 1/8/32 byte-range tasks, surfacing the redundant per-split metadata fetch that the same-file metadata caching proposed in Epic: ArrowReader performance improvements for split FileScanTasks #2172 (attempted in feat(reader): cache Parquet metadata for when FileScanTasks read the same file #2100) targets.
• with_predicate — scans carrying a bound predicate with row-group filtering and row selection enabled, exercising the per-task row-filter setup.

This adds criterion (with the async_tokio feature) as a workspace dev-dependency and a [[bench]] entry to the iceberg crate.

Measuring bytes_read, not just time

Wall-clock time on the local FS cannot show the benefit of optimizations that save round-trips (e.g. the metadata size hint #2173 or range coalescing #2181) — those need a latency-bearing backend, which is left as follow-up. But the same-file-split problem is different: its cost is redundant bytes fetched, not latency, so it is measurable even locally.

The same_file_splits group therefore reports ScanMetrics::bytes_read as its throughput basis (a deterministic count, not a sampled measurement) and prints the read amplification per split count. On a ~1.2 MiB file:

same_file_splits/1:  bytes_read = 1,694,258  (1.43x file size)
same_file_splits/8:  bytes_read = 5,529,051  (4.66x file size)
same_file_splits/32: bytes_read = 18,674,303 (15.72x file size)

Reading one file as 32 byte-range splits fetches ~15.7x the file size, because each split re-fetches the Parquet metadata independently. This is machine-independent evidence for the cost that same-file metadata caching targets — #2100 was closed partly because its author could not demonstrate a benefit on their workload (a ~1:1 task-to-file table, where same-file caching has nothing to hit); with this benchmark a caching implementation would show the amplification dropping back toward 1x.

Run with:

cargo bench -p iceberg --bench arrow_reader

Are these changes tested?

This is a benchmark, not a code change. The harness compiles and runs (cargo bench -p iceberg --bench arrow_reader), and reuses the same Parquet-writing and reader-driving patterns as the existing reader unit tests. cargo fmt and cargo clippy -p iceberg --benches are clean.

[mbutrovich]

Thanks for doing this @viirya! This would have made my life working on #2172 much easier. I will take a proper pass through it today.

[mbutrovich]

The harness fills a real gap: there was no in-repo reader benchmark, and the deterministic data plus per-group mapping to epic items is a good setup. Three notes on what it measures, none blocking:

1. Coverage. It runs on FileIO::new_with_fs(), so the object-store per-task costs the epic targets (operator creation, stat(), TLS, credential init) are absent. Operator caching (#2177) will look near-flat locally, for the same reason the latency items (#2173, #2181) are not measurable here. Add a "what this cannot measure" line to the module doc so a flat local run is not read as "the optimization does not help" (part of why #2100 was closed).

2. Correctness unverified. No group checks the rows returned. A bug that reads less data, such as the #2614 sub-row-group-split duplication that lives in the same_file_splits path, times faster and looks like a win. Assert the expected total row count per group.

3. Warm vs cold. criterion plus the OS page cache means the time-based groups measure warm-cache CPU, not cold first-touch reads (closest to per-file object-store GETs). State the regime in the doc. same_file_splits/bytes_read is a deterministic count and is unaffected.

[mbutrovich]

The doc lists "operator construction" as measured, but on the Fs backend that has no credential-provider init, signer, or TLS, which is the expensive part operator caching (#2177) targets, so it looks near-flat here. The comment should say explicitly that operator creation, stat(), TLS, credential init, and the latency items (#2173, #2181) are not measurable on local FS, and that the time-based groups measure warm-cache CPU (files sit in the OS page cache after the first sample), not cold first-touch reads. What this measures is the CPU / decode / schema-resolution path.

[mbutrovich]

read_all never checks how many rows came back, so a config that reads 0 rows or duplicates rows (the #2614 sub-row-group-split bug is in the same_file_splits path) would not error and would report inflated throughput. Assert the total rows read equals the expected total before black_box; that also makes same_file_splits a regression guard for #2614.

[mbutrovich]

Using ScanMetrics::bytes_read as the throughput basis makes the redundant-metadata-fetch cost visible without a latency backend and is deterministic across machines. This is the measurement #2100 lacked.

[mbutrovich]

Fine for per-task overhead, but timestamp / decimal / nested / INT96 decode paths are not exercised, so column-decode regressions will not show. Worth one line in the doc marking the covered space.

[blackmwk]

Thanks @viirya for this pr, but I don't think it's a good time to add this benchmark harness for ArrowReader now. I'm expecting to add this after we finished file format api.

[viirya]

Thanks — all three are now in (a3fe113, after a rebase onto current main):

1. Doc scope. The module doc now has explicit "What this measures" / "What this cannot measure" sections: warm-cache CPU/decode regime (files sit in the page cache after the first sample), no credential init / signing / TLS / network on local FS so operator caching (perf(reader): cache OpenDAL Operators #2177) will look near-flat and the latency items (perf(reader): Add Parquet metadata size hint option to ArrowReaderBuilder #2173, perf(reader): Implement AsyncFileReader get_byte_ranges and coalesce close ranges #2181) are not measurable at all, with the explicit warning that a flat local result must not be read as "no benefit" — plus a note that only Int64/Utf8 decode is exercised. The "operator construction" phrasing you flagged is gone.

2. Row-count assertions — and they immediately earned their keep. Both read_all and read_all_bytes now assert the exact expected row count per group. On this PR's old base (which predated the filter_row_groups_by_byte_range duplicates rows for sub-row-group file splits #2614 fix), the very first run of same_file_splits/8 failed the new assertion with 114,000 rows instead of 100,000 — 7 split boundaries each double-reading a 2,000-row row group. Exactly the failure mode you described: it had been silently timing faster. After rebasing onto main (which carries the filter_row_groups_by_byte_range duplicates rows for sub-row-group file splits #2614 fix) all groups pass, so same_file_splits is now a live regression guard for that bug.

3. Warm vs cold is stated in the doc as above; bytes_read remains the deterministic count and is unaffected.

[viirya]

Thanks for taking a look @blackmwk. I'd like to make the case for landing it before the file format API rather than after, and see what you think:

• A baseline is most useful before a refactor. The main value of having this in-repo now is that when the file format API lands, we can run the same benches on both sides and see exactly what the abstraction costs (or saves). Added afterwards, we lose the before picture.
• The harness measures behavior, not internals. The benches drive ArrowReader through FileScanTasks and observe wall time, ScanMetrics::bytes_read, and (as of the latest push) asserted row counts. None of that couples to reader internals, so the file-format-api refactor should mostly leave the benches untouched — they're exactly the thing that tells us the refactor didn't regress.
• It is already paying for itself. The row-count assertion added from mbutrovich's review immediately caught the filter_row_groups_by_byte_range duplicates rows for sub-row-group file splits #2614 duplication on this PR's old base (114k rows instead of 100k on same_file_splits/8), and the epic Epic: ArrowReader performance improvements for split FileScanTasks #2172 items it was built to measure are in flight now, not after the file format API.

That said, if you'd still prefer to hold it, I'm happy to park this as a draft until the file format API settles — I'd just rather not lose the pre-refactor baseline window. WDYT?

[blackmwk]

Hi, @viirya My main concern comes from the maintaining effort. Since we are going to do a large refactor, adding such things will introduce extra maintain efforts for both reviewers and contributors. My suggestion is that we keep this pr open, and anyone interested in it could run it locally.

[viirya]

That makes sense — I was focused on the CI/build-time cost, but you're right that's not really the concern. The real cost is whoever touches reader internals during the refactor having to understand and fix a benchmark they didn't write, and that shouldn't be pushed onto them.

Let's keep this PR open rather than merge it. I'll treat it as ours to maintain — watching for reader-internal changes and updating the benchmark to match — and revisit merging once the file format API work settles. Thanks for pushing on this.