From 8e2d750c093d22f85323fe9a846dad3814fa3dba Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Thu, 5 Nov 2026 23:17:07 +0000 Subject: [PATCH 1/2] fix: give first-run errors working advice SQ0008 help names the refused divergence and lists only the fixes that apply to the caller's flags, policy and session. --builtin-policy takes TIMEZONE and the result-neutral session parameters. Policy errors suggest the nearest divergence id or name a partial entry's missing fields. Schema shape errors name the key, relation or column at fault, and the --help footer documents every command's exit status. --- .../src/diagnostics/constants.rs | 6 +- .../sqlquack-core/src/refusals/constants.rs | 4 + .../src/refusals/main/refused_divergence.rs | 3 +- crates/sqlquack-core/src/refusals/mod.rs | 1 + crates/sqlquack-core/src/refusals/models.rs | 12 ++ .../src/policy/_helpers/parsing.rs | 64 +++++++++- .../sqlquack-policy/src/policy/constants.rs | 6 + .../src/policy/tests/test_parse_policy.rs | 16 +++ .../main/session_conversion_options.rs | 29 ++++- .../src/types/_helpers/mod.rs | 1 + .../src/types/_helpers/schema_shape.rs | 82 ++++++++++++ .../sqlquack-semantics/src/types/constants.rs | 9 ++ .../src/types/main/parse_schema.rs | 9 +- .../src/cli/_helpers/convert_command.rs | 11 +- crates/sqlquack/src/cli/_helpers/evidence.rs | 109 +++++++++++++++- .../src/cli/_helpers/function_command.rs | 7 +- crates/sqlquack/src/cli/constants.rs | 6 +- .../sqlquack/src/cli/main/convert_report.rs | 2 +- crates/sqlquack/tests/cli/helpers.rs | 57 ++++++++- crates/sqlquack/tests/cli/test_convert.rs | 120 ++++++++++++++++-- crates/sqlquack/tests/cli/test_types.rs | 15 +++ .../docs/docs/diagnostics/reference.mdx | 2 +- 22 files changed, 535 insertions(+), 36 deletions(-) create mode 100644 crates/sqlquack-core/src/refusals/constants.rs create mode 100644 crates/sqlquack-semantics/src/types/_helpers/schema_shape.rs diff --git a/crates/sqlquack-core/src/diagnostics/constants.rs b/crates/sqlquack-core/src/diagnostics/constants.rs index a93eb1e12..1757bdcd4 100644 --- a/crates/sqlquack-core/src/diagnostics/constants.rs +++ b/crates/sqlquack-core/src/diagnostics/constants.rs @@ -148,9 +148,9 @@ pub const CODES: &[CodeEntry] = &[ alias: None, title: "the conversion needs a known divergence the policy refuses", help: Some( - "allow the divergence under [divergences.] in the policy file (--policy), drop \ - --strict or --loud-divergences-only, or confirm its assumption (--utc-session, \ - --confirm-declared-offsets, --python-runtime); --builtin-policy refuses every one", + "the help names the divergence and the fixes that work in the current mode: allow \ + it in the policy file, drop --strict or --loud-divergences-only, or confirm its \ + assumption; --builtin-policy refuses every one", ), explanation: "The construct at the span converts only by relying on the known \ divergence the label names, where DuckDB may raise or differ from Snowflake as the \ diff --git a/crates/sqlquack-core/src/refusals/constants.rs b/crates/sqlquack-core/src/refusals/constants.rs new file mode 100644 index 000000000..fbef1586a --- /dev/null +++ b/crates/sqlquack-core/src/refusals/constants.rs @@ -0,0 +1,4 @@ +//! The wording refusals share with the code that reads them back. + +/// The words before the divergence id in a refused divergence's reason. +pub const NEEDS_DIVERGENCE: &str = " needs known divergence `"; diff --git a/crates/sqlquack-core/src/refusals/main/refused_divergence.rs b/crates/sqlquack-core/src/refusals/main/refused_divergence.rs index 1fb467857..bb40b4e10 100644 --- a/crates/sqlquack-core/src/refusals/main/refused_divergence.rs +++ b/crates/sqlquack-core/src/refusals/main/refused_divergence.rs @@ -1,13 +1,14 @@ //! `refused_divergence`: the refusal of a construct whose conversion needs a refused divergence. use crate::classification::models::Construct; +use crate::refusals::constants::NEEDS_DIVERGENCE; use crate::refusals::models::{Refusal, RefusalCategory}; /// The construct converts only by relying on `divergence`, which the policy refuses in this mode. pub fn refused_divergence(construct: Construct, divergence: &str) -> Refusal { Refusal { reason: format!( - "{} needs known divergence `{divergence}`, which the policy refuses in this mode", + "{}{NEEDS_DIVERGENCE}{divergence}`, which the policy refuses in this mode", construct.key() ), construct: Some(construct), diff --git a/crates/sqlquack-core/src/refusals/mod.rs b/crates/sqlquack-core/src/refusals/mod.rs index 7ade40c66..927950b3f 100644 --- a/crates/sqlquack-core/src/refusals/mod.rs +++ b/crates/sqlquack-core/src/refusals/mod.rs @@ -1,6 +1,7 @@ //! Structured refusals: what SQLQuack will not convert, where, and why. pub(crate) mod _helpers; +pub mod constants; pub mod main; pub mod models; pub mod types; diff --git a/crates/sqlquack-core/src/refusals/models.rs b/crates/sqlquack-core/src/refusals/models.rs index b1be71471..af6986969 100644 --- a/crates/sqlquack-core/src/refusals/models.rs +++ b/crates/sqlquack-core/src/refusals/models.rs @@ -6,6 +6,7 @@ use serde::Serialize; use crate::classification::models::Construct; use crate::parsing::models::SourceLocation; +use crate::refusals::constants::NEEDS_DIVERGENCE; /// Why a construct or statement is refused. #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize)] @@ -64,6 +65,17 @@ pub struct Refusal { pub reason: String, } +impl Refusal { + /// The id of the known divergence a refused divergence needs; none for other refusals. + pub fn divergence(&self) -> Option<&str> { + if self.category != RefusalCategory::DivergenceRefused { + return None; + } + let (_, rest) = self.reason.split_once(NEEDS_DIVERGENCE)?; + rest.split_once('`').map(|(id, _)| id) + } +} + /// What the rule catalogue says about one construct. #[derive(Clone, Debug, PartialEq, Eq)] pub enum Disposition { diff --git a/crates/sqlquack-policy/src/policy/_helpers/parsing.rs b/crates/sqlquack-policy/src/policy/_helpers/parsing.rs index ce7f474e4..f7be64614 100644 --- a/crates/sqlquack-policy/src/policy/_helpers/parsing.rs +++ b/crates/sqlquack-policy/src/policy/_helpers/parsing.rs @@ -1,12 +1,17 @@ //! Policy text to a validated policy. use crate::policy::_helpers::validation::problems; +use crate::policy::constants::{DIVERGENCE_FIELDS, MAX_SPELLING_DISTANCE}; use crate::policy::errors::PolicyError; use crate::policy::models::Policy; pub(crate) fn parse_policy(text: &str) -> Result { - let policy: Policy = - toml::from_str(text).map_err(|error| PolicyError::Schema(error.to_string()))?; + let policy: Policy = toml::from_str(text).map_err(|error| { + PolicyError::Schema(match text.parse::() { + Ok(table) => incomplete_divergence(&table).unwrap_or_else(|| error.to_string()), + Err(_not_toml) => error.to_string(), + }) + })?; let found = problems(&policy); if found.is_empty() { Ok(policy) @@ -14,3 +19,58 @@ pub(crate) fn parse_policy(text: &str) -> Result { Err(PolicyError::Invalid(found)) } } + +/// Why a `[divergences.]` table lacking fields fails: a misspelt id, or a partial entry. +fn incomplete_divergence(table: &toml::Table) -> Option { + let divergences = table.get("divergences")?.as_table()?; + let missing = |entry: &toml::Value| -> Vec<&str> { + DIVERGENCE_FIELDS + .iter() + .copied() + .filter(|field| entry.get(field).is_none()) + .collect() + }; + let (id, absent) = divergences + .iter() + .map(|(id, entry)| (id, missing(entry))) + .find(|(_, absent)| !absent.is_empty())?; + let nearest = divergences + .iter() + .filter(|(other, entry)| *other != id && missing(entry).is_empty()) + .map(|(other, _)| (edit_distance(id, other), other)) + .filter(|(distance, _)| *distance <= MAX_SPELLING_DISTANCE) + .min(); + Some(match nearest { + Some((_, known)) => { + format!("unknown divergence `{id}` in [divergences.{id}]; did you mean `{known}`?") + } + None => format!( + "[divergences.{id}] is missing field{} {}: a policy file declares each divergence in \ + full, so copy its entry from the repository's sqlquack.toml, or check the id's \ + spelling", + if absent.len() == 1 { "" } else { "s" }, + absent + .iter() + .map(|field| format!("`{field}`")) + .collect::>() + .join(", ") + ), + }) +} + +/// The Levenshtein distance between `left` and `right`, by character. +fn edit_distance(left: &str, right: &str) -> usize { + let right: Vec = right.chars().collect(); + let mut previous: Vec = (0..=right.len()).collect(); + for (row, left_char) in left.chars().enumerate() { + let mut current = vec![row + 1; right.len() + 1]; + for (column, right_char) in right.iter().enumerate() { + let substitution = previous[column] + usize::from(left_char != *right_char); + current[column + 1] = substitution + .min(previous[column + 1] + 1) + .min(current[column] + 1); + } + previous = current; + } + previous[right.len()] +} diff --git a/crates/sqlquack-policy/src/policy/constants.rs b/crates/sqlquack-policy/src/policy/constants.rs index 82dd1dfbc..f55eae1a8 100644 --- a/crates/sqlquack-policy/src/policy/constants.rs +++ b/crates/sqlquack-policy/src/policy/constants.rs @@ -42,3 +42,9 @@ pub const DEFAULT_SESSION_PROFILE: &str = "utc-v1"; /// The only schema version this build understands. pub const SCHEMA_VERSION: u32 = 1; + +/// The fields every `[divergences.]` entry must give. +pub const DIVERGENCE_FIELDS: [&str; 5] = ["constructs", "default", "strict", "reason", "failure"]; + +/// The most character edits between a misspelt divergence id and the one it suggests. +pub const MAX_SPELLING_DISTANCE: usize = 2; diff --git a/crates/sqlquack-policy/src/policy/tests/test_parse_policy.rs b/crates/sqlquack-policy/src/policy/tests/test_parse_policy.rs index ee4700b74..5d184d464 100644 --- a/crates/sqlquack-policy/src/policy/tests/test_parse_policy.rs +++ b/crates/sqlquack-policy/src/policy/tests/test_parse_policy.rs @@ -16,6 +16,22 @@ fn given_policy_text_when_parsing_then_schema_and_rules_are_enforced() { with: "schema_version = 1\noverrides = true", expected_error: Some("unknown field `overrides`"), }, + PolicyTestCase { + description: "a misspelt divergence id names the one it is near", + replace: "failure = \"value\"\n", + with: "failure = \"value\"\n\n[divergences.hsah]\ndefault = \"warn\"\n", + expected_error: Some( + "unknown divergence `hsah` in [divergences.hsah]; did you mean `hash`?", + ), + }, + PolicyTestCase { + description: "a partial divergence entry names the fields it lacks", + replace: "failure = \"value\"\n", + with: "failure = \"value\"\n\n[divergences.json_fidelity]\ndefault = \"warn\"\n", + expected_error: Some( + "[divergences.json_fidelity] is missing fields `constructs`, `strict`, `reason`, `failure`", + ), + }, PolicyTestCase { description: "an unknown comparator key is rejected", replace: "timestamp_tz = \"instant\"", diff --git a/crates/sqlquack-rules/src/session/main/session_conversion_options.rs b/crates/sqlquack-rules/src/session/main/session_conversion_options.rs index a7583d7f9..d1218a6df 100644 --- a/crates/sqlquack-rules/src/session/main/session_conversion_options.rs +++ b/crates/sqlquack-rules/src/session/main/session_conversion_options.rs @@ -5,7 +5,8 @@ use std::collections::BTreeMap; use crate::conversion::constants::SESSION_TIMEZONE_UTC; use crate::conversion::models::ConversionOptions; use sqlquack_policy::policy::constants::{ - DEFAULT_SESSION_PROFILE, QUOTED_IDENTIFIERS_IGNORE_CASE, SESSION_TIME_ZONE, TIMEZONE_PARAMETER, + DEFAULT_SESSION_PROFILE, QUOTED_IDENTIFIERS_IGNORE_CASE, RESULT_NEUTRAL_SESSION_PARAMETERS, + SESSION_TIME_ZONE, TIMEZONE_PARAMETER, }; use sqlquack_policy::policy::models::{Policy, SessionValue}; @@ -21,13 +22,19 @@ pub fn session_conversion_options( parameters: &BTreeMap, ) -> Result<(ConversionOptions, String), String> { let Some(policy) = policy else { - return match parameters.is_empty() { + let unknown: Vec<&str> = parameters + .keys() + .map(String::as_str) + .filter(|name| !builtin_accepts(name)) + .collect(); + return match unknown.is_empty() { true => Ok((ConversionOptions::default(), BUILTIN_PROFILE.to_owned())), - false => Err( - "the built-in default records no session profiles, so it takes no session \ - parameters" - .to_owned(), - ), + false => Err(format!( + "the built-in default records no session profiles, so it takes only TIMEZONE and \ + the result-neutral parameters, not {}; pass --policy with a policy file whose \ + session profiles cover them, such as the repository's sqlquack.toml", + unknown.join(", ") + )), }; }; let profile = matching_profile( @@ -53,3 +60,11 @@ pub fn session_conversion_options( } Ok((options, profile)) } + +/// Whether the built-in default, which records no session profiles, takes parameter `name`. +fn builtin_accepts(name: &str) -> bool { + name == TIMEZONE_PARAMETER + || RESULT_NEUTRAL_SESSION_PARAMETERS + .iter() + .any(|(neutral, _)| *neutral == name) +} diff --git a/crates/sqlquack-semantics/src/types/_helpers/mod.rs b/crates/sqlquack-semantics/src/types/_helpers/mod.rs index d3025096b..693f39f69 100644 --- a/crates/sqlquack-semantics/src/types/_helpers/mod.rs +++ b/crates/sqlquack-semantics/src/types/_helpers/mod.rs @@ -1 +1,2 @@ pub(crate) mod schema_file; +pub(crate) mod schema_shape; diff --git a/crates/sqlquack-semantics/src/types/_helpers/schema_shape.rs b/crates/sqlquack-semantics/src/types/_helpers/schema_shape.rs new file mode 100644 index 000000000..c3b2f905e --- /dev/null +++ b/crates/sqlquack-semantics/src/types/_helpers/schema_shape.rs @@ -0,0 +1,82 @@ +//! What is wrong with a schema file's shape, in its own terms, where JSON decoding fails. + +use serde_json::{Map, Value}; + +use crate::types::constants::{COLUMN_KEYS, REQUIRED_COLUMN_KEYS, SCHEMA_SECTIONS}; + +/// The first shape problem of the schema `top`, naming the key, relation or column at fault. +pub(crate) fn shape_problem(top: &Value) -> Option { + let Value::Object(top) = top else { + return Some( + "the schema must be a JSON object: relations by name, or `relations` and `functions`" + .to_owned(), + ); + }; + let sectioned = SCHEMA_SECTIONS + .iter() + .any(|section| top.contains_key(*section)); + if sectioned + && let Some(key) = top + .keys() + .find(|key| !SCHEMA_SECTIONS.contains(&key.as_str())) + { + return Some(format!( + "unknown top-level key `{key}`; a schema with sections holds only `relations` and \ + `functions`" + )); + } + let relations = match sectioned { + true => top.get("relations")?.as_object()?, + false => top, + }; + relations + .iter() + .find_map(|(relation, columns)| relation_problem(relation, columns)) +} + +/// What is wrong with `relation`'s columns, if anything. +fn relation_problem(relation: &str, columns: &Value) -> Option { + let Value::Array(columns) = columns else { + return Some(format!( + "relation {relation} must list its columns, as [[\"NAME\", \"TYPE\"], …]" + )); + }; + columns.iter().enumerate().find_map(|(index, column)| { + column_problem( + &format!("relation {relation}, column {}", index + 1), + column, + ) + }) +} + +/// What is wrong with the column at `place`, if anything. +fn column_problem(place: &str, column: &Value) -> Option { + match column { + Value::Array(pair) => match pair.as_slice() { + [Value::String(_), Value::String(_)] => None, + [Value::String(name)] => Some(format!("{place} ({name}) has no type")), + _ => Some(format!("{place} must be [\"NAME\", \"TYPE\"], two strings")), + }, + Value::Object(fields) => object_column_problem(place, fields), + _ => Some(format!( + "{place} must be [\"NAME\", \"TYPE\"] or {{\"name\": …, \"type\": …}}" + )), + } +} + +/// What is wrong with the column at `place` written as an object, if anything. +fn object_column_problem(place: &str, fields: &Map) -> Option { + if let Some(key) = fields + .keys() + .find(|key| !COLUMN_KEYS.contains(&key.as_str())) + { + return Some(format!( + "{place} has unknown key `{key}`; a column takes {}", + COLUMN_KEYS.join(", ") + )); + } + REQUIRED_COLUMN_KEYS + .iter() + .find(|key| !fields.get(**key).is_some_and(Value::is_string)) + .map(|key| format!("{place} needs `{key}` as a string")) +} diff --git a/crates/sqlquack-semantics/src/types/constants.rs b/crates/sqlquack-semantics/src/types/constants.rs index 4777c514e..52c1c1320 100644 --- a/crates/sqlquack-semantics/src/types/constants.rs +++ b/crates/sqlquack-semantics/src/types/constants.rs @@ -2,3 +2,12 @@ /// The offsets a schema may declare for a TIMESTAMP_TZ source column, all meaning +00:00. pub const DECLARABLE_OFFSETS: &[&str] = &["+00:00", "UTC"]; + +/// The top-level keys of a schema file written in sections. +pub const SCHEMA_SECTIONS: [&str; 2] = ["relations", "functions"]; + +/// The keys a column written as an object may hold. +pub const COLUMN_KEYS: [&str; 4] = ["name", "type", "typed_leaves", "offset"]; + +/// The keys a column written as an object must give, as strings. +pub const REQUIRED_COLUMN_KEYS: [&str; 2] = ["name", "type"]; diff --git a/crates/sqlquack-semantics/src/types/main/parse_schema.rs b/crates/sqlquack-semantics/src/types/main/parse_schema.rs index 7842d05c6..49989c3b4 100644 --- a/crates/sqlquack-semantics/src/types/main/parse_schema.rs +++ b/crates/sqlquack-semantics/src/types/main/parse_schema.rs @@ -5,13 +5,18 @@ use std::collections::BTreeMap; use sqlquack_core::provenance::models::ProvenanceLog; use crate::types::_helpers::schema_file::{RawSchema, typed_columns, typed_function}; +use crate::types::_helpers::schema_shape::shape_problem; use crate::types::errors::SchemaError; use crate::types::models::Schema; /// The schema `text` declares; a relation or function using a type SQLQuack does not know is left out. pub fn parse_schema(text: &str) -> Result { - let raw: RawSchema = - serde_json::from_str(text).map_err(|error| SchemaError::Shape(error.to_string()))?; + let raw: RawSchema = serde_json::from_str(text).map_err(|error| { + SchemaError::Shape(match serde_json::from_str::(text) { + Ok(top) => shape_problem(&top).unwrap_or_else(|| error.to_string()), + Err(_not_json) => error.to_string(), + }) + })?; let (relations, functions) = match raw { RawSchema::Sections(sections) => (sections.relations, sections.functions), RawSchema::Relations(relations) => (relations, BTreeMap::new()), diff --git a/crates/sqlquack/src/cli/_helpers/convert_command.rs b/crates/sqlquack/src/cli/_helpers/convert_command.rs index fda0b0e3b..853e50caf 100644 --- a/crates/sqlquack/src/cli/_helpers/convert_command.rs +++ b/crates/sqlquack/src/cli/_helpers/convert_command.rs @@ -28,9 +28,10 @@ use crate::analysis::models::Analysis; use crate::cli::_helpers::evidence::caller_choices; use crate::cli::models::{ CheckArguments, CheckReport, ConvertArguments, ConvertRefusal, ConvertReport, ExplainArguments, - OverrideListing, + OverrideListing, PolicyArguments, }; +use crate::cli::_helpers::evidence::with_divergence_help; use crate::cli::main::convert_report::convert_report; /// The path naming standard input. @@ -257,7 +258,7 @@ pub(crate) fn analysis_report( analysis: &Analysis, sql: &str, (schema, options, policy): (&Schema, &ConversionOptions, Option<&Policy>), - (parameters, profile): (&BTreeMap, String), + (arguments, parameters, profile): (&PolicyArguments, &BTreeMap, String), ) -> Result { let converted = analysis.parsed && analysis.refusals.is_empty(); let provenance = merge_provenance(analysis.conversions.iter().map(|done| &done.provenance)); @@ -299,7 +300,11 @@ pub(crate) fn analysis_report( refusals: analysis .refusals .iter() - .zip(diagnose(&analysis.refusals, sql)) + .zip(with_divergence_help( + &analysis.refusals, + diagnose(&analysis.refusals, sql), + (policy, arguments, parameters), + )) .map(convert_refusal) .collect(), python_functions: python_functions(&provenance.divergences, schema)?, diff --git a/crates/sqlquack/src/cli/_helpers/evidence.rs b/crates/sqlquack/src/cli/_helpers/evidence.rs index 51681546e..0d2b74c97 100644 --- a/crates/sqlquack/src/cli/_helpers/evidence.rs +++ b/crates/sqlquack/src/cli/_helpers/evidence.rs @@ -1,18 +1,23 @@ -//! Loading the policy and cases for evidence commands. +//! Loading the policy and cases for evidence commands, and SQ0008's help for refused divergences. +use std::collections::BTreeMap; use std::path::Path; +use sqlquack_core::diagnostics::models::Diagnostic; +use sqlquack_core::refusals::models::Refusal; use sqlquack_evidence::cases::main::load_cases::load_cases; use sqlquack_evidence::cases::models::LoadedCase; +use sqlquack_policy::policy::constants::{SESSION_TIME_ZONE, TIMEZONE_PARAMETER}; use sqlquack_policy::policy::main::load_policy::load_policy; -use sqlquack_policy::policy::models::Policy; +use sqlquack_policy::policy::models::{DivergenceFailure, DivergenceMode, Policy}; use sqlquack_rules::conversion::models::ConversionOptions; +use crate::analysis::main::assumption_id::assumption_id; use crate::analysis::main::confirm_assumption::confirm_assumption; use crate::analysis::main::refuse_value_divergences::refuse_value_divergences; use crate::cli::models::{EvidencePaths, PolicyArguments}; -use sqlquack_rules::conversion::constants::{DECLARED_OFFSET, PROJECT_UDF}; +use sqlquack_rules::conversion::constants::{DECLARED_OFFSET, PROJECT_UDF, SESSION_TIMEZONE_UTC}; use sqlquack_rules::session::main::confirm_utc_session::confirm_utc_session; use sqlquack_rules::session::main::conversion_options::conversion_options; @@ -89,3 +94,101 @@ pub(crate) fn parse_percent(sample: &str) -> Result { Ok(_) | Err(_) => Err(invalid()), } } + +/// `diagnostics`, each SQ0008 one's help naming its divergence and the fixes that work here. +pub(crate) fn with_divergence_help( + refusals: &[Refusal], + diagnostics: Vec, + (policy, arguments, parameters): (Option<&Policy>, &PolicyArguments, &BTreeMap), +) -> Vec { + refusals + .iter() + .zip(diagnostics) + .map(|(refusal, mut diagnostic)| { + if let Some(id) = refusal.divergence() { + diagnostic.help = Some(divergence_help( + assumption_id(id), + (policy, arguments, parameters), + )); + } + diagnostic + }) + .collect() +} + +/// The fixes for divergence `id`, given the policy, the caller's flags and session parameters. +fn divergence_help( + id: &str, + (policy, arguments, parameters): (Option<&Policy>, &PolicyArguments, &BTreeMap), +) -> String { + let Some(policy) = policy else { + return format!( + "--builtin-policy refuses every known divergence; pass --policy with a policy file \ + that allows `{id}`, such as the repository's sqlquack.toml" + ); + }; + let file = arguments.policy.display(); + let Some(divergence) = policy.divergences.get(id) else { + return format!( + "{file} has no [divergences.{id}]; copy it from the repository's sqlquack.toml" + ); + }; + let zone = parameters + .get(TIMEZONE_PARAMETER) + .filter(|zone| *zone != SESSION_TIME_ZONE); + if let (SESSION_TIMEZONE_UTC, Some(zone)) = (id, zone) { + return format!( + "`{id}` holds only in a UTC session, and TIMEZONE = {zone}; convert for a UTC \ + session (--session TIMEZONE=UTC)" + ); + } + let mut fixes: Vec = Vec::new(); + let confirmation = match id { + SESSION_TIMEZONE_UTC => Some(("--utc-session", arguments.utc_session)), + DECLARED_OFFSET => Some(( + "--confirm-declared-offsets", + arguments.confirm_declared_offsets, + )), + PROJECT_UDF => Some(("--python-runtime", arguments.python_runtime)), + _ => None, + }; + let confirmable = match confirmation { + Some((flag, false)) => { + fixes.push(format!("confirm its assumption with {flag}")); + true + } + Some((_, true)) => true, + None => false, + }; + let mode = match arguments.strict { + true => divergence.strict, + false => divergence.default, + }; + if mode == DivergenceMode::Refuse && !confirmable { + let keys = match (arguments.strict, divergence.default) { + (true, DivergenceMode::Warn) => { + fixes.push("drop --strict".to_owned()); + "strict" + } + (true, DivergenceMode::Refuse) => "default and strict", + (false, _) => "default", + }; + fixes.push(format!( + "set {keys} = \"warn\" under [divergences.{id}] in {file}" + )); + } + let value = divergence.failure == DivergenceFailure::Value; + match (arguments.loud_divergences_only && value, fixes.is_empty()) { + (true, true) => format!( + "`{id}` can return a different value rather than raise, which \ + --loud-divergences-only refuses; drop that flag" + ), + (true, false) => format!( + "drop --loud-divergences-only, since `{id}` can return a different value rather \ + than raise, and {}", + fixes.join(", or ") + ), + (false, false) => format!("to rely on `{id}`: {}", fixes.join(", or ")), + (false, true) => format!("allow `{id}` under [divergences.{id}] in {file}"), + } +} diff --git a/crates/sqlquack/src/cli/_helpers/function_command.rs b/crates/sqlquack/src/cli/_helpers/function_command.rs index 3b1a57c28..eac31d2e6 100644 --- a/crates/sqlquack/src/cli/_helpers/function_command.rs +++ b/crates/sqlquack/src/cli/_helpers/function_command.rs @@ -20,6 +20,7 @@ use crate::analysis::main::analyze_function::analyze_function; use crate::census::main::resolve_references::resolve_references; use crate::cli::_helpers::evidence::caller_choices; use crate::cli::_helpers::evidence::chosen_policy; +use crate::cli::_helpers::evidence::with_divergence_help; use crate::cli::_helpers::schema::load_schema; use crate::cli::models::FunctionArguments; use sqlquack_rules::session::main::parse_session::parse_session; @@ -173,7 +174,11 @@ fn convert( (&schema, &options), ) .map_err(|refusals| Failure::Refused { - diagnostics: diagnose(&refusals, &body), + diagnostics: with_divergence_help( + &refusals, + diagnose(&refusals, &body), + (policy.as_ref(), &arguments.conversion, parameters), + ), body, }) } diff --git a/crates/sqlquack/src/cli/constants.rs b/crates/sqlquack/src/cli/constants.rs index d3dc0198e..317d5aca8 100644 --- a/crates/sqlquack/src/cli/constants.rs +++ b/crates/sqlquack/src/cli/constants.rs @@ -1,4 +1,8 @@ //! Command-line text shared by every command. /// What each exit status means, shown after the top-level help. -pub const EXIT_STATUS_HELP: &str = "Exit status of convert, function, census and index:\n 0 the command succeeded\n 1 something was refused; the reasons are on standard error\n 2 the command could not run: bad arguments, an unreadable file, or input without SQL"; +pub const EXIT_STATUS_HELP: &str = "Exit status of check, convert and function:\n \ + 0 the command succeeded\n \ + 1 check found a problem, or something was refused; the reasons are on standard error\n \ + 2 the command could not run: bad arguments, an unreadable file, or input without SQL\n\ + census and index exit 0 when they finish, refused files included, and 2 when they cannot run."; diff --git a/crates/sqlquack/src/cli/main/convert_report.rs b/crates/sqlquack/src/cli/main/convert_report.rs index 177b4a6cc..374271984 100644 --- a/crates/sqlquack/src/cli/main/convert_report.rs +++ b/crates/sqlquack/src/cli/main/convert_report.rs @@ -48,6 +48,6 @@ pub fn convert_report(arguments: &ConvertArguments, sql: &str) -> Result Vec { .cloned() .unwrap_or_default() } + +/// The first refusal's help converting `sql` under the repository policy, its path as ``. +pub(crate) fn first_refusal_help(sql: &str, flags: &[&str]) -> String { + let policy = repository_root().join("sqlquack.toml"); + let output = Command::new(env!("CARGO_BIN_EXE_sqlquack")) + .args(["convert", "--json", "--policy"]) + .arg(&policy) + .args(flags) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .and_then(|mut child| { + child + .stdin + .take() + .expect("stdin") + .write_all(sql.as_bytes())?; + child.wait_with_output() + }) + .expect("sqlquack runs"); + let report: serde_json::Value = serde_json::from_slice(&output.stdout).expect("a JSON report"); + report["refusals"][0]["help"] + .as_str() + .unwrap_or_default() + .replace(&policy.display().to_string(), "") +} + +/// The exit code and standard error of converting a statement with the schema `text`. +pub(crate) fn schema_failure(text: &str) -> (Option, String) { + let directory = tempfile::tempdir().expect("temporary directory"); + let schema = directory.path().join("schema.json"); + std::fs::write(&schema, text).expect("schema written"); + let output = Command::new(env!("CARGO_BIN_EXE_sqlquack")) + .args(["convert", "--builtin-policy", "--schema"]) + .arg(&schema) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .and_then(|mut child| { + child + .stdin + .take() + .expect("stdin") + .write_all(b"SELECT 1 AS one")?; + child.wait_with_output() + }) + .expect("sqlquack runs"); + ( + output.status.code(), + String::from_utf8_lossy(&output.stderr).into_owned(), + ) +} diff --git a/crates/sqlquack/tests/cli/test_convert.rs b/crates/sqlquack/tests/cli/test_convert.rs index 3f52bec9d..4530ee709 100644 --- a/crates/sqlquack/tests/cli/test_convert.rs +++ b/crates/sqlquack/tests/cli/test_convert.rs @@ -1,14 +1,15 @@ use crate::helpers::{ convert_outcome, convert_streams, converted_column_names, converted_first_value, - converted_run_error, converted_value_over, override_failure, override_outcome, - policy_free_exit_code, python_outcome, python_report, python_rows, run_convert, run_with_input, + converted_run_error, converted_value_over, first_refusal_help, override_failure, + override_outcome, policy_free_exit_code, python_outcome, python_report, python_rows, + run_convert, run_with_input, schema_failure, }; use crate::test_types::{ ColumnNameTestCase, ConvertDiagnosticTestCase, ConvertOutcome, ConvertTestCase, ConvertedRunTestCase, CountedCallsTestCase, CrossingTestCase, DedicatedOverrideTestCase, - DeepChainTestCase, FailureKindTestCase, OutputFormatTestCase, OverrideFailureTestCase, - OverrideTestCase, PolicyFreeTestCase, PythonRefusalTestCase, PythonRunTestCase, - RunErrorTestCase, ValueTestCase, + DeepChainTestCase, DivergenceHelpTestCase, FailureKindTestCase, OutputFormatTestCase, + OverrideFailureTestCase, OverrideTestCase, PolicyFreeTestCase, PythonRefusalTestCase, + PythonRunTestCase, RunErrorTestCase, SchemaErrorTestCase, ValueTestCase, }; use sha2::{Digest, Sha256}; @@ -318,6 +319,26 @@ fn given_snowflake_sql_when_converting_then_duckdb_sql_or_named_refusals_are_pri mention: "gives QUOTED_IDENTIFIERS_IGNORE_CASE twice", expected_outcome: convert_outcome(2, false), }, + ConvertTestCase { + description: "the built-in default takes a UTC TIMEZONE and result-neutral parameters", + sql: "SELECT 1 AS total", + flags: &[ + "--builtin-policy", + "--session", + "TIMEZONE=UTC", + "--session", + "query_tag=nightly", + ], + mention: "converted under the built-in default", + expected_outcome: convert_outcome(0, true), + }, + ConvertTestCase { + description: "the built-in default refuses a parameter that can change a result, by name", + sql: "SELECT 1 AS total", + flags: &["--builtin-policy", "--session", "WEEK_START=1"], + mention: "not WEEK_START; pass --policy", + expected_outcome: convert_outcome(1, false), + }, ConvertTestCase { description: "comment-only input is a usage error", sql: "-- no statement here\n", @@ -1323,10 +1344,8 @@ fn given_refused_sql_when_converting_then_text_and_json_carry_the_same_coded_dia expected_text: "error[SQ0008]: the conversion needs a known divergence the policy refuses: \ `fn:HASH`\n --> :1:8\n |\n1 | SELECT HASH(1) AS h\n | ^^^^^^^ fn:HASH \ needs known divergence `hash`, which the policy refuses in this mode\n |\n = help: \ - allow the divergence under [divergences.] in the policy file (--policy), drop \ - --strict or --loud-divergences-only, or confirm its assumption (--utc-session, \ - --confirm-declared-offsets, --python-runtime); --builtin-policy refuses every \ - one\nsqlquack convert: refused, 1 refusal under the built-in default\n", + --builtin-policy refuses every known divergence; pass --policy with a policy \ + file that allows `hash`, such as the repository's sqlquack.toml\nsqlquack convert: refused, 1 refusal under the built-in default\n", expected_json: ("SQ0008", "divergence_refused", 1, 8), }, ConvertDiagnosticTestCase { @@ -1448,7 +1467,10 @@ fn given_output_flags_when_running_then_format_json_and_json_agree_and_help_list ); assert!( test_case.arguments != ["--help"] - || stdout.contains("Exit status of convert, function, census and index:\n 0 the command succeeded"), + || stdout.contains( + "census and index exit 0 when they finish, refused files included, and 2 \ + when they cannot run.", + ), "{}", test_case.description ); @@ -1606,3 +1628,81 @@ fn given_a_long_flat_chain_when_converting_then_it_converts_or_is_refused_within ); } } + +#[test] +fn given_a_refused_divergence_when_converting_then_its_help_names_it_and_only_fixes_that_work() { + let test_cases = [ + DivergenceHelpTestCase { + description: "strict mode refusing what the default allows: --utc-session cannot help", + sql: "SELECT CURRENT_TIMESTAMP AS t", + flags: &["--strict", "--utc-session"], + expected_help: "to rely on `current_time`: drop --strict, or set strict = \"warn\" \ + under [divergences.current_time] in ", + }, + DivergenceHelpTestCase { + description: "refused by default: only the policy file can allow it", + sql: "SELECT HASH(1) AS h", + flags: &[], + expected_help: "to rely on `hash`: set default = \"warn\" under [divergences.hash] in \ + ", + }, + DivergenceHelpTestCase { + description: "refused in both modes under --strict: dropping --strict is not enough", + sql: "SELECT HASH(1) AS h", + flags: &["--strict"], + expected_help: "to rely on `hash`: set default and strict = \"warn\" under \ + [divergences.hash] in ", + }, + DivergenceHelpTestCase { + description: "a value divergence under --loud-divergences-only", + sql: "SELECT CURRENT_TIMESTAMP AS t", + flags: &["--loud-divergences-only"], + expected_help: "`current_time` can return a different value rather than raise, which \ + --loud-divergences-only refuses; drop that flag", + }, + ]; + for test_case in test_cases { + assert_eq!( + first_refusal_help(test_case.sql, test_case.flags), + test_case.expected_help, + "{}", + test_case.description + ); + } +} + +#[test] +fn given_a_malformed_schema_when_converting_then_the_error_names_what_is_wrong_in_schema_terms() { + let test_cases = [ + SchemaErrorTestCase { + description: "a column without a type", + schema: r#"{"ORDERS": [["A"]]}"#, + expected_error: "relation ORDERS, column 1 (A) has no type", + }, + SchemaErrorTestCase { + description: "an unknown key beside the sections", + schema: r#"{"relations": {}, "tables": {}}"#, + expected_error: "unknown top-level key `tables`", + }, + SchemaErrorTestCase { + description: "an unknown key in a column object", + schema: r#"{"ORDERS": [{"name": "A", "type": "NUMBER", "nullable": true}]}"#, + expected_error: "relation ORDERS, column 1 has unknown key `nullable`", + }, + SchemaErrorTestCase { + description: "relations not listed as columns", + schema: r#"{"ORDERS": {"A": "NUMBER"}}"#, + expected_error: "relation ORDERS must list its columns", + }, + ]; + for test_case in test_cases { + let (exit_code, stderr) = schema_failure(test_case.schema); + + assert_eq!(exit_code, Some(2), "{}: {stderr}", test_case.description); + assert!( + stderr.contains(test_case.expected_error) && !stderr.contains("untagged"), + "{}: {stderr}", + test_case.description + ); + } +} diff --git a/crates/sqlquack/tests/cli/test_types.rs b/crates/sqlquack/tests/cli/test_types.rs index 18007da69..c5d96e020 100644 --- a/crates/sqlquack/tests/cli/test_types.rs +++ b/crates/sqlquack/tests/cli/test_types.rs @@ -278,3 +278,18 @@ pub(crate) struct DeepChainTestCase { /// Text standard error must hold, such as the refusal's code. pub(crate) expected_mention: &'static str, } + +/// A statement refused for a divergence under some flags, and the help it must give. +pub(crate) struct DivergenceHelpTestCase { + pub(crate) description: &'static str, + pub(crate) sql: &'static str, + pub(crate) flags: &'static [&'static str], + pub(crate) expected_help: &'static str, +} + +/// A malformed schema file and the words its error must use. +pub(crate) struct SchemaErrorTestCase { + pub(crate) description: &'static str, + pub(crate) schema: &'static str, + pub(crate) expected_error: &'static str, +} diff --git a/website/src/content/docs/docs/diagnostics/reference.mdx b/website/src/content/docs/docs/diagnostics/reference.mdx index 7859a9eed..4b3d01460 100644 --- a/website/src/content/docs/docs/diagnostics/reference.mdx +++ b/website/src/content/docs/docs/diagnostics/reference.mdx @@ -128,7 +128,7 @@ echo "SELECT 1 AS one" | sqlquack convert --builtin-policy --session WEEK_START= The construct at the span converts only by relying on the known divergence the label names, where DuckDB may raise or differ from Snowflake as the policy file describes. The policy refuses that divergence in the current mode, so the statement is refused. Allow it in the policy file, use a mode that allows it, or confirm the assumption it rests on; otherwise rewrite the statement without the construct. -**Help:** allow the divergence under [divergences.<id>] in the policy file (--policy), drop --strict or --loud-divergences-only, or confirm its assumption (--utc-session, --confirm-declared-offsets, --python-runtime); --builtin-policy refuses every one +**Help:** the help names the divergence and the fixes that work in the current mode: allow it in the policy file, drop --strict or --loud-divergences-only, or confirm its assumption; --builtin-policy refuses every one **Example:** From 1aae4b2a67554da03bd9555c6b70542710928e6b Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Thu, 5 Nov 2026 23:27:53 +0000 Subject: [PATCH 2/2] test: expect the built-in default to take TIMEZONE=UTC in the playground The playground shares the command line's session handling, so the built-in default now converts under TIMEZONE=UTC and still refuses WEEK_START with SQ0007. --- website/tests/playground.test.mjs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/website/tests/playground.test.mjs b/website/tests/playground.test.mjs index cd7852c51..8704d135d 100644 --- a/website/tests/playground.test.mjs +++ b/website/tests/playground.test.mjs @@ -66,8 +66,9 @@ test('the policy and session parameters choose the profile, or refuse, as on the assert.equal(monday.status, 'fail'); assert.deepEqual(monday.diagnostics.map((d) => d.code), ['SQ0007']); assert.equal(monday.statements, undefined); - // The built-in default records no profiles, so any session parameter is refused. - assert.deepEqual(translate({ session: 'TIMEZONE=UTC' }).diagnostics.map((d) => d.code), ['SQ0007']); + // The built-in default records no profiles: it takes TIMEZONE and result-neutral parameters only. + assert.equal(translate({ session: 'TIMEZONE=UTC' }).profile, 'the built-in default'); + assert.deepEqual(translate({ session: 'WEEK_START=1' }).diagnostics.map((d) => d.code), ['SQ0007']); assert.match(translate({ policy: 'schema_version = 1\nnot_a_key = 2' }).error, /^policy: /); assert.match(translate({ session: 'TIMEZONE' }).error, /NAME=VALUE/); });