Add dfbench statistics command - #23975
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23975 +/- ##
==========================================
+ Coverage 81.04% 81.22% +0.18%
==========================================
Files 1105 1111 +6
Lines 380163 385878 +5715
Branches 380163 385878 +5715
==========================================
+ Hits 308111 313444 +5333
- Misses 53834 53916 +82
- Partials 18218 18518 +300 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6f2a30b to
8b7b5ac
Compare
There was a problem hiding this comment.
@gabotechs
Thanks for adding this new statistics command.
I found a few issues that could make benchmark reports incomplete, stale, or nondeterministic.
I also left a testing suggestion to help protect the command's repeatability guarantees.
| .map_err(|error| DataFusionError::External(Box::new(error))) | ||
| } | ||
|
|
||
| fn query_files(path: &Path, query: Option<&str>) -> Result<Vec<PathBuf>> { |
There was a problem hiding this comment.
It would be helpful to add focused unit tests for SQL-aware multi-statement parsing, partial-run persistence and exit status, and duplicate table-name rejection. These cases define the repeatability contract for the new standalone CLI and would help prevent regressions.
There was a problem hiding this comment.
Added some tests here 6366a30.
I want to be careful in not introducing too many tests, as those also require maintenance, and this is just a benchmarking tool that is not in a production path.
Let me know if you think we should be covering some more.
|
I think @Omega359 is in the process of porting dfbench code -- maybe we should use the new format |
8b7b5ac to
13ccf04
Compare
|
@alamb I imagine you are referring to #23772. Unless I misinterpreted the intentions there, I see that's focusing on performance benchmarks, while this PR focuses on planning-time statistics vs execution-time metrics statistical divergence. I've tried several things already for shipping a tool that can qualify improvements to DataFusion stats system:
As there's really not any precedence about what I'm trying to do here, and it does not quite fit neither in the existing benchmark infrastructure or the current integration tests, I preferred to ship something as isolated as possible, so that:
Also @Omega359, if you have ideas about how to integrate this with your work, they are more than welcome, but my impression is that integrating this with the existing or future (#23772) is going to require some shoehorning that is likely to get in the way of other efforts rather than being helpful. |
Fair enough. Another thing we could do potentially is to add some sort of mode to the benchmark runner ("stats verification mode" perhaps?) that runs the query and then verifies that the actual metrics match the statistics 🤔 |
Most of the benchmarks in dfbench will be redirected to the SQL benchmark suite, and their corresponding Rust-based implementations will be removed in the somewhat near future. However, dfbench itself will remain because a few benchmarks are not well suited for SQL benchmarking. From a quick look at the code for this PR this looks like it'll be one of those. I don't think it's a concern at this point. |
What I found challenging is how to integrate this with existing benchmark runner without it getting in the way of the classical "performance benchmarks" path. For example, typical performance benchmarks have the option to specify the number of iterations, which does not make much sense with planing stats vs execution metrics benchmarks, and planing stats vs execution metrics benchmarks need to render the output in a very certain way for them to be useful (plans displayed with q-error per operator), which does not make sense for classical benchmarks. What I found is that integrating it in existing benchmarks would require some "if normal_path; do this; else if stats_vs_metrics_path; do this other different thing" conditional logic that could end up getting in the way of other people contributing to the classical benchmarks. Most of the code added in this PR is kind of very specific to this planning stats vs execution metrics benchmarks, but there are some common bits that could potentially be reused with other types of benchmarks: fn query_files(), fn register_parquet_files() and fn collect_parquet_files(), making a total of 80 LOC that could potentially be reused from the ~650 LOC these new benchmarks have. Even if 80 LOC out of 650 is something, it's still not a lot to be reused with existing benchmarks, the other 570 LOC are very specific to this new type of benchmark. |
6366a30 to
e2f79d3
Compare
|
Thanks @gabotechs for this PR, the q-error report is a useful addition that will help improving our supports for statistics! One issue I hit while trying it out: for plans containing a fetch, the reported q-error is not reproducible between runs, which affects the Running the same query (
An earlier set of four runs on the same machine produced -50.0% and -166.7%, so the tool reports both improvements and regressions where nothing changed (it failed AA testing, basically). In my example query, the estimate column is stable at Some options to address the problem:
Claude helped creating the attached repro script, qerr_repro.sh, which runs from the root of a datafusion checkout, builds dfbench itself, and generates its own dataset. It covers just the query shared above, as that's a minimal reproducer for the only problem that could be detected using the existing benchmark. Note the spread depends on core count and load, so your numbers will differ from mine; the claim is that the runtime column varies, not the specific values. I suggest to use |
Is that too bad though? it's not like we are enforcing some specific benchmark numbers in the CI, users of the Do you foresee other places where reporting might be non-deterministic? If It was just me using the tool, I think I could live with it being non-deterministic for certain operators, but in case other people don't, something that comes to mind is to add a new cli argument for skipping certain operators from the comparison. Something like: |
|
Also, @alamb, @kosiew and @Omega359, just to be upfront with the intentions of this cli. The idea would be to follow up immediately by opening a bunch of easily reproducible Github issues with improvement opportunities in the stats propagation logic and link them to #8227. I'm expecting to create a bunch of issues that follow the same template:
|
To me it's still valuable as-is, as at the moment we don't have any easy way to assess improvements over statistics, as we have only a handful of CBO rules ( Re. #23975 (comment), during my investigation for the review, AI stumbled upon several long hanging fruits, and I could genuinely verify some after a cursory look, so I agree landing this benchmark would already have a tangible beneficial effect. |
|
I think it is worth moving forward with this while working out how dfbench should work with A few reasons I think this is valuable:
Longer term, I think the ideal direction is to share suite discovery, query selection, data/configuration handling, etc. with #23772 where practical, while allowing statistics analysis to retain its own execution and reporting model. |
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. The SQL-aware statement parsing, failed-report persistence, duplicate table-name handling, and focused tests address the main points from my previous review.
I found one remaining issue with SQL dialect handling. The new pre-parsing step always uses the generic dialect, while the session can be configured with a different dialect through datafusion.sql_parser.dialect. This means SQL that the configured session would otherwise accept can now fail before planning or potentially be parsed differently.
I think this should be fixed before merging. Once the parser uses the session's configured dialect and there is a regression test covering a non-generic dialect, the concerns from my previous review should be addressed.
There was a problem hiding this comment.
Thanks for the follow-up changes. The earlier issues around SQL-aware parsing, configured dialect handling, failed report persistence, and duplicate table names look addressed.
I found one remaining issue with multi-statement suites that contain session-changing statements. The current execution path plans and collects each statement directly from SessionState, so statements such as DDL do not get applied to the SessionContext before subsequent statements run.
I reproduced this with CREATE TABLE x AS VALUES (1); SELECT * FROM x;. The CREATE TABLE statement fails with Unsupported logical plan: CreateMemoryTable, and the following query then cannot find x.
I think this should be fixed before merging. One option is to route session-changing statements through the context execution path while keeping the existing metric-collection path for query plans. Alternatively, if dfbench statistics is only intended to support query statements, it would be better to reject non-query statements explicitly and document that restriction. A regression test covering a session-changing statement followed by a query would also be useful.
| statement: Statement, | ||
| ) -> Result<Vec<OperatorReport>> { | ||
| let state = ctx.state(); | ||
| let logical_plan = state.statement_to_plan(statement).await?; |
There was a problem hiding this comment.
I think this introduces a regression for multi-statement suites that contain session-changing statements. report_statement calls SessionState::statement_to_plan, optimizes the result, and then directly creates and collects the physical plan. That bypasses the SessionContext execution path that applies session-affecting logical plans such as DDL.
For example, CREATE TABLE x AS VALUES (1); SELECT * FROM x; currently reports the first statement as Unsupported logical plan: CreateMemoryTable, and the second statement then cannot find x. The earlier ctx.sql path applied the session change before continuing.
Could we route session-changing statements through the context execution path while preserving the metric-collection path for query plans? Alternatively, if this command is intended to support query statements only, we should reject non-query statements explicitly and document that restriction. It would also be good to add a regression test with a session-changing statement followed by a query.
There was a problem hiding this comment.
If possible, I'd try to avoid introducing more complexity for the sake of getting a perfect solution, specially given that the usefulness of this command is not yet proven.
As this is just a development tool, if people find the need of extending it for supporting more esoteric SQL setups, they can just contribute a patch in future PRs as needed (there's a high chance these additions are never even needed).
There was a problem hiding this comment.
@gabotechs, thanks for working through the earlier feedback. The SQL-aware statement handling, parser configuration, duplicate table-name checks, failure persistence, and context execution for session-changing statements all look good now.
I have two small follow-up suggestions below. Neither is a merge blocker given that this is intentionally scoped as an experimental local development tool.
|
|
||
| impl RunOpt { | ||
| pub async fn run(self) -> Result<()> { | ||
| let mut config = SessionConfig::from_env()?.with_collect_statistics(true); |
There was a problem hiding this comment.
One small limitation to keep in mind: sql_parser_options is cloned once before the suite statements run. That means something like SET datafusion.sql_parser.dialect = 'MySQL' in one file will not affect how a later file is pre-parsed. The same applies to parser recursion-limit changes.
Supporting this would require more incremental parsing and state handling, so I think deferring it is reasonable for the current scope. It would be good to document the limitation for now. If support is added later, we could fetch the parser options from ctx.state() for each file and add a test where a SET ... dialect statement is followed by dialect-specific SQL.
| success: true, | ||
| error: None, | ||
| }, | ||
| Err(error) => QueryReport { |
There was a problem hiding this comment.
Another small edge case: because the report is replaced after every statement, an interrupted run or a failure while reading a later query file can leave the successful prefix behind as statistics.json. A later --compare would then treat that partial report as a complete baseline.
Publishing the report only after the full run succeeds, or recording a completion status and rejecting incomplete reports as comparison baselines, would avoid that. This does add some persistence machinery, so I think it is reasonable to defer unless incomplete comparisons become a practical problem.
6 participants
Which issue does this PR close?
Rationale for this change
Provide a repeatable way to measure how closely planning cardinality statistics match runtime output for benchmark query suites.
The goal is not to have an automated benchmark suite, and not something to enforce in the CI. Instead, this is meant to be a development tool for developers to locally iterate over stats estimation improvements.
What changes are included in this PR?
dfbench statisticsaccepts any Parquet data directory and SQL file or directory, so it can be used with TPC-DS, TPC-H, or another compatible query suite. It:--compare.Example: verify a TPC-DS Q21 improvement
Compare against a baseline from
mainTPC-H works with its partitioned data layout as well:
Are these changes tested?
No unit tests were added: this is a reporting CLI over planner statistics and runtime metrics, exercised by the benchmark-suite smoke runs above.
If reviewers think this type of code needs to be covered by unit tests let me know.
Are there any user-facing changes?
No, this is just for extending the
dfbenchcommand with a newdfbench statisticssubcommand