Repository navigation
feat(explorer): broaden result comparison views and repair compare entry flows - #2922
Conversation
![benchbox-oracle[bot]](https://avatars.githubusercontent.com/u/319264189?s=60&v=4){kind=small}
There was a problem hiding this comment.
oracle-verdict: failure for b2d86564b83dbdc00f39de7e13bb422f44af5f78
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (changes comparison, validation or result logic: benchbox/validation/bundle.py; 264 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
The PR synthesizes publishable timing-derived cost estimates and adds single-query comparison views. The estimates scale with iteration/stream multiplicity, apply a GCP Databricks table to AWS with an unknown pricing date, can silently replace explicit unavailable verdicts, and pass a validation gate that only constrains normalized costs; the new comparison page also keeps an all-iteration median as the primary value when a narrower basis is selected. Fix the estimate construction, provenance retention, validation parity, and basis handling before merge.
Reviewers not available
- sol: absent (error)
- luna: absent (error)
There was a problem hiding this comment.
oracle-verdict: success for dedc6649a4d85ceb8fbba2a05d7c3cfa5fabb97c
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (changes comparison, validation or result logic: benchbox/validation/bundle.py; 279 code lines exceed 150; touches 2 areas).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
All five earlier defects are fixed in head and no new merge-blocking defects were found in the changed files. Cost estimates are now Snowflake-only per-query-median sums with provenance, validation, builder, schema, read-model, and UI estimate handling consistent.
Verified fixed: D1, D2, D3, D4, D5. Resolve their review threads.
Scope: files changed since head b2d86564b83dbdc00f39de7e13bb422f44af5f78 was reviewed.
…try flows Allow up to ten runs in cross-run comparison, build the Compare picker from comparison-eligible runs with a working ranking-scope control and a prominent Compare All Runs action, reorganize the main comparison charts with titled distribution and CDF views plus compact per-query markers, add a shareable single-query comparison page with iteration spread, add a privacy-checked captured plan viewer backed by published sidecars, and label timing-derived Snowflake and Databricks costs as estimates while billed normalized cost stays unavailable without metered usage.
Restore the public site build by teaching the DuckDB snapshot builder the estimated cost status with estimate columns, mirrored in the schema doc and the read model, and render timing-derived estimates in the UI instead of Not recorded. Rebuild the Snowflake estimate from one median runtime per query so iteration and stream multiplicity cannot scale it, refuse Databricks estimates until cloud-matched rates with known provenance exist, keep prior unavailable reasons alongside any estimate, and require concrete billing, region, deployment, and reason exclusion for estimated blocks. Follow the selected measurement basis on the single-query page for the primary value, range, and chart marker, offer Compare All Runs on the benchmark and platform pages for small single-cohort sets, unify the Query compare action with the shared primary style, and render the shared summary table and speedup chart on the Compare page.
Bring the Chromium specs onto the ten-run limit, give Compare All Runs precedence on small cohorts across benchmark, platform, and Find Runs pages with the shared primary style and a ten-run subtitle, and surface estimate method, source, pricing date, and assumptions in the run receipt. Cost estimates remain Snowflake-only; Databricks stays unavailable pending verified pricing.
dedc664 to
fef96d8
Compare
There was a problem hiding this comment.
oracle-verdict: failure for fef96d8f77452e0209cdc2b19aa12a6eb92cd2ad
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (changes comparison, validation or result logic: benchbox/validation/bundle.py; 279 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
Found 3 must-fix issues: validation accepts estimate metadata without estimated status, validation accepts whitespace-only cost strings, and the Snowflake estimate hardcodes its pricing date while using the live rate. Fix these before merge.
Defects without a review thread
- Medium
benchbox/validation/bundle.py:1421: D8: Whitespace-only cost strings pass validation
_validate_required_cost_string accepts whitespace-only strings because it tests only truthiness, while NormalizedCost and the transformer require stripped non-empty strings for cost_estimate_method/source/pricing-date. Whitespace-only estimate provenance therefore passes submission validation but is rejected downstream, weakening the review gate.
Defects outside the diff count like the others: fix them before merge.
Reject cost estimate metadata without estimated status, strip whitespace-only cost strings, and pin the Snowflake estimate to the verified standard/AWS/US rate cell so pricing-table drift or other editions, clouds, or regions refuse to estimate instead of publishing a mismatched rate and date.
There was a problem hiding this comment.
oracle-verdict: success for 311dfac61c6b73764a5c0ea27a7c89d208f4a3a2
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (changes comparison, validation or result logic: benchbox/validation/bundle.py; 302 code lines exceed 150; touches 2 areas).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
The delta adds a fail-closed Snowflake timing-derived 'estimated' cost path with disclosed assumptions, strict estimated-status validation in bundle.py and NormalizedCost, a drift-refusing rate-cell gate, and tests. All three earlier defects are fixed; no new merge-blocking defects were found in the four re-reviewed files.
Verified fixed: D6, D7, D8. Resolve their review threads.
Scope: files changed since head fef96d8f77452e0209cdc2b19aa12a6eb92cd2ad was reviewed.
…indings Move the leaderboard compare picker below the data surface so skeleton geometry and viewport budgets hold, with a compact compare entry point near the filters and a matching skeleton reserve. Keep subset checkboxes enabled next to Compare All Runs with a secondary picks link, name the active ranking scope in guidance and counts, and offer Compare All Runs on Find Runs for single-cohort sets before any pick. Refuse the Snowflake timing-derived estimate when any passing measurement has missing or zero duration instead of publishing a partial sum. Enforce the plan sidecar size cap incrementally while streaming the response body. Align the Chromium specs with ten-run behavior, settled eligibility, and a named chart-count floor.
There was a problem hiding this comment.
oracle-verdict: failure for 8d20181a07da13773f90353617a1e583e60c6fd1
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (changes comparison, validation or result logic: benchbox/validation/bundle.py; 298 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
New estimate/compare/plan-viewer work is close, but three correctness defects block merge: stale plan evidence across result navigation, cost estimates from partial query sets, and assumed deployment values published as facets.
Scope: files changed since head 311dfac61c6b73764a5c0ea27a7c89d208f4a3a2 was reviewed.
…on chart rewrite Cost estimates refuse partial query sets and no longer publish assumed warehouse, cloud, region, or edition values as environment facets. The plan viewer resets when navigating between results. Comparison charts use one distinct marker shape per run within its radius, link query labels to single-query comparison, keep the svg image role for static charts and a group role once links render, and label the latency axis with its log scale.
There was a problem hiding this comment.
oracle-verdict: success for c1a828513de440393b68fb0f0c96568b4079d0cf
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (changes comparison, validation or result logic: benchbox/validation/bundle.py; 361 code lines exceed 150; touches 2 areas).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
All three earlier defects are fixed and no new merge-blocking defects were found in the files changed since the prior review. The estimate path is gated, documented, and refused outside the verified rate cell; environment facets no longer publish assumed values; the plan viewer resets per result.
Verified fixed: D9, D10, D11. Resolve their review threads.
Scope: files changed since head 8d20181a07da13773f90353617a1e583e60c6fd1 was reviewed.
…nd page layout defects Public run IDs did not resolve. The run page shows each run's public ID, the last eight characters of its result ID, but comparison URLs resolved IDs only through the internal short-ID table, so a pasted ID such as /results/compare/query/18?ids=f0835929 never loaded. The resolver now also matches a unique result-ID suffix, and the single-query page leaves its loading state when IDs cannot be resolved. Databricks runs now get a timing-derived list-price estimate with the same method as Snowflake: summed per-query median runtime times the recorded Serverless SQL warehouse size's DBU/hour times the AWS Serverless SQL list rate ($0.70/DBU, identical for every plan tier). Method, source, pricing date and assumptions are published with each estimate. Estimated values now reach the compare page summary, sparkline table and cost scatter instead of a bare "Estimated cost (not metered)". The per-query latency chart sizes its label column to the longest query name and ends long names with an ellipsis instead of clipping their prefix, uses width-aware 1-2-5 log ticks that stay inside the drawing, draws larger run markers, and places runs without a successful measurement in a separate gutter with a legend entry. Hidden <title> copies of visible query labels are removed. Chart runs 7-10 no longer reuse success, danger and warning colours, and every chart uses the same ten-colour palette, so a run keeps its colour across charts. Badges no longer wrap, disabled buttons look disabled, validation badges use sentence case, and override badges show the short rule name with the full text in the tooltip and accessible name. Home, navigation and 404 copy are clearer, benchmark lists sort by display name, the weekly activity grid has equal cells and a legend, and the run page no longer scrolls sideways. The run page drops duplicated tuning and override displays, and the passes page gains a breadcrumb, a non-trivial default basis and a usable phone layout.
…k the single-query page Every place that starts a comparison now uses one CompareCta component with one rule. When two or more runs are selected, "Compare N selected" is the primary action and "Compare All Runs" is offered alongside it while the filtered set holds 2-10 runs. With no selection, "Compare All Runs" is primary for 2-10 runs. Otherwise a visibly disabled button explains the 2-10 limit. Previously a selection on a benchmark page was ignored whenever the filtered set fit, and each page styled the action differently. Leaderboard compare links keep the page's mode and basis. Dashboards are removed. They stored chart links in browser storage only, did not return to the saved chart on compare and run pages, could overwrite newer stored data, and added a save toolbar to every chart. Shareable URLs and benchmark-page chart anchors already cover the need. The single-query page names runs on its axis, uses a log axis when timings span more than 10x and round ticks otherwise, draws each run with its compare-page colour and marker shape, keeps the basis marker hollow so samples stay visible, titles the page with the benchmark and query (for example "TPC-H Q18"), warns when selected runs differ in scale, phase or stream count, lays runs out as rows on narrow screens, shows captured query plans when published, and links to change runs. The compare page transposes the hardware table beyond four runs, gives query-by-run cells readable contrast and significant-digit ratios, uses one shared-query win count in the summary and standings, names no winner when no selected run is rank-safe, and aligns run-card rows and wording.
… per-query chart Speedup vs Baseline and Diverging Bar plotted the same per-query comparison twice, each several thousand pixels tall on large query sets. The compare page now shows one "Per-query comparison vs baseline" figure with a Speedup / Difference (%) switch; speedup is the default and the choice is kept in the URL. The speedup chart sizes its label column to the query names, thins its log-scale ratio ticks by pixel spacing and labels them with significant digits instead of printing many "0.00×" and overlapping powers of two, draws runs without data in a separate gutter, and replaces green/red faster/slower swatches that clashed with run colours with neutral axis direction text. The difference chart uses the same label column, readable 10 px text, run identity labels so repeated platforms stay distinct, and marks changes beyond its scale with an arrow and the true value instead of clipping bars at the edge with unreadable labels.
…creen overflow
Each run now takes its chart colour and marker shape from a slot keyed by
platform, so Snowflake, DuckDB and the other common platforms look the
same on every compare, benchmark and query page instead of changing with
their position in each selection. Repeated platforms in one view get the
next free slot.
Pages no longer scroll sideways on phones or at 200% zoom. Screen-reader
text inside horizontally scrolling tables escaped its scroll container
and widened the page; scroll containers are now positioned, data tables
for screen readers sit inside a hidden wrapper, and compare buttons and
tray actions wrap. Log-scale latency axes in the box plot, CDF and
percentile charts drop ticks that would overlap at the drawn width.
When ranked-only scope leaves the chosen leaderboard cohort empty, an
"Include unranked runs" button switches scope in one click. Compare-page
population notes say "N shared of M queries" when the geomean covers
only shared queries. Cohort mismatch reasons read as one phrase ("runs
differ in benchmark and scale factor"). Scale factors, missing values,
cost labels and signed deltas use the shared formatters on the remaining
pages, and the client-hardware tooltip names remote servers correctly.
The run page keeps its tuning-settings viewer as a compact disclosure for
tuned runs after the duplicated tuning card was removed, and two heavy
rendering tests get explicit timeouts so suite load does not fail them.
There was a problem hiding this comment.
oracle-verdict: failure for 41412876805aaec56e7513f6040ae451e49d56d3
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (changes comparison, validation or result logic: benchbox/validation/bundle.py; 439 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
PR adds timing-derived estimated costs and Q-prefix normalization. It can merge after fixing assumed deployment metadata passed off as observed and Q-stripping collisions that merge distinct query identities.
Defects without a review thread
- High
_project/scripts/explorer_pipeline/transformer.py:1086-1091: D12: Estimated Snowflake cost publishes assumed size/region/cloud as observed deployment
When warehouse size, cloud, or region are unrecorded, the code defaults to X-Small/aws/us-east-2 and stores those guesses in NormalizedCost.pricing_region and deployment. Those columns are later published as factual dimensions, so an unverifiable assumption becomes a filterable result attribute. Only _environment_facets is guarded; deployment is not. - High
_project/scripts/explorer_pipeline/transformer.py:981-986: D13: Estimated Databricks cost publishes assumed cloud/region as observed deployment
When cloud or region are unrecorded, the code defaults to aws/us-east-2 and stores them in NormalizedCost.pricing_region and deployment. The published row therefore presents reference defaults as recorded bundle metadata, making the estimate unverifiable and region/size faceting wrong. - High
benchbox/core/results/query_normalizer.py:17: D14: Leading-Q stripping collapses distinct query identities
Q(?=\d|[a-z]) maps Qtopn to topn and Qqualify_* to qualify_, the same keys as bare topn and qualify_ inputs. Two distinct raw ids normalize identically, so timings, coverage, and expected-answer joins can merge or double-count queries and produce a wrong benchmark result. - Medium
benchbox/validation/bundle.py:268: D15: Coverage normalizer repeats the Q-collapse, corrupting validation verdicts
_normalize_coverage_query_id strips leading Q before a digit or lowercase letter, so the same collision as the core normalizer occurs during coverage accounting. Distinct submitted ids are grouped together, letting missing canonical queries appear covered or covered queries appear missing.
Defects outside the diff count like the others: fix them before merge.
Timing-derived cost estimates no longer publish assumed deployment metadata as if it were recorded. When a bundle does not record its cloud, region or warehouse size, the estimate still prices the reference scenario and lists those assumptions, but the published pricing region is "unknown" and the deployment fields stay empty instead of carrying the reference values. The bundle validator's coverage query-ID normaliser now follows the same case-aware rule as the core normaliser: it strips the uppercase "Q" prefix that results are written with and a "q" before a digit, so named queries such as qualify_row_number keep their names during coverage accounting. It stays self-contained so the slim published validator can import it, and a test keeps it in step with the core function. The per-query difference chart removed the baseline from its legend by comparing a colour slot with a run position, which dropped the wrong run whenever they differed; it now removes the baseline by position, and the legend entry field is named colorSlot so the two cannot be mixed again. The unused timeSeriesColor alias is removed. Comparisons whose runs only have estimated costs now offer the cost chart. The run receipt says "Plans recorded; file not published" when plans were captured without a published sidecar, matching the plan viewer. The heatmap and rank thumbnails are exposed as images, the rank thumbnail's first-place cells use theme text colour for contrast in dark mode, speedup direction labels shorten on narrow charts, page titles and comparison-check cards wrap long names instead of widening phones, and edge ticks on the single-query phone chart stay inside the drawing. The single-query page joins the phone and tablet document overflow audit.
There was a problem hiding this comment.
oracle-verdict: pending for 2a7865d37240c5362767279265cf3084e18e4aee
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (changes comparison, validation or result logic: benchbox/validation/bundle.py; 457 code lines exceed 150; touches 2 areas).
Reviewers not available
- muse: absent (invalid)
- sol: absent (error)
- luna: absent (error)
- sonnet: skipped: same family as the author (claude)
- agy: disabled (CI authentication and quota output are not yet calibrated, and the workflow has no agy job)
There was a problem hiding this comment.
oracle-verdict: success for 2a7865d37240c5362767279265cf3084e18e4aee
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (changes comparison, validation or result logic: benchbox/validation/bundle.py; 457 code lines exceed 150; touches 2 areas).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
All four earlier defects are fixed: estimated Snowflake/Databricks costs no longer publish assumed size/cloud/region as observed deployment and refuse unverified cells with disclosed assumptions; query-identity normalization is scoped to preserve QC1/QPR1 and lowercase-q names with coverage matching core. No new merge-blocking correctness, validation, or cost-publication defects found in the changed backend files examined.
Verified fixed: D12, D13, D14, D15. Resolve their review threads.
There was a problem hiding this comment.
oracle-verdict: failure for 66d8144735489f04228a04b24a7b1bca44491a73
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (changes comparison, validation or result logic: benchbox/validation/bundle.py; 457 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
Estimated-cost and query-id-repair paths publish explorer numbers that contradict the submission validation contract or cannot be verified against bundle bytes. Fix the unknown-region estimates and the silent id rewrite before merge.
Defects without a review thread
- High
_project/scripts/explorer_pipeline/transformer.py:1076: D16: Snowflake estimate publishes unknown pricing_region
Snowflake timing-derived estimate sets pricing_region to _canonical_cost_region(observed_region) or 'unknown'. The common case with no recorded region therefore publishes cost_status='estimated' with pricing_region='unknown', while its own assumptions claim 'AWS us-east-2 is used as the reference region'. benchbox/validation/bundle.py estimated validation requires a concrete pricing_region and rejects 'unknown'/'not_applicable', so the explorer-published estimate can never satisfy the submission gate and its regional basis is unverifiable. Refuse to estimate when region is unrecorded or publish the concrete reference region used for the rate. - High
_project/scripts/explorer_pipeline/transformer.py:971: D17: Databricks estimate publishes unknown pricing_region
Databricks serverless estimate uses the same pricing_region or 'unknown' fallback. A bundle without a recorded region still yields cost_status='estimated' with pricing_region='unknown' plus an assumption that us-east-2 was used as reference. This contradicts the concrete-pricing_region requirement for estimated normalized cost in benchbox/validation/bundle.py and leaves the published DBU estimate regionally unverifiable. Refuse to estimate or publish the concrete reference region. - Medium
_project/scripts/explorer_pipeline/transformer.py:291: D18: Silent read_primitives query-id repair breaks verifiability
repair_damaged_query_ids rewrites stored 'ualify' ids to 'qualify_' before BundleDocument parsing, but result_id_from_bundle hashes the original file bytes. The explorer therefore publishes repaired query_ids under an id derived from damaged bytes, with no repair provenance recorded. A reader comparing bundle bytes to explorer rows cannot verify the mapping, and coverage/validation on raw bytes still sees the damaged ids. Record the repair or reject damaged bundles instead of silent mutation.
Defects outside the diff count like the others: fix them before merge.
…ry-ID repairs Timing-derived cost estimates now publish the concrete region whose rate was applied as pricing_region, as the submission validator requires for estimated costs. When the bundle does not record a region, that is the reference region named in the estimate's assumptions; the deployment cloud, region and warehouse-size fields still carry only values the bundle recorded. A test runs each kind of estimate through the bundle validator so the explorer cannot publish an estimate the submission gate would reject. Read Primitives bundles published before the query-ID normaliser fix store six qualify_* queries without their leading "q". The explorer still maps them to their catalog names so old and new runs join on the same query, but each affected run now records the mapping in a query_id_repairs column and shows it on the run receipt, so published rows can be traced back to the unchanged bundle bytes.
There was a problem hiding this comment.
oracle-verdict: pending for 290dc13fb7fd416c8ff3efe45a7f4146d485c8b3
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (changes comparison, validation or result logic: benchbox/validation/bundle.py; 489 code lines exceed 150; touches 2 areas).
Reviewers not available
- muse: absent (incomplete)
- sol: absent (error)
- luna: absent (error)
- sonnet: skipped: same family as the author (claude)
- agy: disabled (CI authentication and quota output are not yet calibrated, and the workflow has no agy job)
There was a problem hiding this comment.
oracle-verdict: success for 290dc13fb7fd416c8ff3efe45a7f4146d485c8b3
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (changes comparison, validation or result logic: benchbox/validation/bundle.py; 489 code lines exceed 150; touches 2 areas).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
Prior cost-region and silent-repair defects are fixed with strict guards and disclosed assumptions; spot-checks of cost, normalizer, validation, and explorer display paths found no new correctness, validation-gate, or security blockers.
Verified fixed: D16, D17, D18. Resolve their review threads.
This branch was successfully deployed
1 participant
Summary
Broadens the Results Explorer comparison views, fixes the compare entry flows, and makes charts, tables, labels and numbers read the same on every page.
Comparing runs
CompareCtacontrol appears everywhere a comparison can start: benchmark, platform, Find runs, the leaderboard picker and the tray.modeandbasis.f0835929) now resolve in comparison URLs. A missing ID is reported immediately instead of holding the page on "Loading". Cold or lagging snapshot reads are still retried.Compare page
Single-query comparison (
/results/compare/query/:queryId)Costs
pricing_regionnames the concrete region whose rate was applied, as the submission validator requires. The deployment cloud, region and warehouse-size fields carry only values the bundle recorded. A test runs each kind of estimate through the bundle validator.Consistency across pages
Query IDs
normalize_query_idstripped a leading lowercase "q" before a letter, so the Read Primitivesqualify_*queries were published asualify_*. It now strips only the uppercase "Q" prefix thatformat_query_idwrites, and a "q" before a digit, so stored IDs still round-trip.ualify_*IDs back to their catalog names without editing bundles. Each affected run records the mapping in aquery_id_repairscolumn, and the run receipt shows it, so published rows can be traced back to the unchanged bundle bytes.format_query_idwrites"Q" + id, soQtopnis the stored form oftopn. No benchmark catalog has an ID starting with "Q" followed by a lowercase letter, so this cannot merge two distinct queries.Removed: Dashboards
Verification
npm run typecheckandnpm run typecheck:e2epass./results/compare?sf=10(TPC-Havoc and Ranking Scope)/results/tpchavoc/?sf=10against/results/compare?ids=da24dd5f,e68c9f9c,0f3fae9f/results/compare?sf=10&benchmark=tpch%2Ctpcds&mode=times/results/tpch/?sf=10with runs selected/results/r/tpch-snowflake-sf10.0-20261003-f0835929/results/compare/query/18?ids=f0835929,5f5ccaf2,d3495b28Notes for reviewers
.plans.jsonsidecar yet, so the plan viewer shows its unavailable state in production until plans are published.