[claude] benchmarks v3 cleanup (#7745)

<!--
Thank you for submitting a pull request! We appreciate your time and
effort.

Please make sure to provide enough information so that we can review
your pull
request. The Summary and Testing sections below contain guidance on what
to
include.
-->

## Summary

<!--
If this PR is related to a tracked effort, please link to the relevant
issue
here (e.g., `Closes: #123`). Otherwise, feel free to ignore / delete
this.

In this section, please:

1. Explain the rationale for this change.
2. Summarize the changes included in this PR.

A general rule of thumb is that larger PRs should have larger summaries.
If
there are a lot of changes, please help us review the code by explaining
what
was changed and why.

If there is an issue or discussion attached, there is no need to
duplicate all
the details, but clarity is always preferred over brevity.
-->

Closes: #000

<!--
## API Changes

Uncomment this section if there are any user-facing changes.

Consider whether the change affects users in one of the following ways:

1. Breaks public APIs in some way.
2. Changes the underlying behavior of one of the engine integrations.
3. Should some documentation be updated to reflect this change?

If a public API is changed in a breaking manner, make sure to add the
appropriate label. You can run `./scripts/public-api.sh` locally to see
if there
are any public API changes (and this also runs in our CI).
-->

## Testing

<!--
Please describe how this change was tested. Here are some common
categories for
testing in Vortex:

1. Verifying existing behavior is maintained.
2. Verifying new behavior and functionality works correctly.
3. Serialization compatibility (backwards and forwards) should be
maintained or
   explicitly broken.
-->

---------

Signed-off-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>

Author: connortsui20

benchmarks-website/migrate/src/classifier.rs

@@ -432,17 +432,13 @@ pub enum Skip {
     HistoricalMemory,
 }
 
-/// Engines the v3 emitter produces today. Anything else is historical
-/// and gets bucketed as `Skip::Deprecated`.
-///
-/// ORCHESTRATOR NOTE: confirm against `vortex-bench`'s `Engine` enum
-/// before handing off; edit if the live set differs.
+/// Engines the v3 emitter produces today. Mirrors
+/// `vortex-bench/src/lib.rs::Engine`. Anything else is historical and gets
+/// bucketed as `Skip::Deprecated`.
 const V3_ENGINES: &[&str] = &["datafusion", "duckdb", "vortex", "arrow"];
 
-/// Formats the v3 emitter produces today (`Format::name()` values).
-///
-/// ORCHESTRATOR NOTE: confirm against `vortex-bench/src/lib.rs`
-/// `Format::name()` before handing off.
+/// Formats the v3 emitter produces today (`Format::name()` values from
+/// `vortex-bench/src/lib.rs`).
 const V3_FORMATS: &[&str] = &[
     "vortex-file-compressed",
     "vortex-compact",
@@ -797,6 +793,8 @@ fn split_engine_format(series: &str) -> Option<(String, String)> {
 
 #[cfg(test)]
 mod tests {
+    use anyhow::Context as _;
+
     use super::*;
 
     fn record(name: &str) -> V2Record {
@@ -840,14 +838,16 @@ mod tests {
     }
 
     #[test]
-    fn random_access_bins_dataset_pattern() {
-        let bin = classify(&record("random-access/taxi/take/parquet")).unwrap();
+    fn random_access_bins_dataset_pattern() -> anyhow::Result<()> {
+        let bin = classify(&record("random-access/taxi/take/parquet"))
+            .context("classify returned None for a known-good 4-part random-access name")?;
         assert_eq!(
             bin,
             V3Bin::RandomAccess {
                 dataset: "taxi/take".into(),
                 format: "parquet".into(),
             }
         );
+        Ok(())
     }
 }

benchmarks-website/migrate/src/lib.rs

@@ -13,9 +13,15 @@
 //! The migrator is throwaway: once v3 cuts over, both the binary and
 //! the classifier go away.
 
+/// Routing v2 records into v3 fact tables, ported from v2's `getGroup`.
 pub mod classifier;
+/// V2 commit -> v3 `commits` row upserts.
 pub mod commits;
+/// End-to-end migration of v2 dumps into a v3 DuckDB.
 pub mod migrate;
+/// Streaming readers for the v2 S3 bucket and local dumps.
 pub mod source;
+/// Wire shapes of the v2 benchmark dataset.
 pub mod v2;
+/// Structural diff between a migrated v3 DuckDB and v2's `/api/metadata`.
 pub mod verify;

benchmarks-website/migrate/src/migrate.rs

@@ -869,12 +869,12 @@ mod tests {
 
     use super::*;
 
-    fn open_db_without(table: &str) -> (tempfile::TempDir, Connection) {
-        let dir = tempfile::TempDir::new().unwrap();
+    fn open_db_without(table: &str) -> Result<(tempfile::TempDir, Connection)> {
+        let dir = tempfile::TempDir::new()?;
         let path = dir.path().join("v3.duckdb");
-        let conn = open_target_db(&path).unwrap();
-        conn.execute_batch(&format!("DROP TABLE {table}")).unwrap();
-        (dir, conn)
+        let conn = open_target_db(&path)?;
+        conn.execute_batch(&format!("DROP TABLE {table}"))?;
+        Ok((dir, conn))
     }
 
     fn one_query_row() -> QueryMeasurement {
@@ -898,12 +898,12 @@ mod tests {
     }
 
     #[test]
-    fn flush_all_does_not_overcount_on_failure() {
+    fn flush_all_does_not_overcount_on_failure() -> Result<()> {
         // Drop `compression_times` before flushing so the second
         // flush in `flush_all` fails. The first (queries) succeeded,
         // so its counter must be set; the failed table's counter and
         // every later table's counter must stay at zero.
-        let (_dir, conn) = open_db_without("compression_times");
+        let (_dir, conn) = open_db_without("compression_times")?;
 
         let mut summary = MigrationSummary::default();
         let mut q = QueryAccum::default();
@@ -931,5 +931,6 @@ mod tests {
             summary.compression_size_inserted, 0,
             "later flushes never ran"
         );
+        Ok(())
     }
 }

benchmarks-website/migrate/src/v2.rs

@@ -137,6 +137,7 @@ pub struct V2Commit {
     pub url: Option<String>,
 }
 
+/// Author or committer block on a v2 commit record.
 #[derive(Debug, Clone, Deserialize)]
 pub struct V2Person {
     #[serde(default)]

benchmarks-website/migrate/src/verify.rs

@@ -30,6 +30,8 @@ pub struct VerifyReport {
     pub chart_diffs: Vec<ChartDiff>,
 }
 
+/// One group's chart-count divergence between v2 and v3, captured when the
+/// group is structurally present on both sides but the counts differ.
 #[derive(Debug, Clone)]
 pub struct ChartDiff {
     pub group: String,

benchmarks-website/server/src/api.rs

@@ -39,8 +39,9 @@ pub const DEFAULT_COMMIT_WINDOW: u32 = 100;
 /// Canonical group ordering, ported from the v2 site's hard-coded list at
 /// `origin/ct/vfvb:benchmarks-website/index.html`. Group names not in this
 /// list sort after every listed name in alphabetical order. The order is
-/// significant for the landing page render — the first group is opened by
-/// default and the rest are collapsed.
+/// significant for the landing page render — every group is collapsed by
+/// default, and only the first group's chart payloads are inlined into the
+/// HTML so opening it skips a fetch round-trip.
 pub const GROUP_ORDER: &[&str] = &[
     "Random Access",
     "Compression",
@@ -145,20 +146,13 @@ impl CommitWindow {
     }
 }
 
-/// Query string for `/api/chart/{slug}` and `/chart/{slug}`.
-///
-/// `y` (linear|log) and `mode` (abs|rel) are accepted but ignored by the SQL —
-/// the JSON response is identical regardless. They exist on the API surface so
-/// the client can drive deep links and refetches with a single URL shape; the
-/// rendering hints are applied client-side in `chart-init.js`.
+/// Query string for `/api/chart/{slug}` and `/chart/{slug}`. Only `?n=`
+/// affects the JSON response; per-chart UI state (Y axis, slider) is local
+/// to `chart-init.js` and intentionally not in the URL.
 #[derive(Debug, Default, Deserialize)]
 pub struct ChartQuery {
     /// Commit window: `25`, `50`, `100`, `250`, `all`, etc.
     pub n: Option<String>,
-    /// Y-axis hint (linear|log). Echoed for client-side rendering only.
-    pub y: Option<String>,
-    /// Display mode hint (abs|rel). Echoed for client-side rendering only.
-    pub mode: Option<String>,
 }
 
 impl ChartQuery {
@@ -168,11 +162,14 @@ impl ChartQuery {
     }
 }
 
+/// Body of `GET /api/groups`: every group with its chart links and summary.
 #[derive(Debug, Serialize)]
 pub struct GroupsResponse {
     pub groups: Vec<Group>,
 }
 
+/// One group: a display name, a slug for the group permalink, and the chart
+/// links inside it. Optionally carries a v2-compatible rollup summary.
 #[derive(Debug, Serialize)]
 pub struct Group {
     pub name: String,
@@ -274,12 +271,16 @@ pub struct NamedChartResponse {
     pub chart: ChartResponse,
 }
 
+/// One chart's short label inside a group (e.g. `Q1`) plus the slug that
+/// resolves to its `/api/chart/{slug}` payload.
 #[derive(Debug, Serialize)]
 pub struct ChartLink {
     pub name: String,
     pub slug: String,
 }
 
+/// Body of `GET /api/chart/{slug}`: every commit with data, every series'
+/// values aligned to those commits, and per-series engine/format tags.
 #[derive(Debug, Clone, Serialize)]
 pub struct ChartResponse {
     pub display_name: String,
@@ -317,6 +318,8 @@ pub struct FilterUniverse {
     pub formats: Vec<String>,
 }
 
+/// One row of the `commits[]` array on a [`ChartResponse`]. Carries enough
+/// metadata for the tooltip and the click-to-PR handler in `chart-init.js`.
 #[derive(Debug, Clone, Serialize)]
 pub struct CommitPoint {
     pub sha: String,
@@ -325,6 +328,8 @@ pub struct CommitPoint {
     pub url: String,
 }
 
+/// Body of `GET /health`: liveness probe plus a row-count rollup that's
+/// useful for "did my ingest land?" smoke tests.
 #[derive(Debug, Serialize)]
 pub struct HealthResponse {
     pub status: &'static str,
@@ -334,6 +339,7 @@ pub struct HealthResponse {
     pub row_counts: RowCounts,
 }
 
+/// Per-fact-table row counts surfaced by `/health`.
 #[derive(Debug, Serialize)]
 pub struct RowCounts {
     pub commits: i64,
@@ -1183,16 +1189,6 @@ pub(crate) fn chart_payload(
     }
 }
 
-/// Thin wrapper around [`chart_payload`] kept for callers that prefer the old
-/// name. New code should prefer [`chart_payload`].
-pub(crate) fn collect_chart(
-    conn: &Connection,
-    key: &ChartKey,
-    window: &CommitWindow,
-) -> Result<Option<ChartResponse>> {
-    chart_payload(conn, key, window)
-}
-
 /// Collect every chart inside one group. Returns `None` if the group has no
 /// data at all (callers should render a 404).
 // TODO: this currently re-runs the entire `collect_groups` discovery pass

benchmarks-website/server/src/html.rs

@@ -82,10 +82,10 @@ const VORTEX_BLACK_SVG: &[u8] = include_bytes!("../../public/vortex_black_nobg.s
 const VORTEX_WHITE_SVG: &[u8] = include_bytes!("../../public/vortex_white_nobg.svg");
 const STATIC_ASSET_VERSION: &str = "bench-v3-ui-16";
 
-/// Commits to inline for the open-by-default group. The chart's
-/// initial visible window is ~100 commits; bigger windows just bloat
-/// the cold-page HTML. Users who zoom out trigger a refetch with
-/// `?n=all` via `chart-init.js`.
+/// Commits to inline for the first group's pre-fetched chart payloads.
+/// The chart's initial visible window is ~100 commits; bigger windows
+/// just bloat the cold-page HTML. Users who zoom out trigger a refetch
+/// with `?n=all` via `chart-init.js`.
 const LANDING_INLINE_N: u32 = 100;
 
 /// HTML routes mounted under `/`.
@@ -127,8 +127,7 @@ pub struct UiQuery {
 impl UiQuery {
     /// Resolve the [`CommitWindow`] for HTML routes. Defaults to
     /// [`CommitWindow::All`] so users can pan/zoom all the way back to
-    /// the very first commit on every chart, including the first
-    /// (open-by-default) group on the landing page. Visual downsampling
+    /// the very first commit on every chart. Visual downsampling
     /// happens client-side on the visible commit range only.
     fn fetch_window(&self) -> CommitWindow {
         match self.n.as_deref() {
@@ -208,21 +207,21 @@ async fn landing(State(state): State<AppState>, Query(ui): Query<UiQuery>) -> Re
 
 /// One group's worth of data for the landing page.
 ///
-/// The first group (in canonical order) ships with `charts` populated so
-/// the moment the user expands it the chart hydrates from the inline
-/// JSON without a network round-trip. Every other group ships
-/// with `charts` empty and only their chart-card shells — payloads are
-/// fetched client-side on first `details.toggle` to keep the cold landing
-/// HTML small.
+/// Every disclosure renders closed by default. The first group (in
+/// canonical order) ships with its chart payloads inlined, so the moment
+/// the user expands it the chart hydrates from the inline JSON without a
+/// network round-trip. Every other group ships only its chart-card
+/// shells — payloads are fetched client-side on first `details.toggle`
+/// to keep the cold landing HTML small.
 struct LandingGroup {
     name: String,
     summary: Option<Summary>,
     /// Chart links for every chart in the group. Always present — we need
     /// the slugs server-side so the chart-card shell can carry
     /// `data-chart-slug` for the lazy fetch.
     chart_links: Vec<api::ChartLink>,
-    /// Pre-fetched payloads. Populated only for the open-by-default group.
-    /// `Vec` indices line up with `chart_links`.
+    /// Pre-fetched payloads. Populated only for the first group in
+    /// canonical order. `Vec` indices line up with `chart_links`.
     inlined: Vec<Option<NamedChartResponse>>,
 }
 
@@ -263,7 +262,7 @@ fn collect_landing_groups(conn: &Connection) -> Result<Vec<LandingGroup>> {
             }
             v
         } else {
-            // Closed groups: ship only the shells. The client fetches on
+            // Other groups: ship only the shells. The client fetches on
             // first `details.toggle`.
             (0..group.charts.len()).map(|_| None).collect()
         };
@@ -292,14 +291,14 @@ async fn chart_page(
 
     let window = ui.fetch_window();
     let result = db::run_blocking(&state.db, move |conn| {
-        api::collect_chart(conn, &key, &window)
+        api::chart_payload(conn, &key, &window)
     })
     .await;
     let chart = match result {
         Ok(Some(c)) => c,
         Ok(None) => return error_page(StatusCode::NOT_FOUND, "chart not found").into_response(),
         Err(err) => {
-            tracing::error!(error = ?err, "chart_page: collect_chart failed");
+            tracing::error!(error = ?err, "chart_page: chart_payload failed");
             return error_page(StatusCode::INTERNAL_SERVER_ERROR, "internal error").into_response();
         }
     };

benchmarks-website/server/static/chart-init.js

@@ -1235,17 +1235,17 @@
   }
 
   // -----------------------------------------------------------------------
-  // Lazy fetch on `<details>` toggle for closed-by-default groups.
+  // Lazy fetch on `<details>` toggle. Every group renders closed; this
+  // hydrates the chart cards inside whichever group the user expands.
   // -----------------------------------------------------------------------
   function fetchAndConstruct(card) {
     var canvas = card.querySelector("canvas");
     if (!canvas) return Promise.resolve();
     if (canvas.__bench_chart) return Promise.resolve();
     // `constructChart` reads inline JSON (`<script id="chart-data-N">`)
     // when there's no payload on the canvas yet. The first group's
-    // payloads are inlined server-side regardless of whether the
-    // group is open by default, so try a synchronous construct before
-    // hitting the network.
+    // payloads are inlined server-side, so try a synchronous construct
+    // before hitting the network.
     if (constructChart(card)) {
       bindToolbar(card);
       return Promise.resolve();

benchmarks-website/server/tests/ingest.rs

@@ -283,7 +283,13 @@ async fn read_routes_serve_after_ingest() -> Result<()> {
     assert!(body["display_name"].is_string());
     assert!(body["unit"].is_string());
     assert!(body["commits"].is_array());
-    assert_eq!(body["commits"].as_array().unwrap().len(), 1);
+    assert_eq!(
+        body["commits"]
+            .as_array()
+            .context("commits is array")?
+            .len(),
+        1
+    );
     assert!(body["series"].is_object());
     Ok(())
 }