From 0dbdcd5f41ed724e4459c7441110cc6c97c2bab3 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 26 May 2026 14:50:31 -0400 Subject: [PATCH] Simplify dynamic clap configuration in benchmark_runner cli Collapse the per-suite help-visibility machinery into a single command builder. The `SelectorSuiteOptions` / `SelectorOptionVisibility` / `SelectorCommand` types and the `build_cli_for_args_with_options` -> `build_cli_with_help_options` -> `build_cli_with_selector_options` chain existed only to narrow `info --help` / `query --help` to the selected suite's options. That narrowing is redundant with the richer curated `info ` view rendered manually in output.rs, and the narrowing behavior was untested. Now `build_cli` registers the union of all suites' options once under a single `Suite Options` heading. Suite-defined options are still registered dynamically with clap, and multi-character short aliases (e.g. `-sf`) are still normalized to their long form before parsing. The only behavior change: `info --help` / `query --help` list the union of suite options rather than just the selected suite's. The curated `info ` output is unchanged. Net: removes 3 types and several builder/helper functions (~180 fewer lines in cli.rs). Binary builds, clippy is clean, and all benchmark_runner tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- benchmarks/src/benchmark_runner/cli.rs | 279 +++++-------------------- benchmarks/src/benchmark_runner/mod.rs | 9 +- 2 files changed, 56 insertions(+), 232 deletions(-) diff --git a/benchmarks/src/benchmark_runner/cli.rs b/benchmarks/src/benchmark_runner/cli.rs index 49b1680dbfe42..ae5ca5bf9b197 100644 --- a/benchmarks/src/benchmark_runner/cli.rs +++ b/benchmarks/src/benchmark_runner/cli.rs @@ -27,7 +27,7 @@ use crate::benchmark_runner::native::NativeBenchmarks; use crate::benchmark_runner::output::write_help_section; use crate::benchmark_runner::style::{HELP_STYLES, header}; -use crate::benchmark_runner::suite::{SuiteConfig, SuiteOption, SuiteRegistry}; +use crate::benchmark_runner::suite::{SuiteOption, SuiteRegistry}; use crate::util::CommonOpt; use clap::{Arg, ArgAction, ArgMatches, Args, Command}; use datafusion_common::{Result, exec_datafusion_err}; @@ -75,26 +75,6 @@ pub enum RunnerCommand { Query(InspectArgs), } -/// Suite-option visibility used while constructing selector subcommands. -#[derive(Clone, Copy)] -enum SelectorSuiteOptions<'a> { - /// Include every discovered suite option. - All, - /// Include only options from the selected suite. - Suite(&'a SuiteConfig), - /// Do not include suite options. - None, -} - -/// Selector subcommands that accept a SQL suite name. -#[derive(Clone, Copy)] -enum SelectorCommand { - /// The `info` subcommand. - Info, - /// The `query` subcommand. - Query, -} - /// Common selector arguments for SQL inspection and execution commands. #[derive(Debug, Clone)] pub struct InspectArgs { @@ -106,34 +86,6 @@ pub struct InspectArgs { pub suite_options: BTreeMap, } -/// Per-subcommand suite-option visibility used for help rendering. -#[derive(Clone, Copy)] -struct SelectorOptionVisibility<'a> { - info: SelectorSuiteOptions<'a>, - query: SelectorSuiteOptions<'a>, -} - -/// Constructs visibility rules for normal and help-specific command trees. -impl<'a> SelectorOptionVisibility<'a> { - /// Shows all suite options for each selector command. - fn all() -> Self { - Self { - info: SelectorSuiteOptions::All, - query: SelectorSuiteOptions::All, - } - } - - /// Shows a selected option set for one selector command's help output. - fn for_help(command: SelectorCommand, options: SelectorSuiteOptions<'a>) -> Self { - let mut visibility = Self::all(); - match command { - SelectorCommand::Info => visibility.info = options, - SelectorCommand::Query => visibility.query = options, - }; - visibility - } -} - /// Precomputed suite option metadata used while parsing dynamic CLI options. pub(crate) struct SuiteCliOptions { /// All suite option long names. @@ -181,70 +133,24 @@ impl SuiteCliOptions { } } -/// Builds the full command tree with all discovered suite options available. +/// Builds the full clap command tree. +/// +/// Every suite's options are registered on the `info` and `query` selector +/// subcommands so any of them can be parsed regardless of the selected suite. +/// `--help` for those subcommands therefore lists the union of all suite +/// options; the curated per-suite view lives in the `info ` output. pub(crate) fn build_cli(registry: &SuiteRegistry, native: &NativeBenchmarks) -> Command { - build_cli_with_selector_options(registry, native, SelectorOptionVisibility::all()) -} - -/// Builds a command tree using precomputed suite option metadata. -pub(crate) fn build_cli_for_args_with_options( - registry: &SuiteRegistry, - suite_cli_options: &SuiteCliOptions, - native: &NativeBenchmarks, - args: &[OsString], -) -> Command { - match selector_help_request(suite_cli_options, args) { - Some((command, Some(selector))) => { - let options = registry - .get(&selector) - .map(SelectorSuiteOptions::Suite) - .unwrap_or(SelectorSuiteOptions::None); - - build_cli_with_help_options(registry, native, command, options) - } - Some((command, None)) => build_cli_with_help_options( - registry, - native, - command, - SelectorSuiteOptions::None, - ), - None => build_cli(registry, native), - } -} - -/// Builds a help-oriented command tree for one selector subcommand. -fn build_cli_with_help_options( - registry: &SuiteRegistry, - native: &NativeBenchmarks, - command: SelectorCommand, - options: SelectorSuiteOptions<'_>, -) -> Command { - build_cli_with_selector_options( - registry, - native, - SelectorOptionVisibility::for_help(command, options), - ) -} - -/// Builds the clap command tree with configurable suite-option visibility per selector subcommand. -fn build_cli_with_selector_options( - registry: &SuiteRegistry, - native: &NativeBenchmarks, - visibility: SelectorOptionVisibility<'_>, -) -> Command { - let info = add_configured_suite_options( + let info = add_suite_options( base_selector_command("info", "Show benchmark metadata") .override_usage( "benchmark_runner info [QUERY_ID]\n benchmark_runner info ", ), registry, - visibility.info, ); let info = add_native_info_subcommands(info, native); - let query = add_configured_suite_options( + let query = add_suite_options( base_selector_command("query", "Show parsed benchmark SQL"), registry, - visibility.query, ); Command::new("benchmark_runner") @@ -331,14 +237,8 @@ fn run_usage() -> &'static str { } /// Formats the SQL benchmark `run` help sections reused by SQL `info`. -pub(crate) fn format_sql_run_help_sections_styled( - registry: &SuiteRegistry, -) -> Result { - let run = add_configured_suite_options( - base_run_command(), - registry, - SelectorSuiteOptions::None, - ); +pub(crate) fn format_sql_run_help_sections_styled() -> Result { + let run = base_run_command(); let mut output = String::new(); writeln!(output, "{}", header(format!("Usage: {}", run_usage())))?; @@ -406,33 +306,21 @@ fn base_selector_command(name: &'static str, about: &'static str) -> Command { ) } -/// Adds each distinct suite-defined option to a command. +/// Adds each distinct suite-defined option to a command under the shared +/// `Suite Options` heading. fn add_suite_options(mut command: Command, registry: &SuiteRegistry) -> Command { let mut seen = BTreeSet::new(); for suite in registry.suites() { for option in &suite.options { if seen.insert(option.name.clone()) { - command = add_suite_option(command, option, SUITE_OPTIONS_HEADING, false); + command = add_suite_option(command, option); } } } command } -/// Applies the requested suite-option visibility mode to a selector command. -fn add_configured_suite_options( - command: Command, - registry: &SuiteRegistry, - suite_options: SelectorSuiteOptions<'_>, -) -> Command { - match suite_options { - SelectorSuiteOptions::All => add_suite_options(command, registry), - SelectorSuiteOptions::Suite(suite) => add_suite_options_for(command, suite), - SelectorSuiteOptions::None => command, - } -} - /// Adds each visible native benchmark as an info-only selector subcommand. fn add_native_info_subcommands( mut command: Command, @@ -445,73 +333,37 @@ fn add_native_info_subcommands( command } -/// Adds only one suite's options under a suite-specific help heading. -fn add_suite_options_for(mut command: Command, suite: &SuiteConfig) -> Command { - let heading = suite_options_heading(&suite.name); - - for option in &suite.options { - command = add_suite_option(command, option, heading.clone(), true); - } - - command -} - -/// Registers one suite-defined option with clap, using direct short aliases -/// when clap supports them and documenting multi-character aliases otherwise. -fn add_suite_option( - command: Command, - option: &SuiteOption, - heading: impl Into, - register_short_alias: bool, -) -> Command { - let name = option.name.clone(); - let (short, help) = match &option.short { - Some(short) if register_short_alias && short.chars().count() == 1 => { - (short.chars().next(), option.help.clone()) - } - Some(short) => (None, format!("{} Alias: -{short}.", option.help)), - None => (None, option.help.clone()), +/// Registers one suite-defined option with clap as a long `--name` flag. +/// +/// Short aliases are documented in help text rather than registered directly: +/// clap supports only single-character shorts, and every alias (including +/// multi-character ones such as `-sf`) is rewritten to its long form before +/// parsing by [`normalize_suite_option_aliases_with_options`]. +fn add_suite_option(command: Command, option: &SuiteOption) -> Command { + let help = match &option.short { + Some(short) => format!("{} Alias: -{short}.", option.help), + None => option.help.clone(), }; - let mut arg = Arg::new(name.clone()) - .long(name) - .value_name("VALUE") - .help(help) - .help_heading(heading); - if let Some(short) = short { - arg = arg.short(short); - } - - command.arg(arg) -} -/// Formats the suite-specific heading used for selected suite options. -fn suite_options_heading(suite_name: &str) -> String { - format!("{SUITE_OPTIONS_HEADING} ({suite_name})") -} - -/// Detects whether the provided arguments are asking for help on a selector -/// subcommand and, when possible, extracts the suite name preceding `--help`. -fn selector_help_request( - suite_cli_options: &SuiteCliOptions, - args: &[OsString], -) -> Option<(SelectorCommand, Option)> { - if !args.iter().any(|arg| is_help_arg(arg)) { - return None; - } - - selector_request(suite_cli_options, args, true) + command.arg( + Arg::new(option.name.clone()) + .long(option.name.clone()) + .value_name("VALUE") + .help(help) + .help_heading(SUITE_OPTIONS_HEADING), + ) } -/// Scans raw CLI arguments to find a selector before clap parses them. +/// Scans raw CLI arguments to find the suite selector for `info`/`query`. +/// +/// Returns the suite name that follows the selector subcommand, if one appears +/// before any option flags. Used to pick the suite whose short aliases should be +/// rewritten to long form before clap parses the arguments. fn selector_request( suite_cli_options: &SuiteCliOptions, args: &[OsString], - stop_at_help: bool, -) -> Option<(SelectorCommand, Option)> { - let (command_position, command) = - args.iter().enumerate().find_map(|(position, arg)| { - selector_command(arg).map(|command| (position, command)) - })?; +) -> Option { + let command_position = args.iter().position(|arg| is_selector_command(arg))?; let mut skip_next = false; for arg in &args[command_position + 1..] { @@ -520,11 +372,7 @@ fn selector_request( continue; } - if stop_at_help && is_help_arg(arg) { - return Some((command, None)); - } - - if is_value_taking_selector_option(suite_cli_options, command, arg) { + if is_value_taking_selector_option(suite_cli_options, arg) { skip_next = true; continue; } @@ -533,31 +381,21 @@ fn selector_request( continue; } - return Some((command, Some(arg.to_string_lossy().into_owned()))); + return Some(arg.to_string_lossy().into_owned()); } - Some((command, None)) + None } -/// Converts a raw argument into a selector command name when it matches. -fn selector_command(arg: &OsStr) -> Option { - match arg.to_str()? { - "info" => Some(SelectorCommand::Info), - "query" => Some(SelectorCommand::Query), - _ => None, - } -} - -/// Returns whether a raw argument requests clap help. -fn is_help_arg(arg: &OsStr) -> bool { - arg == "--help" || arg == "-h" +/// Returns whether a raw argument is a SQL selector subcommand (`info`/`query`). +fn is_selector_command(arg: &OsStr) -> bool { + matches!(arg.to_str(), Some("info" | "query")) } -/// Returns whether an argument is an option whose next token should be skipped -/// while looking for a suite selector before `--help`. +/// Returns whether an argument is an option whose next token is its value and +/// should therefore be skipped while looking for a suite selector. fn is_value_taking_selector_option( suite_cli_options: &SuiteCliOptions, - _command: SelectorCommand, arg: &OsStr, ) -> bool { let arg_text = arg.to_string_lossy(); @@ -582,8 +420,7 @@ where .into_iter() .map(|arg| arg.as_ref().to_os_string()) .collect::>(); - let aliases = selector_request(suite_cli_options, &args, false) - .and_then(|(_, selector)| selector) + let aliases = selector_request(suite_cli_options, &args) .and_then(|selector| suite_cli_options.aliases_by_suite.get(&selector)); args.into_iter() @@ -721,17 +558,6 @@ mod tests { .collect() } - fn build_cli_for_args(registry: &SuiteRegistry, args: I) -> Command - where - I: IntoIterator, - T: AsRef, - { - let suite_cli_options = SuiteCliOptions::new(registry); - let args = os_args(args); - let native = native_registry(registry); - build_cli_for_args_with_options(registry, &suite_cli_options, &native, &args) - } - fn command_from_matches( registry: &SuiteRegistry, matches: &ArgMatches, @@ -839,11 +665,10 @@ help = "Selects the suite option." #[test] fn help_mentions_query_id() { - let help = - build_cli_for_args(®istry(), ["benchmark_runner", "query", "--help"]) - .try_get_matches_from(["benchmark_runner", "query", "--help"]) - .unwrap_err() - .to_string(); + let help = build_cli_with_native(®istry()) + .try_get_matches_from(["benchmark_runner", "query", "--help"]) + .unwrap_err() + .to_string(); assert!(help.contains("SUITE")); assert!(help.contains("SQL benchmark suite name")); @@ -868,7 +693,7 @@ help = "Selects the suite option." #[test] fn sql_run_help_sections_use_run_command_metadata() { - let output = format_sql_run_help_sections_styled(®istry()).unwrap(); + let output = format_sql_run_help_sections_styled().unwrap(); assert!(output.contains("Usage:")); assert!(output.contains("SQL Benchmark Target:")); diff --git a/benchmarks/src/benchmark_runner/mod.rs b/benchmarks/src/benchmark_runner/mod.rs index 5bd85e3dc2af8..0557d9af1335b 100644 --- a/benchmarks/src/benchmark_runner/mod.rs +++ b/benchmarks/src/benchmark_runner/mod.rs @@ -63,7 +63,7 @@ mod style; mod suite; use crate::benchmark_runner::cli::{ - InspectArgs, RunnerCommand, SuiteCliOptions, build_cli_for_args_with_options, + InspectArgs, RunnerCommand, SuiteCliOptions, build_cli, command_from_matches_with_options, format_sql_run_help_sections_styled, normalize_suite_option_aliases_with_options, }; @@ -101,8 +101,7 @@ where let args = normalize_suite_option_aliases_with_options(&suite_cli_options, args); let root_help_requested = is_root_help_request(&args); - let mut cli = - build_cli_for_args_with_options(®istry, &suite_cli_options, &native, &args); + let mut cli = build_cli(®istry, &native); let matches = match cli.try_get_matches_from_mut(args) { Ok(matches) => matches, Err(e) if e.kind() == ErrorKind::DisplayHelp => { @@ -228,12 +227,12 @@ async fn load_info_output( )) })?; suite.resolve_option_values(&args.suite_options)?; - let run_help_sections = format_sql_run_help_sections_styled(registry)?; + let run_help_sections = format_sql_run_help_sections_styled()?; return write_suite_info_styled(&ResolvedSuite::from(suite), &run_help_sections); } let (target, sql) = load_benchmark(registry, benchmark_dir, args).await?; - let run_help_sections = format_sql_run_help_sections_styled(registry)?; + let run_help_sections = format_sql_run_help_sections_styled()?; write_info_styled(target.suite.as_ref(), &sql, &run_help_sections) }