fix(web): rank summaries over whole groups and give every group one - #21
Open
joseph-isaacs wants to merge 1 commit into
Open
fix(web): rank summaries over whole groups and give every group one#21joseph-isaacs wants to merge 1 commit into
joseph-isaacs wants to merge 1 commit into
Conversation
The random-access card ranked a single chart. `collectRandomAccessSummary`
walked the group's chart links and returned the first that had rows -- in
practice always the alphabetically first `dataset/pattern` -- then published
its raw times under the group-wide title "Random Access Performance". The
producer emits `dataset` as `{dataset}/{pattern}` plus a legacy bare `taxi`,
so that was one of ~nine charts speaking for all nine: `lance` at 352us on
`feature-vectors/correlated` while it runs over 1ms on the rest.
It also pinned every format to one global latest commit, so an
intermittently benchmarked format (`lance`) vanished from the card on any
commit it skipped -- the exact staleness the compression summaries already
handle with a per-format snapshot policy.
Rank random access across every chart in the group instead, reading each
format at its own newest run per chart. Query, random-access, and
vector-search summaries now share one `rankSeries` model: geomean of the
damped time ratio to the fastest series per bucket, with v2's missing-series
penalty where a series skipped a bucket, plus `measured`/`total` on each row
so a partially covered series is legible in the card's hover text rather
than silently penalty-inflated.
Summaries are now default-on. `queryGroupHasV2Summary` gated query cards
behind a five-dataset v2 allowlist, leaving `spatialbench`, `fineweb`,
`gharchive`, `appian`, `public-bi`, and `clickbench-sorted` with no card;
vector-search groups returned `null` outright. Both gates are gone, and
`collectGroupSummary`'s switch is exhaustive over `GroupKey`, so a sixth
fact table is a compile error rather than a silently blank card.
Checks: `pnpm test` (352 passed, 53 skipped -- the testcontainers Postgres
suites, including the new random-access and vector-search cases, need a
Docker daemon that this environment does not have), `tsc --noEmit`,
`eslint .`, `next build`.
Signed-off-by: "Joe Isaacs" <joe.isaacs@live.co.uk>
|
Vercel preview for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
The random-access card ranked a single chart, not the group.
collectRandomAccessSummarywalked the group's chart links, returned the first that had rows, and published that chart's raw times under the group-wide title "Random Access Performance".The producer decides how many charts that is:
benchmarks/random-access-benchemitsdatasetas{dataset}/{pattern}plus a legacy baretaxi, so the group holds ~nine charts. The chart list isORDER BY dataset, so the card was alwaysfeature-vectors/correlated— one chart of nine speaking for all nine. That is howlancewas reported at 352us leading random access while it runs over 1ms on most of the other charts.A second bug rode along: the SQL pinned every format to one global
MAX(timestamp).lanceruns less often than Vortex, so on any commit it skipped it dropped off the card entirely — the exact staleness the compression summaries already solve with their per-format snapshot policy.Summaries were also gated off by default
Two hardcoded gates left benchmarks with no card at all:
queryGroupHasV2Summaryallowlisted five v2-era datasets (clickbench,statpopgen,polarsignals,tpch,tpcds). Everything the emitter has added since —spatialbench,fineweb,gharchive,appian,public-bi,clickbench-sorted,vortex— fell through tonull.case 'VectorSearchGroup': return Promise.resolve(null)— no vector-search group ever had one.Neither gate had a reason beyond "v2 didn't do it", and a newly added suite silently got nothing.
What changed
rankSeries: geomean of the damped(10 + value) / (10 + best)ratio per bucket, with v2's missing-series penalty where a series skipped a bucket. Query scores are unchanged — same code path, same 300us floor, and the pinned fixture scores (1.10554 / 1.60356) still hold.measured/total, surfaced in the card's hover text, so a series measured in 4 of 9 charts says so rather than presenting a penalty-inflated score as like-for-like.collectGroupSummary's switch is exhaustive overGroupKey, so a sixth fact table becomes a compile error rather than a silently blank card. Recorded as a load-bearing rule inAGENTS.md.Wire-shape change
The
randomAccesssummary variant's rows change from{ name, time, ratio }to the shared{ name, score, totalRuntime, measured, total }, and a newvectorSearchvariant is added. This is the read API (GET /api/groups,GET /api/group/{slug}), not the producer→ingest contract, soSCHEMA_VERSIONis untouched.The card's value column now shows a geomean ratio and a total runtime instead of an absolute time, because no single absolute time is true of a nine-chart group. Worth a look during review — if absolute times are wanted, the honest alternative is a per-chart card, not a group one.
Checks
pnpm test— 352 passed, 53 skippedtsc --noEmit,eslint .,next build— cleanThe 53 skipped are the testcontainers Postgres suites, which include the four new random-access and vector-search cases added here; no Docker daemon was available in the authoring environment, so those ran only as mocked-pool equivalents in
lib/summary.test.ts. CI should be checked for the real ones.To cover that gap the new code path was additionally run end-to-end against a local Postgres 16 with the production schema (
migrations/applied), seeded with the nine-chart shape the producer actually emits —lancefast onfeature-vectors/correlated, slower elsewhere, absent from two charts:Same 352us
lancewin on the first chart that produced the old card; it just no longer decides the ranking. Seeded data, so illustrative of the mechanism rather than a prediction of production values.Generated by Claude Code