Prepare SQL runner for native benchmark migration - #24806
Open
Omega359 wants to merge 1 commit into
Open
Conversation
- 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 Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
Omega359
marked this pull request as ready for review
August 30, 2026 22:09
Contributor
Author
|
fyi @alamb |
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.
Which issue does this PR close?
bench.shto use the newbenchmark_runner#24311 and Remove olddfbenchrunner once benchmarks are migrated to the new SQL benchmark framework #24312.Rationale for this change
The remaining migration from native Rust benchmarks to SQL benchmarks will make
benchmark_runnerthe primary path used bybench.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:
bench.shruns 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
SELECTandWITHstatements. 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_NAMESPACEThis lets future
bench.shCriterion runs assign distinct groups such astpch__parquet_sf1andtpch__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 sqlharness now obtains itsBENCH_*configuration fromsql_benchmark_runner.Keeping this parsing with the shared runner ensures that both Criterion entry points support the same namespace behaviour:
benchmark_runner --criterioncargo bench --bench sqlIt also makes the environment-based harness configuration testable without putting configuration logic in the thin Criterion entry point.
Add regression coverage
The tests cover:
SELECTfollowed by cleanup DDL.BENCH_NAMESPACEhandling in both entry points.--criterion-namespaceoutside Criterion mode.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: