Why isn't Dataset::count_rows() optimized when row counts are already in the manifest?

## Why isn't `Dataset::count_rows()` optimized when row counts are already in the manifest? ### Summary When running `SELECT COUNT(*) FROM lance_table` against a dataset with ~1k fragments and a few hundred deletion files, the query is dominated by metadata I/O rather than data I/O. Looking at the implementation, it seems like a near-zero-cost fast path is possible but not implemented. ### What I observed On a lance table with **999 data fragments** and **603 deletion files** (total ~57k rows), wrapped behind a DataFusion `TableProvider`: | Operation | Time | |----------------------------------------|-----------| | `COUNT(*)` end-to-end (cold) | ~11 s | | Data page scan (post-fix, scan folded) | ~0 s | | `LanceTable::try_new` (load manifest) | ~5.8 s | | `Dataset::count_rows` traversal | ~5.9 s | (The 11s is via a local process over a port-forwarded tunnel; on the cluster LAN the same path was historically ~3 s before the scan was eliminated.) So **counting dominates the query even though no data is read**. ### What the code currently does In `lance/src/dataset.rs` (verified identical on `7.0.0`, `8.0.0`, `9.0.0`, `10.0.0` — diff is empty): ```rust pub(crate) async fn count_all_rows(&self) -> Result<usize> { let cnts = stream::iter(self.get_fragments()) .map(|f| async move { f.count_rows(None).await }) .buffer_unordered(16) .try_collect::<Vec<_>>() .await?; Ok(cnts.iter().sum()) } ``` And `Fragment::count_rows` (also identical across versions): ```rust None => { let total_rows = self.physical_rows(); let deletion_count = self.count_deletions(); let (total_rows, deletion_count) = futures::future::try_join(total_rows, deletion_count).await?; Ok(total_rows - deletion_count) } ``` `Fragment::physical_rows` already has a fast path that returns `self.metadata.physical_rows` when `manifest.writer_version.is_some()`. So per fragment, the "cheap" path is at minimum: read the in-memory fragment metadata + check whether a deletion file exists + (if a deletion file exists without `num_deleted_rows` cached) load the deletion vector. ### Why this seems optimizable The manifest already contains every fragment's `physical_rows` (see `lance-table/src/format/fragment.rs`: `pub physical_rows: Option<usize>` — populated on write since the writer_version was bumped). So for fragments with **no deletions**, the exact row count is already in memory after the manifest is loaded — no per-fragment async work needed at all. For fragments with deletions, only those need a `count_deletions()` call, and the deletion count is often already cached as `deletion_file.num_deleted_rows` in the manifest (no I/O either). ### Concrete suggestion A cheap fast path could look like: ```rust pub(crate) async fn count_all_rows(&self) -> Result<usize> { let mut total = 0usize; let mut need_deletion_check = Vec::new(); for (i, f) in self.get_fragments().iter().enumerate() { match (f.metadata.physical_rows, &f.metadata.deletion_file) { (Some(rows), None) => total += rows, // pure manifest, O(1) (Some(rows), Some(d)) if d.num_deleted_rows.is_some() => { total += rows - d.num_deleted_rows.unwrap(); // cached deletion, O(1) } _ => need_deletion_check.push(i), // needs S3 / async } } if need_deletion_check.is_empty() { return Ok(total); } // Existing parallel path only for fragments that actually need it ... } ``` For our dataset (999 fragments, ~603 with deletion files), this would drop the async fragment fan-out from ~999 concurrent tasks to ~603, and for the "manifest-only" path it eliminates the per-fragment future + `buffer_unordered` scheduling entirely. ### Why I'm asking the community Before I write a PR, I want to make sure: 1. Is there a known reason `count_all_rows` doesn't take this path already? (e.g., older datasets without `writer_version` having `physical_rows=None`? trust concerns about the cached value?) 2. Is this intentionally left as-is because someone is already working on it? (PRs / issues I should look at?) 3. Is there a public benchmark / perf suite I should add to when submitting the change? I checked `7.0.0` → `10.0.0` and the implementation is byte-identical, so if there's appetite for this it would be a clean, low-risk change. ### Reproduction sketch ```rust // Build a lance dataset with N fragments and D deletion files. // (In our case it arose naturally from INSERT/DELETE-heavy static views.) let dataset = Dataset::open("s3://bucket/lance/view-uuid").await?; let start = std::time::Instant::now(); let n = dataset.count_rows(None).await?; println!("count = {n} in {:?}", start.elapsed()); ``` ### Environment - `lance` 7.0.0 (also checked 8/9/10 — same code) - `datafusion` 53 (this question is independent of DF integration; same cost whether we go through the DF `TableProvider` or call directly)