From cd4a2186e895885a7dfff56953634cda4395130e Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Thu, 5 Nov 2026 15:20:10 +0000 Subject: [PATCH 1/3] coverage: judge every receipt against the parser and fail on new contradictions (D141) The snapshot's case lists only reach receipts through catalogue forms, so a receipt on a retired or uncited case id never counted. The check now judges every current receipt by its statement (SQL, setup and session), with SQL from the citing case or from the hash-checked inputs of the run reports, and records receipt_contradictions and unjudged_receipts in the snapshot. diff fails on a contradiction the base did not have; documented (D113) ones go on their own non-failing line (D135), and existing ones are listed without failing. On catalogue 8c9639e7 and evidence a82d976b, 4203044e -> cbe394ec reports 53 new contradictions, including all 27 the D118 audit of #68 found, plus the 68 documented flips on their own line. --- .../src/check/_helpers/contradictions.rs | 64 +++++++++++++++ .../src/check/_helpers/mod.rs | 1 + .../src/check/_helpers/receipts.rs | 2 +- .../src/check/main/check_coverage.rs | 5 ++ .../src/check/models.rs | 4 + .../src/check/tests/helpers.rs | 14 ++++ .../src/check/tests/test_check_coverage.rs | 78 ++++++++++++++++++- .../src/check/tests/test_types.rs | 9 +++ .../src/cli/_helpers/commands.rs | 32 +++++++- .../src/formats/_helpers/retired.rs | 26 +++++-- .../src/formats/_helpers/sections.rs | 14 ++-- .../src/formats/constants.rs | 2 + .../src/formats/main/load_coverage.rs | 1 + .../src/formats/models.rs | 2 + .../src/formats/types.rs | 5 +- .../src/report/main/diff_snapshots.rs | 26 +++++++ .../src/report/main/snapshot.rs | 2 + .../src/report/models.rs | 14 ++++ .../src/report/tests/helpers.rs | 2 + .../src/report/tests/test_diff_snapshots.rs | 76 +++++++++++++++++- .../src/report/tests/test_types.rs | 8 ++ docs/syntax-coverage.md | 18 +++++ 22 files changed, 384 insertions(+), 21 deletions(-) create mode 100644 crates/sqlquack-syntax-coverage/src/check/_helpers/contradictions.rs diff --git a/crates/sqlquack-syntax-coverage/src/check/_helpers/contradictions.rs b/crates/sqlquack-syntax-coverage/src/check/_helpers/contradictions.rs new file mode 100644 index 000000000..8eaed4e33 --- /dev/null +++ b/crates/sqlquack-syntax-coverage/src/check/_helpers/contradictions.rs @@ -0,0 +1,64 @@ +//! Every current receipt against the parser, whether or not a catalogued case cites it (decision D141). + +use std::collections::{BTreeMap, BTreeSet}; + +use crate::check::_helpers::receipts::snowflake_syntax; +use crate::check::models::{CaseResult, LocalVerdict, SnowflakeSyntax}; +use crate::formats::models::{Receipt, SyntaxCoverage}; + +/// Whether the parser's verdict contradicts what Snowflake's parser did with the same statement. +fn contradicts(snowflake: SnowflakeSyntax, local: &LocalVerdict) -> bool { + match snowflake { + SnowflakeSyntax::Parsed => !matches!(local, LocalVerdict::Accepted { .. }), + SnowflakeSyntax::Refused => matches!(local, LocalVerdict::Accepted { .. }), + } +} + +/// The receipt case ids the parser contradicts by SQL, setup and session, and those without SQL. +pub(crate) fn receipt_contradictions( + coverage: &SyntaxCoverage, + cases: &BTreeMap<&str, CaseResult>, + parse: &dyn Fn(&str) -> LocalVerdict, +) -> (BTreeSet, BTreeSet) { + let mut statements: BTreeMap<(&str, &str, &str), Vec<&Receipt>> = BTreeMap::new(); + for receipt in &coverage.receipts { + statements + .entry(( + receipt.sql_sha256.as_str(), + receipt.setup_sha256.as_str(), + receipt.session_sha256.as_str(), + )) + .or_default() + .push(receipt); + } + let mut judged: BTreeMap<&str, LocalVerdict> = cases + .values() + .map(|case| (case.sql_sha256.as_str(), case.local.clone())) + .collect(); + let case_sql: BTreeMap<&str, &str> = coverage + .cases + .values() + .flatten() + .map(|case| (case.sql_sha256.as_str(), case.sql.as_str())) + .collect(); + let mut contradicted: BTreeSet = BTreeSet::new(); + let mut unjudged: BTreeSet = BTreeSet::new(); + for ((sql_sha256, _, _), receipts) in &statements { + let Some(snowflake) = snowflake_syntax(receipts) else { + continue; + }; + let sql: Option<&str> = case_sql + .get(sql_sha256) + .copied() + .or_else(|| coverage.receipt_sql.get(*sql_sha256).map(String::as_str)); + let Some(sql) = sql else { + unjudged.extend(receipts.iter().map(|receipt| receipt.case_id.clone())); + continue; + }; + let local: &LocalVerdict = judged.entry(sql_sha256).or_insert_with(|| parse(sql)); + if contradicts(snowflake, local) { + contradicted.extend(receipts.iter().map(|receipt| receipt.case_id.clone())); + } + } + (contradicted, unjudged) +} diff --git a/crates/sqlquack-syntax-coverage/src/check/_helpers/mod.rs b/crates/sqlquack-syntax-coverage/src/check/_helpers/mod.rs index d774bbdd0..e800470e9 100644 --- a/crates/sqlquack-syntax-coverage/src/check/_helpers/mod.rs +++ b/crates/sqlquack-syntax-coverage/src/check/_helpers/mod.rs @@ -1,3 +1,4 @@ +pub(crate) mod contradictions; pub(crate) mod fingerprints; pub(crate) mod forms; pub(crate) mod join; diff --git a/crates/sqlquack-syntax-coverage/src/check/_helpers/receipts.rs b/crates/sqlquack-syntax-coverage/src/check/_helpers/receipts.rs index 165c40a34..1ece2b1cf 100644 --- a/crates/sqlquack-syntax-coverage/src/check/_helpers/receipts.rs +++ b/crates/sqlquack-syntax-coverage/src/check/_helpers/receipts.rs @@ -63,7 +63,7 @@ fn agreement(phase: Phase, local: &LocalVerdict, terminates: bool) -> Result Option { +pub(crate) fn snowflake_syntax(statement_receipts: &[&Receipt]) -> Option { let verdicts: BTreeSet> = statement_receipts .iter() .filter(|receipt| receipt.measure.is_none()) diff --git a/crates/sqlquack-syntax-coverage/src/check/main/check_coverage.rs b/crates/sqlquack-syntax-coverage/src/check/main/check_coverage.rs index c0779d0e1..05fee1098 100644 --- a/crates/sqlquack-syntax-coverage/src/check/main/check_coverage.rs +++ b/crates/sqlquack-syntax-coverage/src/check/main/check_coverage.rs @@ -2,6 +2,7 @@ use std::collections::BTreeSet; +use crate::check::_helpers::contradictions::receipt_contradictions; use crate::check::_helpers::forms::form_result; use crate::check::_helpers::join::join; use crate::check::_helpers::references::reference_problems; @@ -31,10 +32,14 @@ pub fn check_coverage( forms.push(form_result(form, family, &context)); } } + let (contradicted, unjudged): (BTreeSet, BTreeSet) = + receipt_contradictions(coverage, &joined.cases, parse); CheckResult { parser_tree: parser_tree.map(str::to_owned), forms, unexcluded_pages: unexcluded_pages(&coverage.sources, &excluded_sources), problems: reference_problems(coverage, &joined.forms), + receipt_contradictions: contradicted, + unjudged_receipts: unjudged, } } diff --git a/crates/sqlquack-syntax-coverage/src/check/models.rs b/crates/sqlquack-syntax-coverage/src/check/models.rs index 7c6eb3b2e..9f944763e 100644 --- a/crates/sqlquack-syntax-coverage/src/check/models.rs +++ b/crates/sqlquack-syntax-coverage/src/check/models.rs @@ -162,6 +162,10 @@ pub struct CheckResult { pub unexcluded_pages: Vec, /// References between files that do not resolve; any one fails the check. pub problems: Vec, + /// Receipt case ids whose statement the parser contradicts, cited by a form or not (D141). + pub receipt_contradictions: BTreeSet, + /// Receipt case ids whose SQL no case or run report gives, so no verdict can be compared. + pub unjudged_receipts: BTreeSet, } /// The files joined for checking, with every case already judged. diff --git a/crates/sqlquack-syntax-coverage/src/check/tests/helpers.rs b/crates/sqlquack-syntax-coverage/src/check/tests/helpers.rs index 2c6bff14e..c841cb1b1 100644 --- a/crates/sqlquack-syntax-coverage/src/check/tests/helpers.rs +++ b/crates/sqlquack-syntax-coverage/src/check/tests/helpers.rs @@ -433,3 +433,17 @@ pub(crate) fn move_form(root: &Path, id: &str) { &format!("{declared}anchor = \"Syntax, edited\""), ); } + +/// Adds a receipt recording `fields` for `retired-1`, a case id no form cites, of `sql`. +pub(crate) fn retired_receipt(root: &Path, sql: &str, fields: Value) { + let case: String = case_line("retired-1", "sf.query.select.o001", sql, &[]); + append_receipt(root, &with_fields(&receipt_line(&case, "accepted"), fields)); +} + +/// Writes the run report beside the receipts, listing `sql` among its inputs. +pub(crate) fn report_input(root: &Path, sql: &str) { + let report: Value = serde_json::json!({ + "inputs": [{"sql": sql, "sql_sha256": sha256(sql)}], + }); + write(root, "receipts/run-1.report.json", &report.to_string()); +} diff --git a/crates/sqlquack-syntax-coverage/src/check/tests/test_check_coverage.rs b/crates/sqlquack-syntax-coverage/src/check/tests/test_check_coverage.rs index 7d835c496..b75a3b1db 100644 --- a/crates/sqlquack-syntax-coverage/src/check/tests/test_check_coverage.rs +++ b/crates/sqlquack-syntax-coverage/src/check/tests/test_check_coverage.rs @@ -4,10 +4,12 @@ use crate::check::tests::helpers::{ OTHER_TREE, SETUP, TREE, append_receipt, case_in_zone, case_line, crashing, crashing_on_select_from, drop_lines, exclusion_review, form, hanging, move_form, not_submitted, note_obligation, obligation, opaque, pin_definitions, pin_fingerprints, read, receipt_line, - rejecting, replace, reshaped, rewrite_select_17, select_17, select_17_setup_sha256, sha256, - structural, unobservable, verified_tree, with_fields, write, + rejecting, replace, report_input, reshaped, retired_receipt, rewrite_select_17, select_17, + select_17_setup_sha256, sha256, structural, unobservable, verified_tree, with_fields, write, +}; +use crate::check::tests::test_types::{ + AccountStateTestCase, CheckTestCase, Parameters, ReceiptCheckTestCase, }; -use crate::check::tests::test_types::{AccountStateTestCase, CheckTestCase, Parameters}; use crate::formats::main::load_coverage::load_coverage; #[test] @@ -1169,3 +1171,73 @@ fn given_a_receipt_with_account_state_when_checking_then_only_its_case_carries_i ); } } + +#[test] +fn given_receipts_no_form_cites_when_checking_then_each_is_judged_against_the_parser() { + let test_cases = [ + ReceiptCheckTestCase { + description: "a retired case Snowflake parsed, which the parser now rejects", + mutate: |root| { + retired_receipt(root, "SELECT 2", serde_json::json!({})); + report_input(root, "SELECT 2"); + }, + parse: rejecting, + expected_contradictions: vec!["retired-1"], + expected_unjudged: Vec::new(), + }, + ReceiptCheckTestCase { + description: "a retired case Snowflake parsed, which the parser accepts", + mutate: |root| { + retired_receipt(root, "SELECT 2", serde_json::json!({})); + report_input(root, "SELECT 2"); + }, + parse: structural, + expected_contradictions: Vec::new(), + expected_unjudged: Vec::new(), + }, + ReceiptCheckTestCase { + description: "a retired syntax error with errno 1003, which the parser accepts", + mutate: |root| { + retired_receipt( + root, + "SELECT 2", + serde_json::json!({"phase": "syntax_error", "outcome": "error", "errno": 1003}), + ); + report_input(root, "SELECT 2"); + }, + parse: structural, + expected_contradictions: vec!["retired-1"], + expected_unjudged: Vec::new(), + }, + ReceiptCheckTestCase { + description: "a retired case whose SQL no case or report gives", + mutate: |root| retired_receipt(root, "SELECT 2", serde_json::json!({})), + parse: rejecting, + expected_contradictions: Vec::new(), + expected_unjudged: vec!["retired-1"], + }, + ]; + for test_case in test_cases { + let directory = tempfile::tempdir().expect("tempdir"); + verified_tree(directory.path()); + (test_case.mutate)(directory.path()); + let coverage = load_coverage(directory.path()).expect("loads"); + let check = check_coverage(&coverage, Some(TREE), &test_case.parse); + let contradicted: Vec<&str> = check + .receipt_contradictions + .iter() + .map(String::as_str) + .filter(|id| *id == "retired-1") + .collect(); + let unjudged: Vec<&str> = check.unjudged_receipts.iter().map(String::as_str).collect(); + assert_eq!( + (contradicted, unjudged), + ( + test_case.expected_contradictions, + test_case.expected_unjudged + ), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlquack-syntax-coverage/src/check/tests/test_types.rs b/crates/sqlquack-syntax-coverage/src/check/tests/test_types.rs index 012001a06..18f28edcf 100644 --- a/crates/sqlquack-syntax-coverage/src/check/tests/test_types.rs +++ b/crates/sqlquack-syntax-coverage/src/check/tests/test_types.rs @@ -29,3 +29,12 @@ pub(crate) struct AccountStateTestCase { /// Each case id with its parameter names and values, in case-id order. pub(crate) expected_cases: Vec<(&'static str, Parameters<'static>)>, } + +/// Receipts on a case id no form cites, and the receipt verdicts the checker must then give (D141). +pub(crate) struct ReceiptCheckTestCase { + pub(crate) description: &'static str, + pub(crate) mutate: fn(&Path), + pub(crate) parse: fn(&str) -> LocalVerdict, + pub(crate) expected_contradictions: Vec<&'static str>, + pub(crate) expected_unjudged: Vec<&'static str>, +} diff --git a/crates/sqlquack-syntax-coverage/src/cli/_helpers/commands.rs b/crates/sqlquack-syntax-coverage/src/cli/_helpers/commands.rs index 408efc4b5..20d09709c 100644 --- a/crates/sqlquack-syntax-coverage/src/cli/_helpers/commands.rs +++ b/crates/sqlquack-syntax-coverage/src/cli/_helpers/commands.rs @@ -339,8 +339,16 @@ 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); 10] = [ + let lists: [(&str, &Vec); 12] = [ ("REGRESSION forms lost", &diff.forms_lost), + ( + "REGRESSION receipts the parser now contradicts, cited by a form or not (D141)", + &diff.receipts_contradicted, + ), + ( + "DOCUMENTED (D113) receipts the parser now contradicts, for the auditor", + &diff.documented_receipts_contradicted, + ), ("REGRESSION obligations lost", &diff.obligations_lost), ( "REGRESSION Snowflake-parsed cases the parser now rejects", @@ -360,11 +368,28 @@ fn diff_lines(diff: &SnapshotDiff) -> Vec { ("forms gained", &diff.forms_gained), ("obligations gained", &diff.obligations_gained), ]; - lists + let mut lines: Vec = lists .iter() .filter(|(_, ids)| !ids.is_empty()) .map(|(label, ids)| format!("{label} ({}): {}", ids.len(), ids.join(" "))) - .collect() + .collect(); + let counted: [(&str, usize); 2] = [ + ( + "receipts the parser already contradicted, not failing", + diff.receipts_still_contradicted.len(), + ), + ( + "receipts without SQL to judge, not failing", + diff.unjudged_receipts.len(), + ), + ]; + lines.extend( + counted + .iter() + .filter(|(_, count)| *count > 0) + .map(|(label, count)| format!("{label}: {count} (listed in --json)")), + ); + lines } /// BASE against HEAD, or against the current tree; fails on a regression or catalogue change. @@ -389,6 +414,7 @@ pub(crate) fn diff( let failing: bool = [ &diff.forms_lost, &diff.obligations_lost, + &diff.receipts_contradicted, &diff.parsed_cases_refused, &diff.syntax_error_cases_accepted, &diff.catalogue_files_changed, diff --git a/crates/sqlquack-syntax-coverage/src/formats/_helpers/retired.rs b/crates/sqlquack-syntax-coverage/src/formats/_helpers/retired.rs index 6ba6c7c3c..5734c2425 100644 --- a/crates/sqlquack-syntax-coverage/src/formats/_helpers/retired.rs +++ b/crates/sqlquack-syntax-coverage/src/formats/_helpers/retired.rs @@ -6,13 +6,16 @@ use std::path::{Path, PathBuf}; use serde_json::Value; use crate::formats::_helpers::files::read_text; -use crate::formats::_helpers::hashes::is_sha256; -use crate::formats::constants::{REPORT_RECEIPT_SHA256, REPORT_WITHDRAWN}; +use crate::formats::_helpers::hashes::{is_sha256, sha256_hex}; +use crate::formats::constants::{REPORT_INPUTS, REPORT_RECEIPT_SHA256, REPORT_WITHDRAWN}; use crate::formats::models::Receipt; -/// The receipt line hashes the run reports withdraw, and every problem reading them. -pub(crate) fn withdrawn_receipts(reports: &[PathBuf]) -> (BTreeSet, Vec) { +/// The receipt line hashes the run reports withdraw, the SQL of their inputs by hash, and every problem. +pub(crate) fn read_reports( + reports: &[PathBuf], +) -> (BTreeSet, BTreeMap, Vec) { let mut withdrawn: BTreeSet = BTreeSet::new(); + let mut statements: BTreeMap = BTreeMap::new(); let mut problems: Vec = Vec::new(); for path in reports { let report: Value = match read_text(path).and_then(|(text, _)| { @@ -38,8 +41,21 @@ pub(crate) fn withdrawn_receipts(reports: &[PathBuf]) -> (BTreeSet, Vec< None => problems.push(withdrawn_problem(path)), } } + let inputs: &[Value] = report[REPORT_INPUTS].as_array().map_or(&[], Vec::as_slice); + for input in inputs { + match (input["sql"].as_str(), input["sql_sha256"].as_str()) { + (Some(sql), Some(hash)) if sha256_hex(sql.as_bytes()) == hash => { + statements.insert(hash.to_owned(), sql.to_owned()); + } + (Some(_), Some(hash)) => problems.push(format!( + "{}: input {hash} is not the SHA-256 of its sql", + path.display() + )), + _ => {} + } + } } - (withdrawn, problems) + (withdrawn, statements, problems) } fn withdrawn_problem(path: &Path) -> String { diff --git a/crates/sqlquack-syntax-coverage/src/formats/_helpers/sections.rs b/crates/sqlquack-syntax-coverage/src/formats/_helpers/sections.rs index 6a622233f..17c1d2a27 100644 --- a/crates/sqlquack-syntax-coverage/src/formats/_helpers/sections.rs +++ b/crates/sqlquack-syntax-coverage/src/formats/_helpers/sections.rs @@ -11,7 +11,7 @@ use crate::formats::_helpers::records::{ check_obligation, check_reading, check_receipt, check_restatements, check_review, check_source, check_unobservable, }; -use crate::formats::_helpers::retired::{retire, withdrawn_receipts}; +use crate::formats::_helpers::retired::{read_reports, retire}; use crate::formats::constants::{ ALIASES_FILE, CASES_DIR, CATALOGUE_DIR, INDEXES_FILE, LIMITS_FILE, MARKS, MARKS_DIR, OBLIGATIONS_DIR, RECEIPT_REPORT_SUFFIX, RECEIPTS_DIR, REVIEWS_DIR, SOURCES_FILE, @@ -22,7 +22,7 @@ use crate::formats::models::{ Obligation, ObligationsFile, Reading, Receipt, Review, ReviewsFile, Source, SourcesFile, Unobservable, UnobservableFile, }; -use crate::formats::types::FamilyCatalogue; +use crate::formats::types::{FamilyCatalogue, LoadedReceipts}; /// What one section of the tree yields. pub(crate) struct Section { @@ -206,8 +206,8 @@ pub(crate) fn load_marks(root: &Path) -> Section> { Section::of(records, declared, problems) } -/// Every receipts file, less the receipts superseded or withdrawn by the run reports beside them. -pub(crate) fn load_receipts(root: &Path) -> Section<(Vec, Vec)> { +/// Every receipts file, less those superseded or withdrawn, with the SQL its run reports give by hash. +pub(crate) fn load_receipts(root: &Path) -> Section { let (paths, others) = listed(&root.join(RECEIPTS_DIR), "jsonl"); let mut problems: Vec = others .iter() @@ -226,11 +226,11 @@ pub(crate) fn load_receipts(root: &Path) -> Section<(Vec, Vec)> ); published.extend(receipts); } - let (withdrawn, report_problems) = withdrawn_receipts(&reports); + let (withdrawn, statements, report_problems) = read_reports(&reports); problems.extend(report_problems); let (kept, retired, retire_problems) = retire(published, &withdrawn); problems.extend(retire_problems); - Section::of((kept, retired), Vec::new(), problems) + Section::of((kept, retired, statements), Vec::new(), problems) } /// `unobservable/.toml`: optional, and each obligation listed at most once. @@ -316,7 +316,7 @@ pub(crate) struct Sections { pub(crate) catalogue: Section, pub(crate) obligations: Section>>, pub(crate) cases: Section>>, - pub(crate) receipts: Section<(Vec, Vec)>, + pub(crate) receipts: Section, pub(crate) reviews: Section>, pub(crate) unobservable: Section<(Vec, Vec)>, pub(crate) readings: Section>, diff --git a/crates/sqlquack-syntax-coverage/src/formats/constants.rs b/crates/sqlquack-syntax-coverage/src/formats/constants.rs index 0ddf80c2c..9113fa9be 100644 --- a/crates/sqlquack-syntax-coverage/src/formats/constants.rs +++ b/crates/sqlquack-syntax-coverage/src/formats/constants.rs @@ -72,6 +72,8 @@ pub const SNAPSHOT_FORMAT: &str = "sqlquack-syntax-coverage-snapshot/1"; pub const BASELINE_FILE: &str = "baseline.json"; /// The run-report list of receipt lines the importer withdrew, each by `receipt_sha256`. pub const REPORT_WITHDRAWN: &str = "withdrawn"; +/// The run-report list of statements the importer read, each with `sql` and `sql_sha256`. +pub const REPORT_INPUTS: &str = "inputs"; /// The field of a withdrawn entry naming the receipt line's SHA-256. pub const REPORT_RECEIPT_SHA256: &str = "receipt_sha256"; /// The importer's run report beside each receipts file. diff --git a/crates/sqlquack-syntax-coverage/src/formats/main/load_coverage.rs b/crates/sqlquack-syntax-coverage/src/formats/main/load_coverage.rs index 92afa166c..b9827eace 100644 --- a/crates/sqlquack-syntax-coverage/src/formats/main/load_coverage.rs +++ b/crates/sqlquack-syntax-coverage/src/formats/main/load_coverage.rs @@ -35,6 +35,7 @@ pub fn load_coverage(root: &Path) -> Result> { cases: sections.cases.records, receipts: sections.receipts.records.0, retired_receipts: sections.receipts.records.1, + receipt_sql: sections.receipts.records.2, reviews: sections.reviews.records, unobservable: sections.unobservable.records.0, diagnostics: sections.unobservable.records.1, diff --git a/crates/sqlquack-syntax-coverage/src/formats/models.rs b/crates/sqlquack-syntax-coverage/src/formats/models.rs index 1090e3a6e..59df56c6a 100644 --- a/crates/sqlquack-syntax-coverage/src/formats/models.rs +++ b/crates/sqlquack-syntax-coverage/src/formats/models.rs @@ -564,6 +564,8 @@ pub struct SyntaxCoverage { pub receipts: Vec, /// SHA-256 of each published receipt line that is superseded or withdrawn. pub retired_receipts: Vec, + /// The SQL of every statement a run report read, by its SHA-256, cited by a case or not. + pub receipt_sql: BTreeMap, pub reviews: Vec, pub unobservable: Vec, pub diagnostics: Vec, diff --git a/crates/sqlquack-syntax-coverage/src/formats/types.rs b/crates/sqlquack-syntax-coverage/src/formats/types.rs index 5e1fef53b..af59e907d 100644 --- a/crates/sqlquack-syntax-coverage/src/formats/types.rs +++ b/crates/sqlquack-syntax-coverage/src/formats/types.rs @@ -2,7 +2,10 @@ use std::collections::BTreeMap; -use crate::formats::models::{Definition, Form}; +use crate::formats::models::{Definition, Form, Receipt}; /// Each family's forms and definitions, in file order. pub(crate) type FamilyCatalogue = BTreeMap, Vec)>; + +/// The current receipts, the retired receipt line hashes, and the run reports' SQL by its hash. +pub(crate) type LoadedReceipts = (Vec, Vec, BTreeMap); 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 35eeae8fa..fdd36711e 100644 --- a/crates/sqlquack-syntax-coverage/src/report/main/diff_snapshots.rs +++ b/crates/sqlquack-syntax-coverage/src/report/main/diff_snapshots.rs @@ -25,7 +25,33 @@ pub fn diff_snapshots(base: &Snapshot, head: &Snapshot) -> SnapshotDiff { .intersection(&head.syntax_error_cases_accepted) .cloned() .collect(); + let empty: BTreeSet = BTreeSet::new(); + let head_contradicted: &BTreeSet = + head.receipt_contradictions.as_ref().unwrap_or(&empty); + let new_contradictions: Vec = match &base.receipt_contradictions { + Some(base_contradicted) => head_contradicted + .difference(base_contradicted) + .cloned() + .collect(), + None => Vec::new(), + }; SnapshotDiff { + receipts_contradicted: new_contradictions + .iter() + .filter(|id| !head.documented_cases.contains(*id)) + .cloned() + .collect(), + documented_receipts_contradicted: new_contradictions + .iter() + .filter(|id| head.documented_cases.contains(*id)) + .cloned() + .collect(), + receipts_still_contradicted: head_contradicted + .iter() + .filter(|id| !new_contradictions.contains(id)) + .cloned() + .collect(), + unjudged_receipts: head.unjudged_receipts.iter().flatten().cloned().collect(), catalogue_files_changed: paths .into_iter() .filter(|path| base.catalogue_files.get(*path) != head.catalogue_files.get(*path)) diff --git a/crates/sqlquack-syntax-coverage/src/report/main/snapshot.rs b/crates/sqlquack-syntax-coverage/src/report/main/snapshot.rs index 003a8e2d0..32f0ee050 100644 --- a/crates/sqlquack-syntax-coverage/src/report/main/snapshot.rs +++ b/crates/sqlquack-syntax-coverage/src/report/main/snapshot.rs @@ -65,5 +65,7 @@ pub fn snapshot( syntax_error_cases_rejected: by_snowflake(check, SnowflakeSyntax::Refused, false), syntax_error_cases_accepted: by_snowflake(check, SnowflakeSyntax::Refused, true), documented_cases: coverage.documented_cases.clone(), + receipt_contradictions: Some(check.receipt_contradictions.clone()), + unjudged_receipts: Some(check.unjudged_receipts.clone()), } } diff --git a/crates/sqlquack-syntax-coverage/src/report/models.rs b/crates/sqlquack-syntax-coverage/src/report/models.rs index 6f78a8192..dbd6ef07f 100644 --- a/crates/sqlquack-syntax-coverage/src/report/models.rs +++ b/crates/sqlquack-syntax-coverage/src/report/models.rs @@ -73,6 +73,12 @@ pub struct Snapshot { /// Cases the catalogue marks documented (D113): their acceptance against a syntax error is intended. #[serde(default)] pub documented_cases: BTreeSet, + /// Every receipt case id whose statement the parser contradicts, cited or not; none before D141. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub receipt_contradictions: Option>, + /// Receipt case ids whose SQL no case or run report gives; none before D141. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub unjudged_receipts: Option>, } /// One snapshot against a later one; coverage and catalogue changes kept apart. @@ -95,4 +101,12 @@ pub struct SnapshotDiff { pub syntax_error_cases_accepted: Vec, /// The same flips on cases marked documented (D113): listed for the auditor, not failing (D135). pub documented_cases_accepted: Vec, + /// Receipts the head contradicts and the base did not, less documented ones: regressions (D141). + pub receipts_contradicted: Vec, + /// The same on receipts of cases marked documented (D113): listed, not failing (D135). + pub documented_receipts_contradicted: Vec, + /// Receipts both contradict, or every head contradiction when the base has no receipt verdicts. + pub receipts_still_contradicted: Vec, + /// Receipts the head cannot judge because no case or run report gives their SQL. + pub unjudged_receipts: Vec, } diff --git a/crates/sqlquack-syntax-coverage/src/report/tests/helpers.rs b/crates/sqlquack-syntax-coverage/src/report/tests/helpers.rs index 830ecd882..5f538264f 100644 --- a/crates/sqlquack-syntax-coverage/src/report/tests/helpers.rs +++ b/crates/sqlquack-syntax-coverage/src/report/tests/helpers.rs @@ -34,6 +34,8 @@ pub(crate) fn base() -> Snapshot { syntax_error_cases_rejected: BTreeSet::from(["sf.query.select.k3".to_owned()]), syntax_error_cases_accepted: BTreeSet::new(), documented_cases: BTreeSet::new(), + receipt_contradictions: Some(BTreeSet::new()), + unjudged_receipts: Some(BTreeSet::new()), } } diff --git a/crates/sqlquack-syntax-coverage/src/report/tests/test_diff_snapshots.rs b/crates/sqlquack-syntax-coverage/src/report/tests/test_diff_snapshots.rs index 8650825d4..3f7e3d11b 100644 --- a/crates/sqlquack-syntax-coverage/src/report/tests/test_diff_snapshots.rs +++ b/crates/sqlquack-syntax-coverage/src/report/tests/test_diff_snapshots.rs @@ -1,8 +1,10 @@ +use std::collections::BTreeSet; + use crate::check::models::FormState; use crate::report::main::diff_snapshots::diff_snapshots; use crate::report::models::{Snapshot, SnapshotDiff}; use crate::report::tests::helpers::{base, ids}; -use crate::report::tests::test_types::DiffTestCase; +use crate::report::tests::test_types::{BaseDiffTestCase, DiffTestCase}; #[test] fn given_a_later_snapshot_when_diffing_then_regressions_and_catalogue_changes_are_named() { @@ -105,6 +107,42 @@ fn given_a_later_snapshot_when_diffing_then_regressions_and_catalogue_changes_ar }, expected_diff: SnapshotDiff::default(), }, + DiffTestCase { + description: "a receipt the head newly contradicts, cited by a form or not, is a regression", + head: |head| Snapshot { + receipt_contradictions: Some(BTreeSet::from(["retired-1".to_owned()])), + ..head + }, + expected_diff: SnapshotDiff { + receipts_contradicted: ids(&["retired-1"]), + ..SnapshotDiff::default() + }, + }, + DiffTestCase { + description: "a documented case's receipt newly contradicted is listed apart (D135)", + head: |mut head| { + head.documented_cases + .insert("sf.query.select.k3".to_owned()); + head.receipt_contradictions = + Some(BTreeSet::from(["sf.query.select.k3".to_owned()])); + head + }, + expected_diff: SnapshotDiff { + documented_receipts_contradicted: ids(&["sf.query.select.k3"]), + ..SnapshotDiff::default() + }, + }, + DiffTestCase { + description: "a receipt without SQL to judge is listed, not a regression", + head: |head| Snapshot { + unjudged_receipts: Some(BTreeSet::from(["retired-2".to_owned()])), + ..head + }, + expected_diff: SnapshotDiff { + unjudged_receipts: ids(&["retired-2"]), + ..SnapshotDiff::default() + }, + }, DiffTestCase { description: "a verified form removed from the catalogue is also lost", head: |mut head| { @@ -123,3 +161,39 @@ fn given_a_later_snapshot_when_diffing_then_regressions_and_catalogue_changes_ar assert_eq!(diff, test_case.expected_diff, "{}", test_case.description); } } + +#[test] +fn given_a_base_without_receipt_verdicts_when_diffing_then_no_contradiction_is_new() { + let test_cases = [ + BaseDiffTestCase { + description: "every head contradiction is existing against a base from before D141", + base: |base| Snapshot { + receipt_contradictions: None, + ..base + }, + head: |head| Snapshot { + receipt_contradictions: Some(BTreeSet::from(["retired-1".to_owned()])), + ..head + }, + expected_diff: SnapshotDiff { + receipts_still_contradicted: ids(&["retired-1"]), + ..SnapshotDiff::default() + }, + }, + BaseDiffTestCase { + description: "a head from before D141 lists no receipts at all", + base: |base| base, + head: |head| Snapshot { + receipt_contradictions: None, + unjudged_receipts: None, + ..head + }, + expected_diff: SnapshotDiff::default(), + }, + ]; + for test_case in test_cases { + let diff: SnapshotDiff = + diff_snapshots(&(test_case.base)(base()), &(test_case.head)(base())); + assert_eq!(diff, test_case.expected_diff, "{}", 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 ec1e10877..842e7491b 100644 --- a/crates/sqlquack-syntax-coverage/src/report/tests/test_types.rs +++ b/crates/sqlquack-syntax-coverage/src/report/tests/test_types.rs @@ -10,6 +10,14 @@ pub(crate) struct DiffTestCase { pub(crate) expected_diff: SnapshotDiff, } +/// A change to both snapshots, and the diff it must produce. +pub(crate) struct BaseDiffTestCase { + pub(crate) description: &'static str, + pub(crate) base: fn(Snapshot) -> Snapshot, + 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, diff --git a/docs/syntax-coverage.md b/docs/syntax-coverage.md index 0cdfa6ef4..6c1ad6957 100644 --- a/docs/syntax-coverage.md +++ b/docs/syntax-coverage.md @@ -416,6 +416,24 @@ tree, so any parser commit makes its verified forms stale until it is renewed; a fingerprints goes stale only for the forms whose cases now parse differently. Either shows up as a regression. +## Receipt contradictions (decision D141) + +The case lists above only reach receipts through the catalogue: a receipt whose case id no form +cites, such as a retired or hand-written case, never counts. A snapshot therefore also records +`receipt_contradictions`: every current receipt, cited or not, whose statement the parser +contradicts. Statements are grouped by SQL, setup and session hashes and judged as above: the parser +rejects a statement Snowflake parsed, or accepts one Snowflake refused as syntax. The SQL comes from +the case that names the hash or from the `inputs` of the run reports beside the receipts, each +checked against its hash. A receipt neither gives is listed in `unjudged_receipts` and judged by +nothing. `diff` fails on a receipt the head contradicts and the base did not. A receipt of a case +marked `documented (D113)` is listed on its own line instead (decision D135). Receipts both sides +contradict, and every head contradiction when the base snapshot predates these fields, are listed in +the JSON without failing. + +The tracked baseline and receipts are a subset of the evidence, so a merge gate takes both snapshots +on the full catalogue and evidence: `snapshot` with the target branch's binary, `snapshot` with the +head's binary, then `diff BASE HEAD`. + ## Translation `syntax-coverage translation --sqlquack [--policy sqlquack.toml] [--family ] From 73a10941b1363acb029036f8f72dee2f182a5e55 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Thu, 5 Nov 2026 15:51:26 +0000 Subject: [PATCH 2/3] Fix the 53 receipts #68 contradicted Snowflake receipts show the parser over-rejecting or over-accepting these statements since #68: - Routine arguments and RETURNS TABLE columns may omit their name or type; Snowflake parses them and fails semantically (90201, 90214), including for DEFINE (import 137). The routine argument check is removed and the untyped-column check of tables is limited to non-external tables. - SHOW GRANTS OF/TO/ON reads a trailing kind word as the name and takes an empty kind before ; or LIMIT. - ALTER DYNAMIC TABLE may omit the name before SET =, take an empty UNSET TAG, a nameless SET TAG = '', an empty row access policy column list and a string DROP SEARCH OPTIMIZATION target. - CREATE TASK takes dollar-quoted property values and name tag values, and refuses AFTER once EXECUTE AS USER was read. - CREATE EXPERIMENT may omit TYPE before FROM SPECIFICATION. - CREATE STAGE and FILE FORMAT options may be comma-separated. - Backtick-quoted identifiers are refused in faithful Snowflake modes. - CREATE OR ALTER / DEFINE SEQUENCE refuses a reserved name part. --- .../src/parser/_helpers/ddl/commands.rs | 1 - .../src/parser/_helpers/ddl/create_table.rs | 15 +- .../_helpers/ddl/snowflake_alter_dynamic.rs | 49 ++++- .../parser/_helpers/ddl/snowflake_commands.rs | 12 +- .../_helpers/ddl/snowflake_create_from.rs | 14 +- .../parser/_helpers/ddl/snowflake_routines.rs | 25 +-- .../src/parser/_helpers/ddl/snowflake_show.rs | 38 +++- .../parser/_helpers/expressions/operators.rs | 7 +- .../src/parser/_helpers/statements/entry.rs | 1 + .../_helpers/statements/snowflake_context.rs | 19 ++ .../sqlquack-syntax/src/parser/constants.rs | 3 + .../tests/parser/test_snowflake_fixes.rs | 8 +- .../tests/parser/test_snowflake_recorded.rs | 177 ++++++++++++++++++ 13 files changed, 324 insertions(+), 45 deletions(-) diff --git a/crates/sqlquack-syntax/src/parser/_helpers/ddl/commands.rs b/crates/sqlquack-syntax/src/parser/_helpers/ddl/commands.rs index 821453e71..2ccd6daa7 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/ddl/commands.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/ddl/commands.rs @@ -1445,7 +1445,6 @@ impl Parser { let (parameters, has_parens) = if self.match_token(TokenType::LParen) { let params = self.parse_function_parameters()?; self.expect_token(TokenType::RParen)?; - self.check_snowflake_routine_arguments(¶ms)?; (params, true) } else { (Vec::new(), false) diff --git a/crates/sqlquack-syntax/src/parser/_helpers/ddl/create_table.rs b/crates/sqlquack-syntax/src/parser/_helpers/ddl/create_table.rs index b2f33a777..2eedb950c 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/ddl/create_table.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/ddl/create_table.rs @@ -399,7 +399,11 @@ impl Parser { snowflake_inline_properties, )?; - let as_select = self.parse_create_table_column_list_query(no_column_defs, &columns)?; + let as_select = self.parse_create_table_column_list_query( + no_column_defs, + &columns, + table_modifier == Some("EXTERNAL"), + )?; table_properties = self.parse_create_table_partitioning(table_properties)?; @@ -564,18 +568,23 @@ impl Parser { Ok(table_properties) } - /// The `AS ` after a column list; Snowflake requires one when a listed column has no type. + /// The `AS ` after a column list; Snowflake requires one when a non-external table's column has no type. fn parse_create_table_column_list_query( &mut self, no_column_defs: bool, columns: &[ColumnDef], + external: bool, ) -> Result> { let as_select = if !no_column_defs && self.match_token(TokenType::As) { Some(self.parse_statement()?) } else { None }; - if self.is_snowflake() && as_select.is_none() && columns.iter().any(|c| c.no_type) { + if self.is_snowflake() + && !external + && as_select.is_none() + && columns.iter().any(|c| c.no_type) + { return Err(self.parse_error("Expected a data type for the column")); } Ok(as_select) diff --git a/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_alter_dynamic.rs b/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_alter_dynamic.rs index 83e863a64..02b90807d 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_alter_dynamic.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_alter_dynamic.rs @@ -51,7 +51,11 @@ const ACTIONS: &[&[&str]] = &[ impl Parser { /// Parse ALTER DYNAMIC TABLE after its kind. pub(super) fn parse_alter_dynamic_table(&mut self) -> Result { - let head = self.parse_alter_object_head()?; + let head = if self.omits_dynamic_table_name() { + (crate::expressions::names::models::TableRef::new(""), false) + } else { + self.parse_alter_object_head()? + }; let mut names: Vec = Vec::new(); while self.match_token(TokenType::Comma) { names.push(Expression::Table(Box::new(self.parse_object_name()?))); @@ -71,6 +75,16 @@ impl Parser { )) } + /// Whether the table name is omitted before `SET =` which Snowflake parses and refuses by name check. + fn omits_dynamic_table_name(&self) -> bool { + let follows = |offset: usize, types: &[TokenType]| { + self.tokens + .get(self.current + offset) + .is_some_and(|token| types.contains(&token.token_type)) + }; + (self.check_identifier("SET") || self.check(TokenType::Set)) && follows(2, &[TokenType::Eq]) + } + /// Parse what follows the words of an ALTER DYNAMIC TABLE action. fn parse_dynamic_table_action(&mut self, action: &str) -> Result { Ok(match action { @@ -80,7 +94,7 @@ impl Parser { self.expect_alter_words(&[&["ADD", "ROW", "ACCESS", "POLICY"]])?; let added = self.policy_operand()?; self.expect_token(TokenType::On)?; - return Ok((vec![name, added, self.column_list_operand()?], vec![])); + return Ok((vec![name, added, self.row_access_columns()?], vec![])); } (vec![name], vec![]) } @@ -100,7 +114,11 @@ impl Parser { let mut targets: Vec = Vec::new(); if self.match_token(TokenType::On) { loop { - targets.push(self.search_target(action.starts_with("ADD"))?); + if action.starts_with("DROP") && self.check(TokenType::String) { + targets.push(self.parse_primary()?); + } else { + targets.push(self.search_target(action.starts_with("ADD"))?); + } if !self.match_token(TokenType::Comma) { break; } @@ -108,11 +126,16 @@ impl Parser { } (targets, vec![]) } - "ADD STORAGE LIFECYCLE POLICY" | "ADD ROW ACCESS POLICY" => { + "ADD STORAGE LIFECYCLE POLICY" => { let policy = self.policy_operand()?; self.expect_token(TokenType::On)?; (vec![policy, self.column_list_operand()?], vec![]) } + "ADD ROW ACCESS POLICY" => { + let policy = self.policy_operand()?; + self.expect_token(TokenType::On)?; + (vec![policy, self.row_access_columns()?], vec![]) + } "SET AGGREGATION POLICY" => { let policy = self.policy_operand()?; let mut properties: Vec = Vec::new(); @@ -129,6 +152,15 @@ impl Parser { } (vec![policy], properties) } + "UNSET TAG" if self.is_at_end() || self.check(TokenType::Semicolon) => (vec![], vec![]), + "SET TAG" if self.check(TokenType::Eq) => { + self.skip(); + let value = self.parse_object_value()?; + ( + vec![], + vec![property("TAG", Some(Identifier::new("")), Some(value))], + ) + } "SET TAG" | "UNSET TAG" => (vec![], self.parse_tag_list(action == SET_TAG)?), "SET" | "UNSET" => (vec![], self.parse_dynamic_table_properties(action == SET)?), "ALTER" | "MODIFY" => (self.parse_dynamic_column_actions()?, vec![]), @@ -141,6 +173,15 @@ impl Parser { Ok(Expression::Table(Box::new(self.parse_object_name()?))) } + /// Parse the row access policy columns, which Snowflake parses even when empty. + fn row_access_columns(&mut self) -> Result { + if self.check(TokenType::LParen) && self.check_next(TokenType::RParen) { + self.current += 2; + return Ok(tuple(vec![])); + } + self.column_list_operand() + } + /// Parse `( [ , ... ] )`. fn column_list_operand(&mut self) -> Result { self.expect_token(TokenType::LParen)?; diff --git a/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_commands.rs b/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_commands.rs index 3dcb03e15..d2f7d318b 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_commands.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_commands.rs @@ -246,24 +246,32 @@ impl Parser { /// Reads Snowflake CREATE TASK properties up to AS, refusing items that are not tags, contacts, assignments or clauses. pub(crate) fn check_snowflake_task_properties(&mut self, graph: bool) -> Result<()> { + let mut executes_as = false; while !(graph && (self.is_at_end() || self.check(TokenType::Semicolon))) && !(!graph && self.check(TokenType::As)) { self.match_token(TokenType::With); if self.match_identifier("TAG") { self.skip_snowflake_task_assignments(|parser| { - parser.expect_token(TokenType::String).map(|_| ()) + if parser.match_token(TokenType::String) { + return Ok(()); + } + parser.skip_snowflake_name() })?; } else if self.match_identifier("CONTACT") { self.skip_snowflake_task_assignments(Self::skip_snowflake_name)?; } else if self.match_identifier("TASK_DEFAULTS") { self.skip_snowflake_task_assignments(Self::skip_snowflake_task_value)?; } else if self.match_token(TokenType::After) { + if executes_as { + return Err(self.parse_error("AFTER comes before EXECUTE AS USER")); + } self.skip_snowflake_name()?; while self.match_token(TokenType::Comma) { self.skip_snowflake_name()?; } } else if self.match_text_seq(&["EXECUTE", "AS", "USER"]) { + executes_as = true; self.skip_snowflake_name()?; } else if self.match_token(TokenType::When) { if self.check(TokenType::As) { @@ -311,6 +319,8 @@ impl Parser { } self.match_token(TokenType::Dash); if self.check(TokenType::String) + || self.check(TokenType::DollarString) + || self.check(TokenType::HeredocString) || self.check(TokenType::Number) || self.check(TokenType::True) || self.check(TokenType::False) diff --git a/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_create_from.rs b/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_create_from.rs index af767c26f..d161b5273 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_create_from.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_create_from.rs @@ -96,7 +96,7 @@ impl Parser { })))) } - /// Parses `MODEL ... FROM MODEL`, `GATEWAY ... FROM SPECIFICATION` or a bare `EXPERIMENT`; None for other forms. + /// Parses `MODEL ... FROM MODEL`, `{ GATEWAY | EXPERIMENT } ... FROM SPECIFICATION` or a bare `EXPERIMENT`; None for other forms. fn parse_snowflake_create_ml_object(&mut self, or_replace: bool) -> Result> { let kind = if self.check_identifier("MODEL") && !self.check_next_identifier("MONITOR") { "MODEL" @@ -138,7 +138,10 @@ impl Parser { } "MODEL" } - "GATEWAY" => { + "GATEWAY" if !self.check(TokenType::From) => { + return Err(self.parse_error("Expected FROM SPECIFICATION")); + } + _ if self.check(TokenType::From) => { self.expect_token(TokenType::From)?; self.expect_alter_words(&[&["SPECIFICATION"]])?; if !self.check(TokenType::String) && !self.check(TokenType::DollarString) { @@ -215,6 +218,13 @@ impl Parser { None, Some(value), )); + if self.check(TokenType::Comma) + && self + .peek_nth(1) + .is_some_and(|token| token.token_type != TokenType::Semicolon) + { + self.skip(); + } } Ok(Some(Expression::CreateObject(Box::new(CreateObject { kind: kind.to_owned(), diff --git a/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_routines.rs b/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_routines.rs index 817f8a89f..359b08fe5 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_routines.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_routines.rs @@ -1,7 +1,6 @@ //! Snowflake function, procedure and object-name rules that the shared parsers do not apply. use crate::expressions::names::models::TableRef; -use crate::expressions::schema_objects::models::FunctionParameter; use crate::parser::constants::{ SNOWFLAKE_RESERVED_KEYWORDS, SNOWFLAKE_ROUTINE_NAME_LIST_PROPERTIES, SNOWFLAKE_ROUTINE_NAME_MAP_PROPERTIES, SNOWFLAKE_ROUTINE_NAME_PROPERTIES, @@ -97,7 +96,7 @@ impl Parser { Ok(()) } - /// In a Snowflake parse, refuses `TABLE ( ... )` with a reserved column name or a column lacking its type. + /// In a Snowflake parse, refuses `TABLE ( ... )` with a reserved column name. pub(crate) fn check_snowflake_table_columns(&self) -> Result<()> { if !self.is_snowflake() || !self.check(TokenType::Table) @@ -122,31 +121,13 @@ impl Parser { .iter() .any(|word| next.text.eq_ignore_ascii_case(word)) }); - let untyped = next.is_some_and(|next| next.token_type != TokenType::RParen) - && self - .tokens - .get(self.current + offset + 3) - .is_some_and(|after| { - matches!(after.token_type, TokenType::Comma | TokenType::RParen) - }); - if opens_column && (reserved || untyped) { - return Err(self.parse_error("Expected a column name and type in TABLE ( ... )")); + if opens_column && reserved { + return Err(self.parse_error("Expected a column name in TABLE ( ... )")); } } Ok(()) } - /// In a Snowflake parse, refuses a routine argument that lacks its name or its type. - pub(crate) fn check_snowflake_routine_arguments( - &self, - arguments: &[FunctionParameter], - ) -> Result<()> { - if self.is_snowflake() && arguments.iter().any(|argument| argument.name.is_none()) { - return Err(self.parse_error("Expected a name and a type for each argument")); - } - Ok(()) - } - /// In a Snowflake parse, refuses a ROW ACCESS POLICY body that is not ` ON ( , ... )`. pub(crate) fn check_snowflake_row_access_policy(&mut self) -> Result<()> { if !self.is_snowflake() { diff --git a/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_show.rs b/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_show.rs index 7c63d9b96..e59012848 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_show.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/ddl/snowflake_show.rs @@ -261,14 +261,14 @@ impl Parser { let (clause, show) = self.parse_show_grants_to(show)?; (Some(clause), show) } else if self.match_text_seq(&["OF"]) { - let words = self.expect_alter_words(&[ + let words = self.parse_show_grants_kind(&[ &["APPLICATION", "ROLE"], &["SERVICE", "ROLE"], &["DATABASE", "ROLE"], &["ROLE"], &["SHARE"], ])?; - (Some(format!("OF {words}")), show) + (Some(format!("OF {words}").trim_end().to_owned()), show) } else if self.match_token(TokenType::In) { self.expect_alter_words(&[&["DCM", "PROJECT"]])?; (Some("IN DCM PROJECT".to_owned()), show) @@ -319,14 +319,14 @@ impl Parser { { kind.push(self.advance_text()?.to_ascii_uppercase()); } - if kind.is_empty() { + if kind.is_empty() && !self.ends_show_grants_clause(1) { return Err(self.parse_error("Expected an object kind after ON")); } show.scope = Some(self.parse_show_name()?); if self.check(TokenType::LParen) { show.privileges = self.parse_function_param_types()?; } - Ok((format!("ON {}", kind.join(" ")), show)) + Ok((format!("ON {}", kind.join(" ")).trim_end().to_owned(), show)) } /// The grantee after SHOW GRANTS TO; a class instance role names its class as the target. @@ -340,8 +340,9 @@ impl Parser { &["SHARE"], &["USER"], ]; - if let Some(words) = GRANTEES.iter().find(|words| self.match_text_seq(words)) { - return Ok((format!("TO {}", words.join(" ")), show)); + if GRANTEES.iter().any(|words| self.check_text_seq(words)) { + let words = self.parse_show_grants_kind(GRANTEES)?; + return Ok((format!("TO {words}").trim_end().to_owned(), show)); } show.target = Some(self.parse_show_name()?); self.expect_alter_words(&[&["ROLE"]])?; @@ -349,6 +350,31 @@ impl Parser { Ok(("TO INSTANCE ROLE".to_owned(), show)) } + /// One of `kinds`; in Snowflake its last word is the name when nothing follows and the rest is a kind, as `OF ROLE ;`. + fn parse_show_grants_kind(&mut self, kinds: &[&[&str]]) -> Result { + if self.ends_show_grants_clause(1) { + return Ok(String::new()); + } + let words = self.expect_alter_words(kinds)?; + let shorter: Vec<&str> = words.split(' ').collect(); + let rest: &[&str] = &shorter[..shorter.len() - 1]; + if self.ends_show_grants_clause(0) && kinds.contains(&rest) { + self.current -= 1; + return Ok(rest.join(" ")); + } + Ok(words) + } + + /// Whether the statement or its LIMIT starts `offset` tokens on, in a faithful Snowflake parse. + fn ends_show_grants_clause(&self, offset: usize) -> bool { + self.applies_snowflake_context_rules() + && (self.is_statement_end_at(offset) + || self + .tokens + .get(self.current + offset) + .is_some_and(|token| token.token_type == TokenType::Limit)) + } + /// `!`, as a pair. fn parse_show_instance_role(&mut self) -> Result { let instance = self.parse_show_name()?; diff --git a/crates/sqlquack-syntax/src/parser/_helpers/expressions/operators.rs b/crates/sqlquack-syntax/src/parser/_helpers/expressions/operators.rs index 5509f79aa..b04abe71c 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/expressions/operators.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/expressions/operators.rs @@ -283,7 +283,6 @@ impl Parser { let (parameters, has_parens) = if self.match_token(TokenType::LParen) { let params = self.parse_function_parameters()?; self.expect_token(TokenType::RParen)?; - self.check_snowflake_routine_arguments(¶ms)?; (params, true) } else if self.check(TokenType::Var) && !self.check(TokenType::As) { let params = self.parse_tsql_procedure_params()?; @@ -444,7 +443,11 @@ impl Parser { let if_not_exists = self.match_keywords(&[TokenType::If, TokenType::Not, TokenType::Exists]); - let name = self.parse_table_ref()?; + let name = if self.applies_snowflake_context_rules() { + self.parse_object_name()? + } else { + self.parse_table_ref()? + }; let mut seq = CreateSequence { name, diff --git a/crates/sqlquack-syntax/src/parser/_helpers/statements/entry.rs b/crates/sqlquack-syntax/src/parser/_helpers/statements/entry.rs index fbb4eb55b..d00b724d5 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/statements/entry.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/statements/entry.rs @@ -265,6 +265,7 @@ impl Parser { /// Parses every remaining statement, split on semicolons, one `Expression` each. pub(crate) fn parse(&mut self) -> Result> { self.ensure_complexity_guards()?; + self.reject_snowflake_backticks()?; let mut statements: Vec = Vec::new(); while !self.is_at_end() { diff --git a/crates/sqlquack-syntax/src/parser/_helpers/statements/snowflake_context.rs b/crates/sqlquack-syntax/src/parser/_helpers/statements/snowflake_context.rs index 3951f1cf9..2c454b1d1 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/statements/snowflake_context.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/statements/snowflake_context.rs @@ -12,6 +12,25 @@ impl Parser { && !self.config.record_call_spans } + /// Refuses a backtick-quoted identifier, which Snowflake does not tokenize, in its faithful modes. + pub(crate) fn reject_snowflake_backticks(&mut self) -> Result<()> { + if !self.applies_snowflake_context_rules() { + return Ok(()); + } + let backtick = self.tokens.iter().position(|token| { + token.token_type == crate::tokens::models::TokenType::QuotedIdentifier + && self.source_text_range(token.span.start, token.span.start + 1) + == crate::parser::constants::BACKTICK + }); + match backtick { + Some(index) => { + self.current = index; + Err(self.parse_error("Snowflake does not quote identifiers with backticks")) + } + None => Ok(()), + } + } + /// Refuses TABLE( ) or IDENTIFIER( ) around nothing or a bare name, except a named argument's TABLE(name). pub(crate) fn reject_snowflake_name_wrappers(&self, statements: &[Expression]) -> Result<()> { use crate::traversal::types::ExpressionWalk; diff --git a/crates/sqlquack-syntax/src/parser/constants.rs b/crates/sqlquack-syntax/src/parser/constants.rs index 3322a89b4..ac25e0b6e 100644 --- a/crates/sqlquack-syntax/src/parser/constants.rs +++ b/crates/sqlquack-syntax/src/parser/constants.rs @@ -924,6 +924,9 @@ pub(crate) const SNOWFLAKE_INTERVAL_LEADING: std::ops::RangeInclusive = 1..= /// Snowflake's recorded fractional-second precisions after TO, as in `DAY TO SECOND ( 0 )`. pub(crate) const SNOWFLAKE_INTERVAL_FRACTION: std::ops::RangeInclusive = 0..=9; +/// The backtick that quotes identifiers in other dialects but not in Snowflake. +pub(crate) const BACKTICK: &str = "`"; + /// Snowflake function and procedure properties whose value is a string. pub(crate) const SNOWFLAKE_ROUTINE_TEXT_PROPERTIES: &[&str] = &[ "COMMENT", diff --git a/crates/sqlquack-syntax/tests/parser/test_snowflake_fixes.rs b/crates/sqlquack-syntax/tests/parser/test_snowflake_fixes.rs index 9d23fef72..647995332 100644 --- a/crates/sqlquack-syntax/tests/parser/test_snowflake_fixes.rs +++ b/crates/sqlquack-syntax/tests/parser/test_snowflake_fixes.rs @@ -1035,14 +1035,14 @@ fn given_a_snowflake_function_or_procedure_when_parsing_then_its_properties_are_ expected_kind: None, }, SnowflakeVerdictTestCase { - description: "an argument with a type and no name (recorded errno 1003)", + description: "an argument with a type and no name (recorded errno 90201, import 137)", sql: "DEFINE PROCEDURE d1.s1.v1 ( INT ) RETURNS INT LANGUAGE JAVASCRIPT AS 'm' ;", - expected_kind: None, + expected_kind: Some("create_procedure"), }, SnowflakeVerdictTestCase { - description: "a returned table column with no type (recorded errno 1003)", + description: "a returned table column with no type (recorded errno 90214, import 137)", sql: "DEFINE PROCEDURE d1.s1.v1 ( ) RETURNS TABLE ( v1 ) LANGUAGE SQL AS $$ BEGIN RETURN 1 ; END $$ ;", - expected_kind: None, + expected_kind: Some("create_procedure"), }, SnowflakeVerdictTestCase { description: "a reserved procedure name (recorded errno 1003)", diff --git a/crates/sqlquack-syntax/tests/parser/test_snowflake_recorded.rs b/crates/sqlquack-syntax/tests/parser/test_snowflake_recorded.rs index a3a944b1c..7aad45185 100644 --- a/crates/sqlquack-syntax/tests/parser/test_snowflake_recorded.rs +++ b/crates/sqlquack-syntax/tests/parser/test_snowflake_recorded.rs @@ -760,3 +760,180 @@ fn given_snowflake_create_and_begin_name_when_parsing_then_the_tree_keeps_the_el ); } } + +#[test] +fn given_receipts_contradicted_by_pr_68_when_parsing_then_acceptance_matches_snowflake() { + let test_cases = [ + AcceptanceTestCase { + description: "a function argument may omit its name (recorded errno 90201)", + sql: "CREATE FUNCTION v1 ( INT ) RETURNS INT AS 'm' ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "a function argument may omit its type (recorded errno 90201)", + sql: "CREATE FUNCTION v1 ( v1 ) RETURNS INT LANGUAGE JAVASCRIPT AS 'm' ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "a procedure argument may omit its type (recorded errno 90201)", + sql: "CREATE PROCEDURE v1 ( v1 ) RETURNS INT LANGUAGE SQL AS $$ BEGIN RETURN 1 ; END $$ ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "a procedure's RETURNS TABLE column may omit its type (recorded errno 90214)", + sql: "CREATE PROCEDURE v1 ( ) RETURNS TABLE ( v1 ) LANGUAGE SQL AS $$ BEGIN RETURN 1 ; END $$ ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "a procedure's RETURNS TABLE column may omit its name (recorded success)", + sql: "CREATE PROCEDURE v1 ( ) RETURNS TABLE ( INT ) LANGUAGE SQL AS $$ BEGIN RETURN 1 ; END $$ ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "SHOW GRANTS OF reads a kind word as the name (recorded errno 2003)", + sql: "SHOW GRANTS OF ROLE ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "SHOW GRANTS ON takes a bare name (recorded errno 2003)", + sql: "SHOW GRANTS ON v1 ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "SHOW GRANTS TO takes an empty kind (recorded errno 2003)", + sql: "SHOW GRANTS TO APPLICATION ROLE ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "SHOW GRANTS OF APPLICATION ROLE needs a name (recorded errno 1003)", + sql: "SHOW GRANTS OF APPLICATION ROLE ;", + dialect: Dialect::Snowflake, + expected_accepted: false, + }, + AcceptanceTestCase { + description: "a task tag value may be a name (recorded errno 391805)", + sql: "CREATE TASK v1 WITH TAG ( v1 = m1 ) AS SELECT 1 ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "a task property takes a dollar-quoted value and commas (recorded success)", + sql: "CREATE TASK v1 CONFIG = $$x$$ TIMEZONE = 'UTC', AUTOCOMMIT = TRUE AS SELECT 1", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "a task's AFTER comes before EXECUTE AS USER (recorded errno 1003)", + sql: "CREATE TASK t2 WAREHOUSE = wh EXECUTE AS USER u1 AFTER t1 AS SELECT 1", + dialect: Dialect::Snowflake, + expected_accepted: false, + }, + AcceptanceTestCase { + description: "a task's EXECUTE AS USER follows AFTER (recorded success)", + sql: "CREATE TASK t2 AFTER t1 EXECUTE AS USER u1 AS SELECT 1", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "DEFINE SEQUENCE refuses a reserved name part (recorded errno 1003)", + sql: "DEFINE SEQUENCE d1.s1.SELECT ;", + dialect: Dialect::Snowflake, + expected_accepted: false, + }, + AcceptanceTestCase { + description: "an experiment may omit its TYPE (recorded errno 400022)", + sql: "CREATE EXPERIMENT v1 FROM SPECIFICATION $$$$ ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "stage options may be comma-separated (recorded success)", + sql: "CREATE STAGE s1 COMMENT = 'x', FILE_FORMAT = ( TYPE = CSV )", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "file format options may be comma-separated (recorded success)", + sql: "CREATE FILE FORMAT f1 TYPE = 'CSV' FIELD_DELIMITER = '|', SKIP_HEADER = 2", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "Snowflake has no backtick identifiers (recorded errno 1003)", + sql: "CREATE PROCEDURE v1 ( ) RETURNS INT LANGUAGE PYTHON RUNTIME_VERSION = '3.10' ARTIFACT_REPOSITORY = ` v1 ` HANDLER = 'm1.run' ;", + dialect: Dialect::Snowflake, + expected_accepted: false, + }, + AcceptanceTestCase { + description: "ALTER DYNAMIC TABLE may omit the name before SET (recorded errno 926)", + sql: "ALTER DYNAMIC TABLE SET TARGET_LAG = '1 minute' ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "ALTER DYNAMIC TABLE refuses a missing name before UNSET (recorded errno 1003)", + sql: "ALTER DYNAMIC TABLE UNSET INITIALIZATION_WAREHOUSE ;", + dialect: Dialect::Snowflake, + expected_accepted: false, + }, + AcceptanceTestCase { + description: "a dynamic table's UNSET TAG may be empty (recorded errno 1420)", + sql: "ALTER DYNAMIC TABLE v1 UNSET TAG ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "a dynamic table's SET TAG may omit the tag name (recorded errno 1420)", + sql: "ALTER DYNAMIC TABLE v1 SET TAG = 'm' ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "a dynamic table column's UNSET TAG needs a tag (recorded errno 1003)", + sql: "ALTER DYNAMIC TABLE v1 ALTER v1 UNSET TAG ;", + dialect: Dialect::Snowflake, + expected_accepted: false, + }, + AcceptanceTestCase { + description: "a row access policy may name no columns (recorded errno 2003)", + sql: "ALTER DYNAMIC TABLE v1 ADD ROW ACCESS POLICY v1 ON ( ) ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "a storage lifecycle policy needs columns (recorded errno 1003)", + sql: "ALTER DYNAMIC TABLE v1 ADD STORAGE LIFECYCLE POLICY v1 ON ( ) ;", + dialect: Dialect::Snowflake, + expected_accepted: false, + }, + AcceptanceTestCase { + description: "DROP SEARCH OPTIMIZATION takes a string target (recorded errno 904)", + sql: "ALTER DYNAMIC TABLE v1 DROP SEARCH OPTIMIZATION ON 'v1' ;", + dialect: Dialect::Snowflake, + expected_accepted: true, + }, + AcceptanceTestCase { + description: "RESUME SEARCH OPTIMIZATION refuses a string target (recorded errno 1003)", + sql: "ALTER DYNAMIC TABLE v1 RESUME SEARCH OPTIMIZATION ON 'v1' ;", + dialect: Dialect::Snowflake, + expected_accepted: false, + }, + ]; + + for test_case in test_cases { + assert_eq!( + accepted(test_case.sql, test_case.dialect), + test_case.expected_accepted, + "{}", + test_case.description + ); + } +} From a38ef65e74e80168cd8e2d7fa7a4e18ad0e069ba Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Thu, 5 Nov 2026 15:56:13 +0000 Subject: [PATCH 3/3] Keep USE SCHEMA name parts with their own quoting USE SCHEMA "v1 Q" . v1 and USE SCHEMA "v1 Q.v1" parsed to the same tree, since the two parts were joined into one name with a single quoted flag. In faithful Snowflake modes the Use node now also lists the parts of a qualified name, each with its quoting (D118 on 58b33313). --- .../src/expressions/statements/models.rs | 3 +++ .../src/parser/_helpers/ddl/alter.rs | 6 ++++++ .../tests/parser/test_snowflake_recorded.rs | 18 ++++++++++++++++++ 3 files changed, 27 insertions(+) diff --git a/crates/sqlquack-syntax/src/expressions/statements/models.rs b/crates/sqlquack-syntax/src/expressions/statements/models.rs index 7e4a29516..55435bf91 100644 --- a/crates/sqlquack-syntax/src/expressions/statements/models.rs +++ b/crates/sqlquack-syntax/src/expressions/statements/models.rs @@ -503,6 +503,9 @@ pub struct Use { /// Snowflake's secondary roles one by one, quoting kept; empty for ALL, NONE and other kinds. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub roles: Vec, + /// Snowflake's qualified name part by part, quoting kept; empty for a one-part name. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub parts: Vec, } /// Kind of USE statement diff --git a/crates/sqlquack-syntax/src/parser/_helpers/ddl/alter.rs b/crates/sqlquack-syntax/src/parser/_helpers/ddl/alter.rs index b86cb66bf..d554ac203 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/ddl/alter.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/ddl/alter.rs @@ -1486,6 +1486,7 @@ impl Parser { true => listed, false => Vec::new(), }, + parts: Vec::new(), }))); } } @@ -1517,10 +1518,14 @@ impl Parser { let mut ident = self.expect_identifier_or_keyword_with_quoted()?; self.reject_reserved_name_part(&ident)?; + let mut parts: Vec = Vec::new(); if self.match_token(TokenType::Dot) { let second_part = self.expect_identifier_or_keyword_with_quoted()?; self.reject_reserved_name_part(&second_part)?; + if self.applies_snowflake_context_rules() { + parts = vec![ident.clone(), second_part.clone()]; + } ident.name = format!("{}.{}", ident.name, second_part.name); } @@ -1528,6 +1533,7 @@ impl Parser { kind, this: ident, roles: Vec::new(), + parts, }))) } diff --git a/crates/sqlquack-syntax/tests/parser/test_snowflake_recorded.rs b/crates/sqlquack-syntax/tests/parser/test_snowflake_recorded.rs index 7aad45185..a7c8506e2 100644 --- a/crates/sqlquack-syntax/tests/parser/test_snowflake_recorded.rs +++ b/crates/sqlquack-syntax/tests/parser/test_snowflake_recorded.rs @@ -749,6 +749,24 @@ fn given_snowflake_create_and_begin_name_when_parsing_then_the_tree_keeps_the_el pointer: "/0/select/expressions/0/cast/to/name", expected_value: json!("PERIOD ( INT )"), }, + ProjectionTestCase { + description: "USE SCHEMA keeps the database part's quoting", + sql: "USE SCHEMA \"v1 Q\" . v1", + pointer: "/0/use/parts/0/quoted", + expected_value: json!(true), + }, + ProjectionTestCase { + description: "USE SCHEMA keeps the schema part apart from the database", + sql: "USE SCHEMA \"v1 Q\" . v1", + pointer: "/0/use/parts/1/name", + expected_value: json!("v1"), + }, + ProjectionTestCase { + description: "USE SCHEMA with a quoted dotted name has one part", + sql: "USE SCHEMA \"v1 Q.v1\"", + pointer: "/0/use/parts", + expected_value: json!(null), + }, ]; for test_case in test_cases {