adriangb opened a new pull request, #25441: URL: https://github.com/apache/datafusion/pull/25441
## Which issue does this PR close? - N/A. This is a benchmark harness fix found while measuring https://github.com/apache/datafusion/pull/25339. ## Rationale for this change A benchmark file can assert a string in the physical plan with the `expect_plan` directive. That check ran on every iteration, inside the measured region, and it rendered the plan with `{:#?}`. A derived `Debug` for an in-memory source prints every `RecordBatch` the source holds. Thus, for a benchmark whose tables come from `CREATE TABLE ... AS SELECT`, the check builds a multi-megabyte string per iteration and then searches it. For the `null_aware_join` suite, whose tables are built from `range()`, that string is 1.85 MB and the check is most of the measured time: the suite reports 17.6 ms for Q01, while `datafusion-cli` runs the same query in 4 ms. This inflates absolute numbers and, more importantly, dilutes the A/B difference the benchmark exists to show. ## What changes are included in this PR? The check now renders the plan as `EXPLAIN` displays it, through `DisplayableExecutionPlan`. It also runs on the first iteration only, because a plan does not change between iterations. `null_aware_join`, release build, both sides at the same commit (`c4f5a9e0f2`), median of 30 iterations: | Query | before | after | |---|---|---| | Q01 | 17.6 ms | 4.6 ms | | Q02 | 15.3 ms | 2.6 ms | | Q03 | 14.7 ms | 2.1 ms | | Q04 | 0.9 ms | 0.4 ms | | Q05 | 0.9 ms | 0.4 ms | | Q06 | 0.8 ms | 0.3 ms | | Q07 | 0.9 ms | 0.3 ms | | Q08 | 1.2 ms | 0.5 ms | A failed check now also prints an 8-line plan instead of a 1.8 MB dump. There are 96 `expect_plan` strings in the suites. 89 are operator names, which both forms print. The 6 `null_aware_join` strings that read `null_aware: true` become `null_aware`, which is how `HashJoinExec` displays the flag; this PR changes those 6 lines. The one remaining string, h2o's `output_ordering=[pk@0 ASC NULLS LAST, ob@1 DESC]`, holds in the display form as well. ## What is the testing strategy for this PR? - New unit test `run_checks_expect_plan_once_per_benchmark`: the first run checks the plan, and a later run does not repeat the check. The existing tests still cover the accept and reject paths of a first run. - I ran the suites that need no downloaded data, and all their `expect_plan` strings still hold: `null_aware_join` (8 queries), `smj` (26 queries, which include the 3 `LeftMark` checks), `nlj` (4 queries) and `array_agg_distinct`. - For the h2o `output_ordering` string, which needs data I do not have, I built an equivalent case: a Parquet table with `WITH ORDER`, a window query over it, and that `expect_plan` line. It passes. - Counterfactual check: with a string that is not in the plan, the first iteration still fails, and the message names the string. - `cargo test -p datafusion-benchmarks --lib` passes (143 tests). `cargo fmt` and clippy are clean. ## Are there any user-facing changes? No. This changes the benchmark harness and its documentation only. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
