From 767a404491397e097749e2c3961c99cbb304ee88 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Mon, 7 Sep 2026 09:16:54 -0500 Subject: [PATCH 1/2] bench: discover ClickBench query files in sql_planner instead of hardcoding ranges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sql_planner` built its ClickBench planning set from hardcoded ranges: `(0..=42)` for `queries/` and `(0..=7)` for `extended/`. The extended directory now holds q0..q13, so extended q8..q13 were never planned. Replace both ranges with `read_numbered_queries`, which reads `q{N}.sql` starting at `q0.sql` and stops at the first missing file — the same discovery scheme `dfbench clickbench` uses (`get_query_sql` / `get_query_path` in `benchmarks/src/clickbench.rs`). Newly added query files are now picked up without a code change. A missing file previously panicked via `read_to_string().unwrap()`; the helper treats `NotFound` as the end of the sequence, still panics on any other IO error, and asserts the directory was not empty so a wrong `benchmarks_path` fails loudly rather than silently registering nothing. `cargo bench -p datafusion --bench sql_planner -- --list` now enumerates 57 `physical_plan_clickbench_q*` benchmarks (43 standard + 14 extended), up from 51, and the six newly covered queries plan successfully. Co-Authored-By: Claude Opus 5 --- datafusion/core/benches/sql_planner.rs | 44 ++++++++++++++++++-------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/datafusion/core/benches/sql_planner.rs b/datafusion/core/benches/sql_planner.rs index de81ed021b427..dc1868764f8cb 100644 --- a/datafusion/core/benches/sql_planner.rs +++ b/datafusion/core/benches/sql_planner.rs @@ -85,6 +85,29 @@ fn physical_plan(ctx: &SessionContext, rt: &Runtime, sql: &str) { })); } +/// Read the `q{N}.sql` query files from `dir`, starting at `q0.sql` and +/// stopping at the first missing file. +/// +/// This mirrors the discovery scheme used by the `dfbench clickbench` runner +/// (see `get_query_sql` / `get_query_path` in `benchmarks/src/clickbench.rs`) +/// so that newly added query files are picked up without changing this code. +fn read_numbered_queries(dir: &str) -> Vec { + let mut queries = Vec::new(); + for q in 0.. { + let path = format!("{dir}q{q}.sql"); + match std::fs::read_to_string(&path) { + Ok(sql) => queries.push(sql), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => break, + Err(e) => panic!("Failed to read query file '{path}': {e}"), + } + } + assert!( + !queries.is_empty(), + "No `q*.sql` query files found in '{dir}'" + ); + queries +} + /// Create schema with the specified number of columns fn create_schema(column_prefix: &str, num_columns: usize) -> Schema { let fields: Fields = (0..num_columns) @@ -636,20 +659,13 @@ fn criterion_benchmark(c: &mut Criterion) { // }); // -- clickbench -- - let clickbench_queries = (0..=42) - .map(|q| { - std::fs::read_to_string(format!( - "{benchmarks_path}queries/clickbench/queries/q{q}.sql" - )) - .unwrap() - }) - .chain((0..=7).map(|q| { - std::fs::read_to_string(format!( - "{benchmarks_path}queries/clickbench/extended/q{q}.sql" - )) - .unwrap() - })) - .collect::>(); + let clickbench_queries = + read_numbered_queries(&format!("{benchmarks_path}queries/clickbench/queries/")) + .into_iter() + .chain(read_numbered_queries(&format!( + "{benchmarks_path}queries/clickbench/extended/" + ))) + .collect::>(); let clickbench_ctx = register_clickbench_hits_table(&rt); From d559b1b413e4be9f56e54860b99270363b373044 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Mon, 7 Sep 2026 10:11:28 -0500 Subject: [PATCH 2/2] bench: fail loudly on a gap in the ClickBench query numbering Review feedback: stopping at the first missing file means a gap silently truncates the set. If `q8.sql` disappeared while q9..q13 remained, the benchmark would quietly plan only q0..q7 and the non-empty assertion would still pass. Discover the queries by listing the directory instead, keeping only entries matching `q{N}.sql` (which also skips the `sorted_data` directory alongside the standard ClickBench queries), sort them numerically, and assert the numbering is contiguous from `q0.sql`. Verified by temporarily removing `extended/q8.sql`: the bench now panics with `left: [0..7, 9..13]` / `right: [0..12]`, naming the directory and making the gap obvious, rather than silently dropping six queries. Co-Authored-By: Claude Opus 5 --- datafusion/core/benches/sql_planner.rs | 60 ++++++++++++++++++-------- 1 file changed, 43 insertions(+), 17 deletions(-) diff --git a/datafusion/core/benches/sql_planner.rs b/datafusion/core/benches/sql_planner.rs index dc1868764f8cb..48770541ca9b6 100644 --- a/datafusion/core/benches/sql_planner.rs +++ b/datafusion/core/benches/sql_planner.rs @@ -85,27 +85,53 @@ fn physical_plan(ctx: &SessionContext, rt: &Runtime, sql: &str) { })); } -/// Read the `q{N}.sql` query files from `dir`, starting at `q0.sql` and -/// stopping at the first missing file. +/// Read the `q{N}.sql` query files from `dir`, in ascending numeric order. /// -/// This mirrors the discovery scheme used by the `dfbench clickbench` runner -/// (see `get_query_sql` / `get_query_path` in `benchmarks/src/clickbench.rs`) -/// so that newly added query files are picked up without changing this code. +/// The files are discovered by listing the directory rather than by walking a +/// hardcoded range, so newly added queries are picked up without changing this +/// code. Entries that do not match `q{N}.sql` (such as the `sorted_data` +/// directory alongside the ClickBench queries) are ignored. +/// +/// Panics if `dir` contains no queries, or if the numbering is not contiguous +/// starting at `q0.sql`. A gap almost certainly means a query file was lost, and +/// silently benchmarking a subset would hide that. fn read_numbered_queries(dir: &str) -> Vec { - let mut queries = Vec::new(); - for q in 0.. { - let path = format!("{dir}q{q}.sql"); - match std::fs::read_to_string(&path) { - Ok(sql) => queries.push(sql), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => break, - Err(e) => panic!("Failed to read query file '{path}': {e}"), - } - } + let entries = std::fs::read_dir(dir) + .unwrap_or_else(|e| panic!("Failed to read query directory '{dir}': {e}")); + + let mut numbers = entries + .map(|entry| { + entry.unwrap_or_else(|e| panic!("Failed to read entry in '{dir}': {e}")) + }) + .filter_map(|entry| { + let file_name = entry.file_name(); + let stem = file_name + .to_str()? + .strip_prefix('q')? + .strip_suffix(".sql")?; + stem.parse::().ok() + }) + .collect::>(); + numbers.sort_unstable(); + assert!( - !queries.is_empty(), - "No `q*.sql` query files found in '{dir}'" + !numbers.is_empty(), + "No `q{{N}}.sql` query files found in '{dir}'" + ); + assert_eq!( + numbers, + (0..numbers.len()).collect::>(), + "Query files in '{dir}' are not numbered contiguously from q0.sql" ); - queries + + numbers + .into_iter() + .map(|q| { + let path = format!("{dir}q{q}.sql"); + std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("Failed to read query file '{path}': {e}")) + }) + .collect() } /// Create schema with the specified number of columns