Skip to content

runner: resolve benchmarks at runtime, drop the allowlist - #15

Merged
adriangb merged 1 commit into
mainfrom
unify-datafusion-runner
Jun 9, 2026
Merged

runner: resolve benchmarks at runtime, drop the allowlist#15
adriangb merged 1 commit into
mainfrom
unify-datafusion-runner

Conversation

@adriangb

@adriangb adriangb commented Jun 9, 2026

Copy link
Copy Markdown
Owner

Motivation

Follow-up to #13's review: the bench_standard runner hardcoded an
is_criterion_harness name match to decide how each benchmark was compared,
and the controller maintained a per-repo allowlist (standard / criterion
name lists). Both drift as upstream evolves — e.g. run_tpch is itself
Criterion-harness-based upstream now, and the SQL-harness suite set keeps
growing.

This implements the two asks from that thread:

  1. "first try criterion and then fall back to bench.sh so we don't have to match like this"
  2. "remove the benchmark allowlist and always try to resolve any benchmarks the user requests"

What changed

Runtime resolution instead of name matching. For each requested
benchmark the runner asks cargo metadata whether the name is a real
Criterion [[bench]] target:

  • Yes → cargo bench --bench <name> -- --save-baseline <side>.
  • No → bench.sh run <name> (TPC-H keeps its direct dfbench shortcut).

SQL_CARGO_COMMAND is now set unconditionally on the bench.sh path.
bench.sh only reads it inside its SQL-harness run functions, so it's a no-op
for the dfbench suites and gives the harness suites (wide_schema, …) a named
baseline for critcmp — no name list required.

Comparison driven by artifacts, not names: results/*.json
bench.sh compare_detail; target/criterion baselines → critcmp. A run
mixing both families emits both sections. is_criterion_harness is gone.

Allowlist removed. Any requested name is scheduled and resolved on the
runner (a name that matches neither a Criterion target nor a bench.sh suite
fails there). RepoEntry now carries only kind ("datafusion" | "arrow")
and default_standard. JobType/BenchType collapse Standard+Criterion
into a single Datafusion variant (legacy "standard"/"criterion" strings
still map to it for in-flight DB rows). bench_standard.rs + bench_criterion.rs
merge into bench_datafusion.rs; bench_arrow.rs is unchanged (separate repo).

Deploy note

The BENCHMARK_CONFIG schema changed (services/controller.ts), so this
needs a Pulumi apply on deploy.

Testing

cargo fmt --check, cargo clippy --all-targets -- -D warnings, and
cargo test (127 tests) all pass.

🤖 Generated with Claude Code

Previously the controller maintained a per-repo allowlist and a hardcoded
`is_criterion_harness` match to decide how each benchmark ran and was
compared. Both drift as upstream adds suites (e.g. `run_tpch` is now
Criterion-harness-based upstream).

Resolve everything at runtime instead:

- For each requested benchmark, ask `cargo metadata` whether the name is a
  real Criterion `[[bench]]` target. If so, run `cargo bench --bench <name>
  --save-baseline <side>`; otherwise run it through `bench.sh` (TPC-H keeps
  its direct `dfbench` shortcut).
- Always set `SQL_CARGO_COMMAND` on the bench.sh path — `bench.sh` only reads
  it inside its SQL-harness run functions, so it's a no-op for the dfbench
  suites and gives the harness suites a named baseline for `critcmp`.
- Drive comparison off the artifacts produced, not the name: `results/*.json`
  -> `bench.sh compare_detail`; `target/criterion` baselines -> `critcmp`. A
  mixed run emits both.

Drop the allowlist: any requested name is scheduled and resolved on the
runner (an unresolvable name fails there). `RepoEntry` now carries only
`kind` ("datafusion" | "arrow") and `default_standard`. `JobType`/`BenchType`
collapse Standard+Criterion into a single Datafusion variant (legacy
"standard"/"criterion" strings still map to it for in-flight rows); arrow is
unchanged. `bench_standard.rs` + `bench_criterion.rs` merge into
`bench_datafusion.rs`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@adriangb

adriangb commented Jun 9, 2026

Copy link
Copy Markdown
Owner Author

Keeping up the whitelists has been a PITA. Let's try this out.

@adriangb
adriangb merged commit 8cba2cd into main Jun 9, 2026
3 checks passed
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