Skip to content

runner: compare Criterion SQL-harness benchmarks with critcmp - #13

Merged
adriangb merged 1 commit into
mainfrom
sql-bench-criterion-compare
Jun 9, 2026
Merged

runner: compare Criterion SQL-harness benchmarks with critcmp#13
adriangb merged 1 commit into
mainfrom
sql-bench-criterion-compare

Conversation

@adriangb

@adriangb adriangb commented Jun 9, 2026

Copy link
Copy Markdown
Owner

Problem

bench.sh has two benchmark families, and the standard runner only knew how to compare one:

Family Output Compared by
dfbench-based (clickbench, imdb, h2o, tpcds, sort_*) results/<name>/*.json bench.sh compare_detail
Criterion SQL harness (wide_schema, predicate_eval, run via cargo bench --bench sql) target/criterion/** nothing

The standard runner always called compare_detail, which reads results/*.json. The SQL-harness suites never write those, so the comparison was empty — the result comment showed:

Comparing HEAD and <branch>
Note: Skipping .../results/HEAD/*.json as .../results/<branch>/*.json does not exist

i.e. only resource usage was posted, no per-query timings — confirmed on a real wide_schema run.

Fix

Route the SQL-harness suites through the same mechanism bench_criterion.rs already uses for native Criterion benches:

  1. Run each side with SQL_CARGO_COMMAND=cargo bench --bench sql -- --save-baseline <RESULTS_NAME> so Criterion saves a named baseline per side (HEAD for base, the branch name for the PR). The SQL harness's arg parser uses ignore_errors, so --save-baseline passes through to Criterion.
  2. After both sides, gather the baselines into one target/criterion tree (reusing bench_criterion::copy_criterion_baselines) and diff them with critcmp HEAD <branch>.

dfbench suites are unchanged (still compare_detail). A run mixing both families emits both sections. is_criterion_harness() enumerates the SQL-harness suites and generalizes to future ones.

No datafusion-side changes and no new dependencies — critcmp is already in the runner image and used by the criterion path.

Testing

cargo fmt --check, cargo clippy --all-targets -- -D warnings, and cargo test (135 tests, incl. a new criterion_harness_classification) all pass. End-to-end verification will come from a wide_schema run once deployed (the suite already runs to completion; this is purely about producing the comparison output).

🤖 Generated with Claude Code

bench.sh has two benchmark families: dfbench-based suites that write
results/*.json (compared via `bench.sh compare_detail`), and Criterion
SQL-harness suites (`cargo bench --bench sql`: wide_schema, predicate_eval)
that write only target/criterion. The standard runner always used
compare_detail, so the SQL-harness suites produced an empty comparison —
the result comment showed "Skipping results/HEAD/*.json as
results/<branch>/*.json does not exist" and no timings.

Route the SQL-harness suites through the same machinery the criterion
runner already uses:
- run each side with `SQL_CARGO_COMMAND=cargo bench --bench sql --
  --save-baseline <RESULTS_NAME>` so Criterion saves a named baseline
  per side (HEAD / <branch>);
- after both sides, copy the baselines into one target/criterion tree
  (reusing bench_criterion::copy_criterion_baselines) and diff them with
  `critcmp HEAD <branch>`.

dfbench suites are unchanged (still compare_detail). A run that mixes
both families gets both sections. `is_criterion_harness` lists the
SQL-harness suites and generalizes to future ones.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@adriangb
adriangb merged commit c1e1bd9 into main Jun 9, 2026
3 checks passed
Comment on lines +21 to +23
fn is_criterion_harness(bench: &str) -> bool {
matches!(bench, "wide_schema" | "predicate_eval")
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice if these weren't hardcoded...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant