Omega359 opened a new pull request, #24806:
URL: https://github.com/apache/datafusion/pull/24806
## Which issue does this PR close?
- Part of #21706.
- Prepares for #24311 and #24312.
- This PR does not close these issues.
## 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:
```shell
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
