From 8235dbb0493560ddfed03d6a1f46ebeca9fbfaa2 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Mon, 20 Nov 2023 13:55:17 -0500 Subject: [PATCH 1/3] Use clap for sqllogicttest parsing --- datafusion/sqllogictest/Cargo.toml | 1 + datafusion/sqllogictest/bin/sqllogictests.rs | 82 +++++++++++--------- 2 files changed, 45 insertions(+), 38 deletions(-) diff --git a/datafusion/sqllogictest/Cargo.toml b/datafusion/sqllogictest/Cargo.toml index 436c6159e7a3..684cc3470ef3 100644 --- a/datafusion/sqllogictest/Cargo.toml +++ b/datafusion/sqllogictest/Cargo.toml @@ -52,6 +52,7 @@ tempfile = { workspace = true } thiserror = { workspace = true } tokio = { version = "1.0" } tokio-postgres = { version = "0.7.7", optional = true } +clap = { version = "4.4.8", features = ["derive", "env"] } [features] avro = ["datafusion/avro"] diff --git a/datafusion/sqllogictest/bin/sqllogictests.rs b/datafusion/sqllogictest/bin/sqllogictests.rs index 618e3106c629..ea1fedf02765 100644 --- a/datafusion/sqllogictest/bin/sqllogictests.rs +++ b/datafusion/sqllogictest/bin/sqllogictests.rs @@ -21,6 +21,7 @@ use std::path::{Path, PathBuf}; #[cfg(target_family = "windows")] use std::thread; +use clap::Parser; use datafusion_sqllogictest::{DataFusion, TestContext}; use futures::stream::StreamExt; use log::info; @@ -77,7 +78,7 @@ async fn run_tests() -> Result<()> { // Enable logging (e.g. set RUST_LOG=debug to see debug logs) env_logger::init(); - let options = Options::new(); + let options: Options = clap::Parser::parse(); // Run all tests in parallel, reporting failures at the end // @@ -88,7 +89,7 @@ async fn run_tests() -> Result<()> { .map(|test_file| { tokio::task::spawn(async move { println!("Running {:?}", test_file.relative_path); - if options.complete_mode { + if options.complete { run_complete_file(test_file).await?; } else if options.postgres_runner { run_test_file_with_postgres(test_file).await?; @@ -272,49 +273,54 @@ fn read_dir_recursive>(path: P) -> Box for more details +#[derive(Parser, Debug)] +#[clap(author, version, about, long_about= None)] struct Options { - // regex like - /// arguments passed to the program which are treated as - /// cargo test filter (substring match on filenames) - filters: Vec, - - /// Auto complete mode to fill out expected results - complete_mode: bool, - - /// Run Postgres compatibility tests with Postgres runner + #[clap(long, help = "Auto complete mode to fill out expected results")] + complete: bool, + + #[clap( + long, + env = "PG_COMPAT", + help = "Run Postgres compatibility tests with Postgres runner" + )] postgres_runner: bool, - /// Include tpch files + #[clap(long, env = "INCLUDE_TPCH", help = "Include tpch files")] include_tpch: bool, -} -impl Options { - fn new() -> Self { - let args: Vec<_> = std::env::args().collect(); - - let complete_mode = args.iter().any(|a| a == "--complete"); - let postgres_runner = std::env::var("PG_COMPAT").map_or(false, |_| true); - let include_tpch = std::env::var("INCLUDE_TPCH").map_or(false, |_| true); - - // treat args after the first as filters to run (substring matching) - let filters = if !args.is_empty() { - args.into_iter() - .skip(1) - // ignore command line arguments like `--complete` - .filter(|arg| !arg.as_str().starts_with("--")) - .collect::>() - } else { - vec![] - }; + #[clap( + action, + help = "regex like arguments passed to the program which are treated as cargo test filter (substring match on filenames)" + )] + filters: Vec, - Self { - filters, - complete_mode, - postgres_runner, - include_tpch, - } - } + #[clap( + long, + help = "IGNORED (for compatibility with built in rust test runner)" + )] + format: Option, + + #[clap( + short = 'Z', + long, + help = "IGNORED (for compatibility with built in rust test runner)" + )] + z_options: Option, + + #[clap( + long, + help = "IGNORED (for compatibility with built in rust test runner)" + )] + show_output: bool, +} +impl Options { /// Because this test can be run as a cargo test, commands like /// /// ```shell From b56e930177a86c4affead0231060c819f31b4711 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Mon, 20 Nov 2023 14:32:44 -0500 Subject: [PATCH 2/3] tomlfmt --- datafusion/sqllogictest/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/sqllogictest/Cargo.toml b/datafusion/sqllogictest/Cargo.toml index 684cc3470ef3..82b8c8adde8d 100644 --- a/datafusion/sqllogictest/Cargo.toml +++ b/datafusion/sqllogictest/Cargo.toml @@ -36,6 +36,7 @@ async-trait = { workspace = true } bigdecimal = { workspace = true } bytes = { version = "1.4.0", optional = true } chrono = { workspace = true, optional = true } +clap = { version = "4.4.8", features = ["derive", "env"] } datafusion = { path = "../core", version = "33.0.0" } datafusion-common = { workspace = true } futures = { version = "0.3.28" } @@ -52,7 +53,6 @@ tempfile = { workspace = true } thiserror = { workspace = true } tokio = { version = "1.0" } tokio-postgres = { version = "0.7.7", optional = true } -clap = { version = "4.4.8", features = ["derive", "env"] } [features] avro = ["datafusion/avro"] From 950611678ac575c616b8770cfc590381d508489c Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Mon, 1 Jan 2024 08:28:16 -0500 Subject: [PATCH 3/3] Add warning messages to try and reduce confusion --- datafusion/sqllogictest/bin/sqllogictests.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/datafusion/sqllogictest/bin/sqllogictests.rs b/datafusion/sqllogictest/bin/sqllogictests.rs index 5e3571c8786a..ffae144eae84 100644 --- a/datafusion/sqllogictest/bin/sqllogictests.rs +++ b/datafusion/sqllogictest/bin/sqllogictests.rs @@ -79,6 +79,7 @@ async fn run_tests() -> Result<()> { env_logger::init(); let options: Options = clap::Parser::parse(); + options.warn_on_ignored(); // Run all tests in parallel, reporting failures at the end // @@ -365,4 +366,19 @@ impl Options { let file_name = path.file_name().unwrap().to_str().unwrap().to_string(); !self.postgres_runner || file_name.starts_with(PG_COMPAT_FILE_PREFIX) } + + /// Logs warning messages to stdout if any ignored options are passed + fn warn_on_ignored(&self) { + if self.format.is_some() { + println!("WARNING: Ignoring `--format` compatibility option"); + } + + if self.z_options.is_some() { + println!("WARNING: Ignoring `-Z` compatibility option"); + } + + if self.show_output { + println!("WARNING: Ignoring `--show-output` compatibility option"); + } + } }