You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Omega359 (via GitHub)" <gi...@apache.org> on 2026/10/11 15:00:30 UTC
[PR] fix: enable Criterion automatically when a benchmark namespace is set [datafusion]
Omega359 opened a new pull request, #26210:
URL: https://github.com/apache/datafusion/pull/26210
## Is your feature request related to a problem or challenge?
Running a SQL benchmark with `--criterion-namespace` or `BENCH_NAMESPACE` currently requires a separate `--criterion` flag. Since namespaces apply to Criterion benchmark groups, selecting a namespace should also select Criterion mode.
## Describe the solution you'd like
Enable Criterion when either namespace option is set. Apply the existing mode checks after inferring Criterion mode, so incompatible `--output` and `--iterations` options still fail and `--save-baseline` works. Print the inferred-mode notice to stderr to preserve stdout output.
## Describe alternatives you've considered
Keeping the explicit `--criterion` requirement would preserve the current behavior but require callers to specify both options.
## Additional context
This follows the SQL benchmark migration preparation in #24806.
## Willingness to contribute
Implementation included in this draft PR.
## Which issue does this PR close?
No linked issue.
## Rationale for this change
Allow benchmark callers to select namespaced Criterion runs with a single option or environment variable.
## What changes are included in this PR?
- Infer Criterion mode from `--criterion-namespace` and `BENCH_NAMESPACE` and update the CLI help.
- Share namespace validation between the CLI and the Criterion runner.
- Extend CLI tests for inferred mode, environment defaults, CLI overrides, list and dry-run behavior, conflicting options, saved baselines, and stderr notices.
## What is the testing strategy for this PR?
The benchmark-runner tests exercise argument parsing and environment handling, including a subprocess test for `BENCH_NAMESPACE`. Existing tests cover invalid namespaces and distinct Criterion artifacts.
Local validation:
- `cargo test -p datafusion-benchmarks --bin benchmark_runner`: 62 passed.
- `cargo fmt --all` and diff whitespace checks passed.
- `cargo clippy --all-targets --all-features -- -D warnings`: failed with three `mutable_key_type` errors in unchanged `datafusion/optimizer/src/push_down_filter.rs` (lines 1138, 1142, and 1606).
- The standard lint suite and extended workspace tests are still in progress; their results are pending.
This changes CLI mode selection, not query execution performance; no performance improvement is claimed.
## Are there any user-facing changes?
Yes. `--criterion-namespace` and `BENCH_NAMESPACE` now enable Criterion without an explicit `--criterion` flag. The runner reports inferred mode on stderr.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(benchmarks): enable Criterion automatically when a benchmark namespace is set [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #26210:
URL: https://github.com/apache/datafusion/pull/26210#issuecomment-6129973106
I am excited -- I want to get https://github.com/apache/datafusion/issues/21706 over the line
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat(benchmarks): enable Criterion automatically when a benchmark namespace is set [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #26210:
URL: https://github.com/apache/datafusion/pull/26210
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: enable Criterion automatically when a benchmark namespace is set [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26210:
URL: https://github.com/apache/datafusion/pull/26210#issuecomment-6110513880
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26210?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `96.34146%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.80%. Comparing base ([`454ca08`](https://app.codecov.io/gh/apache/datafusion/commit/454ca08b43fd8bb83362f575750850d5745ddee3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c89713c`](https://app.codecov.io/gh/apache/datafusion/commit/c89713c1a88b01e97f0696b7fba0ba5686453c1d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/26210?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [benchmarks/src/bin/benchmark\_runner.rs](https://app.codecov.io/gh/apache/datafusion/pull/26210?src=pr&el=tree&filepath=benchmarks%2Fsrc%2Fbin%2Fbenchmark_runner.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YmVuY2htYXJrcy9zcmMvYmluL2JlbmNobWFya19ydW5uZXIucnM=) | 96.29% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26210?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #26210 +/- ##
==========================================
- Coverage 82.80% 82.80% -0.01%
==========================================
Files 1149 1149
Lines 452213 452272 +59
Branches 452213 452272 +59
==========================================
+ Hits 374473 374520 +47
- Misses 54945 54953 +8
- Partials 22795 22799 +4
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26210?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org