[claude] benchmarks v3 cleanup

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(())
 }