Skip to content

Prepare SQL runner for native benchmark migration - #24806

Open
Omega359 wants to merge 1 commit into
apache:mainfrom
Omega359:sql_benchmark_prepare_for_migration
Open

Prepare SQL runner for native benchmark migration#24806
Omega359 wants to merge 1 commit into
apache:mainfrom
Omega359:sql_benchmark_prepare_for_migration

Conversation

@Omega359

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

The remaining migration from native Rust benchmarks to SQL benchmarks will make benchmark_runner the primary path used by bench.sh. Before making that switch, the SQL runner needs to preserve the reporting behaviour of the native runner and keep Criterion results from different benchmark configurations separate.

Two issues currently prevent that:

  1. In fixed-iteration mode, a SQL benchmark with cleanup DDL after its result-producing query can report the DDL row count instead of the query row count. This would produce incorrect row counts in the JSON output used by the benchmark comparison tools.
  2. Criterion derives its artifact paths from benchmark group and function names. bench.sh runs some suites under several configurations, such as TPC-H with different formats and scale factors or H2O with different formats and sizes. These runs can share the same SQL group and query names, causing their Criterion artifacts to collide.

Fixing these issues allows the final migration to retain fixed-iteration reporting while offering Criterion as an optional mode for every migrated SQL benchmark.

What changes are included in this PR?

Preserve the result-producing query row count

The streaming SQL runner now updates the reported row count only for SELECT and WITH statements. It still executes subsequent DDL, but that DDL no longer replaces the row count from the last result-producing query.

The buffered and streaming paths now use the same result-statement classification.

This matches the behaviour needed by the native-to-SQL migration: fixed-iteration benchmark output continues to report the number of rows produced by the workload rather than zero from a trailing cleanup statement.

Add Criterion result namespaces

The SQL Criterion runner accepts an optional namespace and appends it to the Criterion group name. The standalone runner exposes this through:

  • --criterion-namespace <NAMESPACE>
  • BENCH_NAMESPACE

This lets future bench.sh Criterion runs assign distinct groups such as tpch__parquet_sf1 and tpch__memory_sf1, preventing one configuration from overwriting another configuration's measurements.

Namespaces are restricted to lowercase ASCII letters, digits, _, and -. Lowercase-only names avoid collisions on case-insensitive filesystems, while the character restrictions prevent a namespace from introducing extra path components.

The combined Criterion group and namespace must not exceed 64 bytes. Criterion truncates longer directory components, so accepting longer names could make otherwise distinct namespaces resolve to the same artifact directory.

The runner validates all selected group names before starting any benchmarks. An invalid namespace therefore fails the invocation before Criterion writes partial results.

Share direct Criterion harness configuration

The direct cargo bench --bench sql harness now obtains its BENCH_* configuration from sql_benchmark_runner.

Keeping this parsing with the shared runner ensures that both Criterion entry points support the same namespace behaviour:

  • benchmark_runner --criterion
  • cargo bench --bench sql

It also makes the environment-based harness configuration testable without putting configuration logic in the thin Criterion entry point.

Add regression coverage

The tests cover:

  • Preserving the row count from a SELECT followed by cleanup DDL.
  • Safe and unsafe Criterion namespace values.
  • Case-insensitive filesystem collision prevention.
  • Criterion's 64-byte directory-name limit.
  • BENCH_NAMESPACE handling in both entry points.
  • Rejection of --criterion-namespace outside Criterion mode.
  • Separate Criterion artifact directories for different namespaces.

The environment-variable tests use subprocesses where needed so process-wide environment changes cannot leak into concurrently running tests.

What is the testing strategy for this PR?

The following checks were run:

cargo test -p datafusion-benchmarks --lib --bins
cargo check -p datafusion-benchmarks --bench sql
./dev/rust_lint.sh
git diff --check

The tests include an artifact-level check that runs the same SQL benchmark under two namespaces and verifies that Criterion writes separate  estimates.json files.

## Are there any user-facing changes?

Yes, for benchmark users.

Criterion-mode SQL benchmarks gain the optional --criterion-namespace argument and corresponding BENCH_NAMESPACE environment variable. Fixed-
iteration SQL benchmark output now reports the last result-producing query's row count when later DDL statements execute.

There are no changes to DataFusion's public library APIs.

- Preserve the last result-producing query's row count when trailing DDL is executed, keeping fixed-iteration output compatible with native benchmarks.
- Add collision-safe Criterion namespaces to separate results for different formats, scale factors, and configurations. Share environment parsing between the direct Criterion harness and benchmark runner so both entry points support the same behaviour.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.69965% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.53%. Comparing base (e4cf35c) to head (2288cfc).

Files with missing lines Patch % Lines
benchmarks/src/bin/benchmark_runner.rs 92.40% 12 Missing ⚠️
benchmarks/src/sql_benchmark_runner.rs 97.16% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #24806    +/-   ##
========================================
  Coverage   81.52%   81.53%            
========================================
  Files        1123     1123            
  Lines      406148   406423   +275     
  Branches   406148   406423   +275     
========================================
+ Hits       331124   331378   +254     
- Misses      55659    55677    +18     
- Partials    19365    19368     +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Omega359
Omega359 marked this pull request as ready for review August 30, 2026 22:09
@Omega359

Copy link
Copy Markdown
Contributor Author

fyi @alamb

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.

2 participants