diff --git a/crates/sqlquack-syntax-coverage/src/check/_helpers/join.rs b/crates/sqlquack-syntax-coverage/src/check/_helpers/join.rs index 50f13d9eb..2d8801066 100644 --- a/crates/sqlquack-syntax-coverage/src/check/_helpers/join.rs +++ b/crates/sqlquack-syntax-coverage/src/check/_helpers/join.rs @@ -51,20 +51,44 @@ fn receipts_by_case(coverage: &SyntaxCoverage) -> BTreeMap<&str, Vec<&Receipt>> receipts } +/// The receipts of each statement, by its SQL, setup and session hashes, whatever the case id. +fn receipts_by_statement(coverage: &SyntaxCoverage) -> BTreeMap<(&str, &str, &str), Vec<&Receipt>> { + let mut receipts: BTreeMap<(&str, &str, &str), Vec<&Receipt>> = BTreeMap::new(); + for receipt in &coverage.receipts { + receipts + .entry(( + receipt.sql_sha256.as_str(), + receipt.setup_sha256.as_str(), + receipt.session_sha256.as_str(), + )) + .or_default() + .push(receipt); + } + receipts +} + /// Every join the checker needs, with each case judged against `parse`. pub(crate) fn join<'a>( coverage: &'a SyntaxCoverage, parse: &dyn Fn(&str) -> LocalVerdict, ) -> Joined<'a> { let receipts: BTreeMap<&str, Vec<&Receipt>> = receipts_by_case(coverage); + let statements: BTreeMap<(&str, &str, &str), Vec<&Receipt>> = receipts_by_statement(coverage); let mut cases: BTreeMap<&str, CaseResult> = BTreeMap::new(); for case in coverage.cases.values().flatten() { let found: &[&Receipt] = receipts .get(case.case_id.as_str()) .map_or(&[], Vec::as_slice); + let statement: &[&Receipt] = statements + .get(&( + case.sql_sha256.as_str(), + case.setup_sha256.as_str(), + case.session_sha256.as_str(), + )) + .map_or(&[], Vec::as_slice); cases.insert( case.case_id.as_str(), - case_result(case, found, parse(&case.sql)), + case_result(case, found, statement, parse(&case.sql)), ); } Joined { diff --git a/crates/sqlquack-syntax-coverage/src/check/_helpers/receipts.rs b/crates/sqlquack-syntax-coverage/src/check/_helpers/receipts.rs index 84f269362..165c40a34 100644 --- a/crates/sqlquack-syntax-coverage/src/check/_helpers/receipts.rs +++ b/crates/sqlquack-syntax-coverage/src/check/_helpers/receipts.rs @@ -2,7 +2,8 @@ use std::collections::{BTreeMap, BTreeSet}; -use crate::check::models::{CaseResult, LocalVerdict, Reason}; +use crate::check::models::{CaseResult, LocalVerdict, Reason, SnowflakeSyntax}; +use crate::formats::constants::{SYNTAX_ERRNO, SYNTAX_ERRNO_42601}; use crate::formats::models::{AccountSetting, Case, Expect, Phase, Receipt}; /// A syntax receipt bound to exactly this case's SQL, setup, session and capture, with a query id. @@ -61,8 +62,35 @@ fn agreement(phase: Phase, local: &LocalVerdict, terminates: bool) -> Result CaseResult { +/// What Snowflake's parser did by the syntax receipts of the case's statement; none unless all agree. +fn snowflake_syntax(statement_receipts: &[&Receipt]) -> Option { + let verdicts: BTreeSet> = statement_receipts + .iter() + .filter(|receipt| receipt.measure.is_none()) + .map(|receipt| match (receipt.phase, receipt.errno) { + (Phase::SyntaxError, Some(errno)) + if errno == SYNTAX_ERRNO || errno == SYNTAX_ERRNO_42601 => + { + Some(SnowflakeSyntax::Refused) + } + (Phase::SyntaxError, _) => None, + (_, Some(errno)) if errno == SYNTAX_ERRNO => None, + _ => Some(SnowflakeSyntax::Parsed), + }) + .collect(); + match verdicts.len() { + 1 => verdicts.into_iter().next().flatten(), + _ => None, + } +} + +/// The case's result against its receipts, its statement's receipts and the parser's verdict. +pub(crate) fn case_result( + case: &Case, + receipts: &[&Receipt], + statement_receipts: &[&Receipt], + local: LocalVerdict, +) -> CaseResult { let (phase, reason, semantic_only): (Option, Option, bool) = match receipt_phase(case, receipts) { _ if case.expect == Some(Expect::Observe) => { @@ -84,6 +112,7 @@ pub(crate) fn case_result(case: &Case, receipts: &[&Receipt], local: LocalVerdic local, reason, semantic_only, + snowflake: snowflake_syntax(statement_receipts), account_state: receipts .iter() .filter(|receipt| admissible(case, receipt)) diff --git a/crates/sqlquack-syntax-coverage/src/check/models.rs b/crates/sqlquack-syntax-coverage/src/check/models.rs index 405fc97b5..7c6eb3b2e 100644 --- a/crates/sqlquack-syntax-coverage/src/check/models.rs +++ b/crates/sqlquack-syntax-coverage/src/check/models.rs @@ -76,6 +76,16 @@ pub enum Reason { BlockedByHold, } +/// What Snowflake's parser did with a case's statement, by every receipt of its SQL, setup and session. +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum SnowflakeSyntax { + /// Every receipt stopped after parsing or succeeded, with an errno other than 1003. + Parsed, + /// Every receipt is a syntax error: errno 1003 with a parse position, or errno 921. + Refused, +} + /// One case's result: its receipts against the current parser. #[derive(Clone, Debug, PartialEq, Eq, Serialize)] pub struct CaseResult { @@ -89,6 +99,8 @@ pub struct CaseResult { pub reason: Option, /// Agrees, while Snowflake stopped after parsing (semantic, runtime or unsupported phase). pub semantic_only: bool, + /// What Snowflake's parser did, whatever the case expects; none on disagreeing or no receipts. + pub snowflake: Option, /// The account parameters its admissible receipts ran under (decision D120). #[serde(skip_serializing_if = "BTreeMap::is_empty")] pub account_state: BTreeMap, diff --git a/crates/sqlquack-syntax-coverage/src/check/tests/helpers.rs b/crates/sqlquack-syntax-coverage/src/check/tests/helpers.rs index e5b8b9547..2c6bff14e 100644 --- a/crates/sqlquack-syntax-coverage/src/check/tests/helpers.rs +++ b/crates/sqlquack-syntax-coverage/src/check/tests/helpers.rs @@ -66,6 +66,11 @@ pub(crate) fn reshaped(sql: &str) -> LocalVerdict { vec![structural(sql), accepted_as("another tree")].swap_remove(usize::from(sql == "SELECT 17")) } +/// `structural`, except that `SELECT 17` is rejected. +pub(crate) fn rejecting_select_17(sql: &str) -> LocalVerdict { + vec![structural(sql), LocalVerdict::Rejected].swap_remove(usize::from(sql == "SELECT 17")) +} + /// `structural`, except that `SELECT FROM` crashes instead of being rejected. pub(crate) fn crashing_on_select_from(sql: &str) -> LocalVerdict { vec![structural(sql), crashing(sql)].swap_remove(usize::from(sql == "SELECT FROM")) diff --git a/crates/sqlquack-syntax-coverage/src/cli/_helpers/commands.rs b/crates/sqlquack-syntax-coverage/src/cli/_helpers/commands.rs index 806de8ec8..d03334744 100644 --- a/crates/sqlquack-syntax-coverage/src/cli/_helpers/commands.rs +++ b/crates/sqlquack-syntax-coverage/src/cli/_helpers/commands.rs @@ -339,13 +339,17 @@ fn read_snapshot(path: &Path) -> Result { /// Lines naming each non-empty list of the diff. fn diff_lines(diff: &SnapshotDiff) -> Vec { - let lists: [(&str, &Vec); 8] = [ + let lists: [(&str, &Vec); 9] = [ ("REGRESSION forms lost", &diff.forms_lost), ("REGRESSION obligations lost", &diff.obligations_lost), ( "REGRESSION Snowflake-parsed cases the parser now rejects", &diff.parsed_cases_refused, ), + ( + "REGRESSION Snowflake syntax errors the parser now accepts", + &diff.syntax_error_cases_accepted, + ), ("CATALOGUE files changed", &diff.catalogue_files_changed), ("CATALOGUE forms added", &diff.forms_added), ("CATALOGUE forms removed", &diff.forms_removed), @@ -382,6 +386,7 @@ pub(crate) fn diff( &diff.forms_lost, &diff.obligations_lost, &diff.parsed_cases_refused, + &diff.syntax_error_cases_accepted, &diff.catalogue_files_changed, &diff.forms_added, &diff.forms_removed, diff --git a/crates/sqlquack-syntax-coverage/src/formats/constants.rs b/crates/sqlquack-syntax-coverage/src/formats/constants.rs index a8b13e9a0..64d269e26 100644 --- a/crates/sqlquack-syntax-coverage/src/formats/constants.rs +++ b/crates/sqlquack-syntax-coverage/src/formats/constants.rs @@ -88,3 +88,9 @@ pub const DCM_LINE: &str = "line "; /// What separates an inner error's line from its position. pub const DCM_POSITION: &str = " at position "; + +/// Snowflake's errno for a syntax error; with a parse position it is a refusal by the parser. +pub const SYNTAX_ERRNO: i64 = 1003; + +/// Snowflake's errno 921, SQLSTATE 42601, reviewed as a syntax refusal. +pub const SYNTAX_ERRNO_42601: i64 = 921; diff --git a/crates/sqlquack-syntax-coverage/src/report/main/diff_snapshots.rs b/crates/sqlquack-syntax-coverage/src/report/main/diff_snapshots.rs index 0f6b9d04d..8b5035512 100644 --- a/crates/sqlquack-syntax-coverage/src/report/main/diff_snapshots.rs +++ b/crates/sqlquack-syntax-coverage/src/report/main/diff_snapshots.rs @@ -59,5 +59,10 @@ pub fn diff_snapshots(base: &Snapshot, head: &Snapshot) -> SnapshotDiff { .intersection(&head.refused_cases) .cloned() .collect(), + syntax_error_cases_accepted: base + .syntax_error_cases_rejected + .intersection(&head.syntax_error_cases_accepted) + .cloned() + .collect(), } } diff --git a/crates/sqlquack-syntax-coverage/src/report/main/snapshot.rs b/crates/sqlquack-syntax-coverage/src/report/main/snapshot.rs index da9fc384c..9eefea5da 100644 --- a/crates/sqlquack-syntax-coverage/src/report/main/snapshot.rs +++ b/crates/sqlquack-syntax-coverage/src/report/main/snapshot.rs @@ -2,9 +2,8 @@ use std::collections::{BTreeMap, BTreeSet}; -use crate::check::models::{CheckResult, LocalVerdict}; +use crate::check::models::{CheckResult, LocalVerdict, SnowflakeSyntax}; use crate::formats::constants::{ALIASES_FILE, CATALOGUE_DIR, SNAPSHOT_FORMAT, SOURCES_FILE}; -use crate::formats::models::Phase; use crate::report::models::Snapshot; /// Whether a coverage path belongs to the catalogue rather than to obligations or evidence. @@ -12,15 +11,19 @@ fn is_catalogue_file(path: &str) -> bool { path == SOURCES_FILE || path == ALIASES_FILE || path.starts_with(&format!("{CATALOGUE_DIR}/")) } -/// The receipted cases Snowflake parsed whose local verdict `accepted` says whether it parsed too. -fn snowflake_parsed(check: &CheckResult, accepted: bool) -> BTreeSet { +/// Cases whose receipts say `snowflake` and whose parser verdict is acceptance when `accepted`. +fn by_snowflake( + check: &CheckResult, + snowflake: SnowflakeSyntax, + accepted: bool, +) -> BTreeSet { check .forms .iter() .flat_map(|form| &form.obligations) .flat_map(|obligation| &obligation.cases) .filter(|case| { - case.phase.is_some_and(|phase| phase != Phase::SyntaxError) + case.snowflake == Some(snowflake) && matches!(case.local, LocalVerdict::Accepted { .. }) == accepted && !matches!(case.local, LocalVerdict::Failed { .. }) }) @@ -55,7 +58,9 @@ pub fn snapshot( .filter(|obligation| obligation.required && obligation.reason.is_none()) .map(|obligation| obligation.id.clone()) .collect(), - parsed_cases: snowflake_parsed(check, true), - refused_cases: snowflake_parsed(check, false), + parsed_cases: by_snowflake(check, SnowflakeSyntax::Parsed, true), + refused_cases: by_snowflake(check, SnowflakeSyntax::Parsed, false), + syntax_error_cases_rejected: by_snowflake(check, SnowflakeSyntax::Refused, false), + syntax_error_cases_accepted: by_snowflake(check, SnowflakeSyntax::Refused, true), } } diff --git a/crates/sqlquack-syntax-coverage/src/report/models.rs b/crates/sqlquack-syntax-coverage/src/report/models.rs index 4aea498f4..da7fbed1a 100644 --- a/crates/sqlquack-syntax-coverage/src/report/models.rs +++ b/crates/sqlquack-syntax-coverage/src/report/models.rs @@ -64,6 +64,12 @@ pub struct Snapshot { /// Receipted cases Snowflake parsed that the parser rejects. #[serde(default)] pub refused_cases: BTreeSet, + /// Receipted cases Snowflake refused as syntax that the parser rejects too. + #[serde(default)] + pub syntax_error_cases_rejected: BTreeSet, + /// Receipted cases Snowflake refused as syntax that the parser accepts, opaque or not. + #[serde(default)] + pub syntax_error_cases_accepted: BTreeSet, } /// One snapshot against a later one; coverage and catalogue changes kept apart. @@ -82,4 +88,6 @@ pub struct SnapshotDiff { pub obligations_gained: Vec, /// Cases Snowflake parsed that the base parser accepted, opaque or not, and the head rejects. pub parsed_cases_refused: Vec, + /// Cases Snowflake refused as syntax that the base parser rejected and the head accepts. + pub syntax_error_cases_accepted: Vec, } diff --git a/crates/sqlquack-syntax-coverage/src/report/tests.rs b/crates/sqlquack-syntax-coverage/src/report/tests.rs index 76c33936a..53dc6cf3b 100644 --- a/crates/sqlquack-syntax-coverage/src/report/tests.rs +++ b/crates/sqlquack-syntax-coverage/src/report/tests.rs @@ -2,5 +2,7 @@ mod helpers; #[path = "tests/test_diff_snapshots.rs"] mod test_diff_snapshots; +#[path = "tests/test_parser_changes.rs"] +mod test_parser_changes; #[path = "tests/test_types.rs"] mod test_types; diff --git a/crates/sqlquack-syntax-coverage/src/report/tests/helpers.rs b/crates/sqlquack-syntax-coverage/src/report/tests/helpers.rs index 4758a98dd..f51b61131 100644 --- a/crates/sqlquack-syntax-coverage/src/report/tests/helpers.rs +++ b/crates/sqlquack-syntax-coverage/src/report/tests/helpers.rs @@ -1,6 +1,14 @@ use std::collections::{BTreeMap, BTreeSet}; +use std::path::Path; + +use crate::check::main::check_coverage::check_coverage; +use crate::check::models::{FormState, LocalVerdict}; +use crate::check::tests::helpers::{ + TREE, case_line, replace, rewrite_select_17, select_17, with_fields, +}; +use crate::formats::main::load_coverage::load_coverage; +use crate::report::main::snapshot::snapshot; -use crate::check::models::FormState; use crate::formats::constants::SNAPSHOT_FORMAT; use crate::report::models::Snapshot; @@ -23,5 +31,48 @@ pub(crate) fn base() -> Snapshot { satisfied_obligations: BTreeSet::from(["sf.query.select.o001".to_owned()]), parsed_cases: BTreeSet::from(["sf.query.select.k1".to_owned()]), refused_cases: BTreeSet::from(["sf.query.with.k2".to_owned()]), + syntax_error_cases_rejected: BTreeSet::from(["sf.query.select.k3".to_owned()]), + syntax_error_cases_accepted: BTreeSet::new(), } } + +/// Makes `select-17` an observe case whose receipt records `phase` and `errno`. +pub(crate) fn observed_select_17(root: &Path, phase: &str, errno: i64) { + rewrite_select_17( + root, + &with_fields(&select_17(), serde_json::json!({"expect": "observe"})), + ); + replace( + root, + "receipts/run-1.jsonl", + "\"phase\":\"accepted\"", + &format!("\"errno\":{errno},\"phase\":\"{phase}\""), + ); +} + +/// Makes `select-from` an observe case whose receipt is a syntax error with `errno`. +pub(crate) fn observed_select_from(root: &Path, errno: i64) { + let case: String = case_line("select-from", "sf.query.select.o002", "SELECT FROM", &[]); + replace( + root, + "cases/query.jsonl", + &case, + &with_fields(&case, serde_json::json!({"expect": "observe"})), + ); + replace( + root, + "receipts/run-1.jsonl", + "\"phase\":\"syntax_error\"", + &format!("\"errno\":{errno},\"phase\":\"syntax_error\""), + ); +} + +/// The snapshot of the tree at `root` under `parse`. +pub(crate) fn snapshot_with(root: &Path, parse: fn(&str) -> LocalVerdict) -> Snapshot { + let coverage = load_coverage(root).expect("load"); + snapshot( + &check_coverage(&coverage, Some(TREE), &parse), + &coverage.file_hashes, + "catalogue", + ) +} diff --git a/crates/sqlquack-syntax-coverage/src/report/tests/test_parser_changes.rs b/crates/sqlquack-syntax-coverage/src/report/tests/test_parser_changes.rs new file mode 100644 index 000000000..7380f43e2 --- /dev/null +++ b/crates/sqlquack-syntax-coverage/src/report/tests/test_parser_changes.rs @@ -0,0 +1,116 @@ +use crate::check::tests::helpers::{ + append_receipt, opaque, receipt_line, rejecting_select_17, select_17, structural, + verified_tree, with_fields, +}; +use crate::report::main::diff_snapshots::diff_snapshots; +use crate::report::models::SnapshotDiff; +use crate::report::tests::helpers::{observed_select_17, observed_select_from, snapshot_with}; +use crate::report::tests::test_types::ParserChangeTestCase; + +/// The shapes of the four regressions #55 brought on observe rows (audit 38bed222), and controls. +#[test] +fn given_receipted_cases_of_any_expectation_when_the_parser_changes_then_the_diff_flags_contradictions() + { + let test_cases = [ + ParserChangeTestCase { + description: "an observe case Snowflake accepted, now rejected (create-database-role)", + mutate: |root| observed_select_17(root, "accepted", 0), + base: structural, + head: rejecting_select_17, + expected_parsed_cases_refused: vec!["select-17"], + expected_syntax_error_cases_accepted: Vec::new(), + }, + ParserChangeTestCase { + description: "an observe case that failed semantic analysis, now rejected (database-role-clone)", + mutate: |root| observed_select_17(root, "semantic_error", 2003), + base: structural, + head: rejecting_select_17, + expected_parsed_cases_refused: vec!["select-17"], + expected_syntax_error_cases_accepted: Vec::new(), + }, + ParserChangeTestCase { + description: "an observe case Snowflake parsed, opaque before and rejected now (create-stream-clone)", + mutate: |root| observed_select_17(root, "semantic_error", 1422), + base: opaque, + head: rejecting_select_17, + expected_parsed_cases_refused: vec!["select-17"], + expected_syntax_error_cases_accepted: Vec::new(), + }, + ParserChangeTestCase { + description: "an observe case refused with a positioned 1003, now accepted (create-database)", + mutate: |root| observed_select_from(root, 1003), + base: structural, + head: opaque, + expected_parsed_cases_refused: Vec::new(), + expected_syntax_error_cases_accepted: vec!["select-from"], + }, + ParserChangeTestCase { + description: "an observe case refused with 921, now accepted", + mutate: |root| observed_select_from(root, 921), + base: structural, + head: opaque, + expected_parsed_cases_refused: Vec::new(), + expected_syntax_error_cases_accepted: vec!["select-from"], + }, + ParserChangeTestCase { + description: "a positionless 1003 is no parse, so its rejection is not flagged", + mutate: |root| observed_select_17(root, "semantic_error", 1003), + base: structural, + head: rejecting_select_17, + expected_parsed_cases_refused: Vec::new(), + expected_syntax_error_cases_accepted: Vec::new(), + }, + ParserChangeTestCase { + description: "receipts that disagree on the statement are not used", + mutate: |root| { + observed_select_17(root, "accepted", 0); + let receipt: String = receipt_line(&select_17(), "syntax_error"); + append_receipt( + root, + &with_fields( + &receipt, + serde_json::json!({"errno": 1003, "query_id": "query-again", "run_id": "run-2"}), + ), + ); + }, + base: structural, + head: rejecting_select_17, + expected_parsed_cases_refused: Vec::new(), + expected_syntax_error_cases_accepted: Vec::new(), + }, + ParserChangeTestCase { + description: "an unchanged parser flags nothing", + mutate: |root| observed_select_17(root, "accepted", 0), + base: structural, + head: structural, + expected_parsed_cases_refused: Vec::new(), + expected_syntax_error_cases_accepted: Vec::new(), + }, + ]; + for test_case in test_cases { + let directory = tempfile::tempdir().expect("temporary directory"); + verified_tree(directory.path()); + (test_case.mutate)(directory.path()); + let diff: SnapshotDiff = diff_snapshots( + &snapshot_with(directory.path(), test_case.base), + &snapshot_with(directory.path(), test_case.head), + ); + assert_eq!( + (diff.parsed_cases_refused, diff.syntax_error_cases_accepted), + ( + test_case + .expected_parsed_cases_refused + .iter() + .map(|id| (*id).to_owned()) + .collect::>(), + test_case + .expected_syntax_error_cases_accepted + .iter() + .map(|id| (*id).to_owned()) + .collect::>(), + ), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlquack-syntax-coverage/src/report/tests/test_types.rs b/crates/sqlquack-syntax-coverage/src/report/tests/test_types.rs index c92fb7fbb..f11eeb416 100644 --- a/crates/sqlquack-syntax-coverage/src/report/tests/test_types.rs +++ b/crates/sqlquack-syntax-coverage/src/report/tests/test_types.rs @@ -1,3 +1,6 @@ +use std::path::Path; + +use crate::check::models::LocalVerdict; use crate::report::models::{Snapshot, SnapshotDiff}; /// A change from the base snapshot, and the diff it must produce. @@ -6,3 +9,13 @@ pub(crate) struct DiffTestCase { pub(crate) head: fn(Snapshot) -> Snapshot, pub(crate) expected_diff: SnapshotDiff, } + +/// Receipts and expectations set by `mutate`, a base and a head parser, and what the diff must flag. +pub(crate) struct ParserChangeTestCase { + pub(crate) description: &'static str, + pub(crate) mutate: fn(&Path), + pub(crate) base: fn(&str) -> LocalVerdict, + pub(crate) head: fn(&str) -> LocalVerdict, + pub(crate) expected_parsed_cases_refused: Vec<&'static str>, + pub(crate) expected_syntax_error_cases_accepted: Vec<&'static str>, +}