diff --git a/corpora/everyday/queries.toml b/corpora/everyday/queries.toml index 0bdaca95e..f4d49e172 100644 --- a/corpora/everyday/queries.toml +++ b/corpora/everyday/queries.toml @@ -2954,8 +2954,8 @@ expect_repository = "parse SQ0001" [[query]] category = "invalid_syntax" sql = '''SELECT order_id FROM orders LIMIT''' -expect = "refused SQ0003 node:limit" -expect_repository = "refused SQ0003 node:limit" +expect = "translated" +expect_repository = "translated" snowflake = "compiles" [[query]] diff --git a/crates/sqlquack-core/src/diagnostics/_helpers/mod.rs b/crates/sqlquack-core/src/diagnostics/_helpers/mod.rs index 6cbeca015..987af2dee 100644 --- a/crates/sqlquack-core/src/diagnostics/_helpers/mod.rs +++ b/crates/sqlquack-core/src/diagnostics/_helpers/mod.rs @@ -1,4 +1,5 @@ pub(crate) mod code_entry; pub(crate) mod coded; pub(crate) mod line_index; +pub(crate) mod set_operations; pub(crate) mod unparsed; diff --git a/crates/sqlquack-core/src/diagnostics/_helpers/set_operations.rs b/crates/sqlquack-core/src/diagnostics/_helpers/set_operations.rs new file mode 100644 index 000000000..a6f34e74c --- /dev/null +++ b/crates/sqlquack-core/src/diagnostics/_helpers/set_operations.rs @@ -0,0 +1,18 @@ +//! How long a statement's set-operation chain is. + +use sqlquack_syntax::tokens::models::TokenType; +use sqlquack_syntax::{Dialect, TokenKind, tokenize}; + +/// How many UNION, INTERSECT, EXCEPT and MINUS operators the source holds. +pub(crate) fn set_operations(source: &str) -> usize { + tokenize(source, Dialect::Snowflake) + .unwrap_or_default() + .iter() + .filter(|token| { + matches!( + token.kind, + TokenKind::Significant(TokenType::Union | TokenType::Intersect | TokenType::Except) + ) + }) + .count() +} diff --git a/crates/sqlquack-core/src/diagnostics/constants.rs b/crates/sqlquack-core/src/diagnostics/constants.rs index c89625c65..c0829aeb2 100644 --- a/crates/sqlquack-core/src/diagnostics/constants.rs +++ b/crates/sqlquack-core/src/diagnostics/constants.rs @@ -84,6 +84,12 @@ pub const TYPE_CONDITION: &str = "SQ0003"; pub const UNKNOWN_FUNCTION: &str = "unknown_function"; /// The name of the code of two FROM items under one alias. pub const DUPLICATE_ALIAS: &str = "duplicate_alias"; +/// The parsed statement nests deeper than SQLQuack accepts. +pub const AST_DEPTH_EXCEEDED: &str = "SQ0105"; +/// The most branches of a set-operation chain the help suggests per common table expression, well below the depth limit. +pub const SET_OPERATION_GROUP: usize = 250; +/// The fix for a statement too deep because of a long UNION, INTERSECT or EXCEPT chain. +pub const SET_OPERATION_CHAIN_HELP: &str = "split the chain: put at most 250 branches in each common table expression, then combine the expressions with the same operator"; /// The name of the code of a call with more arguments than its Snowflake function takes. pub const TOO_MANY_ARGUMENTS: &str = "too_many_arguments"; /// The name of the code of a read of a column a PIVOT consumed. diff --git a/crates/sqlquack-core/src/diagnostics/main/diagnose.rs b/crates/sqlquack-core/src/diagnostics/main/diagnose.rs index f4f47ac04..e1a66475c 100644 --- a/crates/sqlquack-core/src/diagnostics/main/diagnose.rs +++ b/crates/sqlquack-core/src/diagnostics/main/diagnose.rs @@ -3,10 +3,12 @@ use crate::diagnostics::_helpers::code_entry::code_entry; use crate::diagnostics::_helpers::coded::coded; use crate::diagnostics::_helpers::line_index::LineIndex; +use crate::diagnostics::_helpers::set_operations::set_operations; use crate::diagnostics::_helpers::unparsed::unparsed; use crate::diagnostics::constants::{ - CASE_ONLY_MATCH, CASE_ONLY_MATCH_HELP, CONVERSION_BUDGET, DIVERGENCE_REFUSED, DOUBLE_QUOTE, - GUARD_PREFIX, MISSING_RULE, NOT_EXPRESSIBLE, PARSE_FIDELITY, QUOTED_VALUE_HELP, TYPE_CONDITION, + AST_DEPTH_EXCEEDED, CASE_ONLY_MATCH, CASE_ONLY_MATCH_HELP, CONVERSION_BUDGET, + DIVERGENCE_REFUSED, DOUBLE_QUOTE, GUARD_PREFIX, MISSING_RULE, NOT_EXPRESSIBLE, PARSE_FIDELITY, + QUOTED_VALUE_HELP, SET_OPERATION_CHAIN_HELP, SET_OPERATION_GROUP, TYPE_CONDITION, UNPARSED_PREFIX, UNRESOLVED_COLUMN_PREFIX, UNRESOLVED_NAME, ZONED_LOCAL_TIME, ZONED_LOCAL_TIME_HELP, ZONED_LOCAL_TIME_NOTE, }; @@ -32,6 +34,9 @@ pub fn diagnose(refusals: &[Refusal], source: &str) -> Vec { { diagnostic.help = Some(CASE_ONLY_MATCH_HELP.to_owned()); } + if code == AST_DEPTH_EXCEEDED && set_operations(source) > SET_OPERATION_GROUP { + diagnostic.help = Some(SET_OPERATION_CHAIN_HELP.to_owned()); + } if let Some(help) = quoted_value_help(refusal, source) { diagnostic.help = Some(help); } diff --git a/crates/sqlquack-core/src/diagnostics/tests/test_render.rs b/crates/sqlquack-core/src/diagnostics/tests/test_render.rs index aa8fee009..c73785954 100644 --- a/crates/sqlquack-core/src/diagnostics/tests/test_render.rs +++ b/crates/sqlquack-core/src/diagnostics/tests/test_render.rs @@ -1,6 +1,8 @@ use crate::diagnostics::main::diagnose::diagnose; use crate::diagnostics::tests::helpers::{placed, rendered, unlocated, unparsed}; -use crate::diagnostics::tests::test_types::{GuardTestCase, RenderTestCase, UnparsedTestCase}; +use crate::diagnostics::tests::test_types::{ + GuardHelpTestCase, GuardTestCase, RenderTestCase, UnparsedTestCase, +}; use crate::refusals::models::RefusalCategory; #[test] @@ -215,3 +217,35 @@ fn given_everyday_parse_failures_when_diagnosed_then_the_label_names_what_was_ex ); } } + +#[test] +fn given_a_statement_too_deep_when_diagnosed_then_a_long_set_operation_chain_gets_its_own_fix() { + let test_cases = [ + GuardHelpTestCase { + description: "600 UNION ALL branches are told to split the chain into common table expressions", + source: vec!["SELECT order_id FROM orders"; 600].join(" UNION ALL "), + expected_help: Some( + "split the chain: put at most 250 branches in each common table expression, then \ + combine the expressions with the same operator", + ), + }, + GuardHelpTestCase { + description: "600 nested CASE expressions keep the general fix", + source: format!( + "SELECT {}1{}", + "CASE WHEN TRUE THEN ".repeat(600), + " END".repeat(600) + ), + expected_help: Some("move nested parts into common table expressions"), + }, + ]; + for test_case in test_cases { + let diagnostics = diagnose(&[unparsed(&test_case.source)], &test_case.source); + assert_eq!( + (diagnostics[0].code, diagnostics[0].help.as_deref()), + ("SQ0105", test_case.expected_help), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlquack-core/src/diagnostics/tests/test_types.rs b/crates/sqlquack-core/src/diagnostics/tests/test_types.rs index cb1667769..42ca8dd15 100644 --- a/crates/sqlquack-core/src/diagnostics/tests/test_types.rs +++ b/crates/sqlquack-core/src/diagnostics/tests/test_types.rs @@ -34,3 +34,9 @@ pub(crate) struct UnparsedTestCase { pub(crate) expected_column: usize, pub(crate) expected_help: Option<&'static str>, } + +pub(crate) struct GuardHelpTestCase { + pub(crate) description: &'static str, + pub(crate) source: String, + pub(crate) expected_help: Option<&'static str>, +} diff --git a/crates/sqlquack-syntax/src/parser/_helpers/queries/select.rs b/crates/sqlquack-syntax/src/parser/_helpers/queries/select.rs index 536f14462..0498c7bf6 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/queries/select.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/queries/select.rs @@ -43,7 +43,7 @@ impl Parser { || self.check(TokenType::Intersect) || self.check(TokenType::Except) || self.check(TokenType::Order) - || self.check(TokenType::Limit) + || self.check(TokenType::Limit) && !self.snowflake_limit_qualifier() || self.check(TokenType::Qualify) || self.check(TokenType::Semicolon) || self.check_text_seq(&["BULK", "COLLECT", "INTO"]) @@ -76,24 +76,25 @@ impl Parser { self.parse_select_item_colon_alias(expr, pre_alias_comments)? } else if self.match_token(TokenType::As) { self.parse_select_item_as_alias(expr, leading_comments, pre_alias_comments)? - } else if ((self.check(TokenType::Var) && !self.check_keyword() - || self.check(TokenType::QuotedIdentifier) - || self.check(TokenType::String) && self.supports_string_aliases(false) - || self.can_be_alias_keyword() - || self.is_command_keyword_as_alias() - || self.check(TokenType::Overlaps)) - && !self.check_text_seq(&["BULK", "COLLECT", "INTO"])) - && !(self.check(TokenType::Fetch) - || self.check(TokenType::Limit) - || self.check(TokenType::Offset)) - && !self.check_text_seq(&["GROUP", "BY"]) - && !self.check_text_seq(&["ORDER", "BY"]) - && !self.check(TokenType::Qualify) - && !self.is_snowflake_reserved_keyword() - && !(self.check(TokenType::Window) - && self.peek_nth(1).is_some_and(|token| { - matches!(token.token_type, TokenType::Var | TokenType::Identifier) - })) + } else if self.limit_as_snowflake_alias() + || (((self.check(TokenType::Var) && !self.check_keyword() + || self.check(TokenType::QuotedIdentifier) + || self.check(TokenType::String) && self.supports_string_aliases(false) + || self.can_be_alias_keyword() + || self.is_command_keyword_as_alias() + || self.check(TokenType::Overlaps)) + && !self.check_text_seq(&["BULK", "COLLECT", "INTO"])) + && !(self.check(TokenType::Fetch) + || self.check(TokenType::Limit) + || self.check(TokenType::Offset)) + && !self.check_text_seq(&["GROUP", "BY"]) + && !self.check_text_seq(&["ORDER", "BY"]) + && !self.check(TokenType::Qualify) + && !self.is_snowflake_reserved_keyword() + && !(self.check(TokenType::Window) + && self.peek_nth(1).is_some_and(|token| { + matches!(token.token_type, TokenType::Var | TokenType::Identifier) + }))) { let alias = self.parse_projection_alias_identifier(false)?; let trailing_comments = self.previous_trailing_comments().to_vec(); diff --git a/crates/sqlquack-syntax/src/parser/_helpers/statements/analyze.rs b/crates/sqlquack-syntax/src/parser/_helpers/statements/analyze.rs index 20a0a9cf4..6a1f3c86b 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/statements/analyze.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/statements/analyze.rs @@ -263,6 +263,7 @@ impl Parser { || self.check(TokenType::Pivot) && !self.check_next(TokenType::LParen) || self.check(TokenType::Unpivot) && !self.is_unpivot_clause_start() || self.check(TokenType::Partition) + || self.limit_as_snowflake_alias() || self.check(TokenType::Window) && { let next_pos = self.current + 1; next_pos >= self.tokens.len() @@ -310,6 +311,27 @@ impl Parser { .any(|keyword| self.peek().text.eq_ignore_ascii_case(keyword)) } + /// Whether a bare Snowflake LIMIT or OFFSET with no row count after it is an alias, as Snowflake reads it. + pub(crate) fn limit_as_snowflake_alias(&self) -> bool { + let followers: &[TokenType] = match self.peek().token_type { + TokenType::Limit => &crate::parser::constants::SNOWFLAKE_LIMIT_ALIAS_FOLLOWERS, + TokenType::Offset => &crate::parser::constants::SNOWFLAKE_OFFSET_ALIAS_FOLLOWERS, + _ => &[], + }; + self.config.dialect == Some(crate::tokens::models::DialectType::Snowflake) + && !followers.is_empty() + && self.tokens.get(self.current + 1).is_none_or(|next| { + next.token_type == TokenType::Eof || followers.contains(&next.token_type) + }) + } + + /// Whether a Snowflake LIMIT qualifies a column, as `LIMIT.a` does after `FROM ... LIMIT`. + pub(crate) fn snowflake_limit_qualifier(&self) -> bool { + self.config.dialect == Some(crate::tokens::models::DialectType::Snowflake) + && self.check(TokenType::Limit) + && self.check_next(TokenType::Dot) + } + /// Whether a keyword may be a table alias; LEFT, RIGHT, OUTER and FULL may. pub(crate) fn can_be_alias_keyword(&self) -> bool { if self.is_at_end() { diff --git a/crates/sqlquack-syntax/src/parser/constants.rs b/crates/sqlquack-syntax/src/parser/constants.rs index a304c98d2..da77a6608 100644 --- a/crates/sqlquack-syntax/src/parser/constants.rs +++ b/crates/sqlquack-syntax/src/parser/constants.rs @@ -1274,3 +1274,18 @@ pub(crate) const SNOWFLAKE_BODY_POLICY_KINDS: &[&str] = &[ "ROW ACCESS POLICY", "STORAGE LIFECYCLE POLICY", ]; + +/// The tokens after which Snowflake reads a bare LIMIT as an alias (receipted in import 163); end of input too. +pub(crate) const SNOWFLAKE_LIMIT_ALIAS_FOLLOWERS: [crate::tokens::models::TokenType; 5] = [ + crate::tokens::models::TokenType::Semicolon, + crate::tokens::models::TokenType::RParen, + crate::tokens::models::TokenType::Where, + crate::tokens::models::TokenType::Order, + crate::tokens::models::TokenType::Limit, +]; + +/// The tokens after which Snowflake reads a bare OFFSET as an alias (receipted in import 163); end of input too. +pub(crate) const SNOWFLAKE_OFFSET_ALIAS_FOLLOWERS: [crate::tokens::models::TokenType; 2] = [ + crate::tokens::models::TokenType::Semicolon, + crate::tokens::models::TokenType::RParen, +]; diff --git a/crates/sqlquack-syntax/tests/parser/test_fixes.rs b/crates/sqlquack-syntax/tests/parser/test_fixes.rs index 7e951b62f..3ab4eb2a8 100644 --- a/crates/sqlquack-syntax/tests/parser/test_fixes.rs +++ b/crates/sqlquack-syntax/tests/parser/test_fixes.rs @@ -1753,3 +1753,61 @@ fn given_a_time_travel_clause_when_parsing_snowflake_then_its_parentheses_close( ); } } + +#[test] +fn given_a_bare_snowflake_limit_or_offset_when_parsing_then_it_is_an_alias() { + let test_cases = [ + StatementTestCase { + description: "a bare LIMIT names the column before it (recorded: the column is LIMIT)", + sql: "SELECT 1 LIMIT ;", + pointer: "/0/select/expressions/0/alias/alias/name", + expected_value: json!("LIMIT"), + }, + StatementTestCase { + description: + "a bare LIMIT names the FROM item, which LIMIT.a reads (recorded as accepted)", + sql: "SELECT LIMIT.a FROM ( SELECT 1 AS a ) LIMIT ;", + pointer: "/0/select/expressions/0/column/table/name", + expected_value: json!("LIMIT"), + }, + StatementTestCase { + description: "LIMIT before WHERE is an alias (recorded as accepted)", + sql: "SELECT a FROM ( SELECT 1 AS a ) LIMIT WHERE a = 1 ;", + pointer: "/0/select/from/expressions/0/subquery/alias/name", + expected_value: json!("LIMIT"), + }, + StatementTestCase { + description: "LIMIT before ORDER BY is an alias (recorded as accepted)", + sql: "SELECT a FROM ( SELECT 1 AS a ) LIMIT ORDER BY a ;", + pointer: "/0/select/from/expressions/0/subquery/alias/name", + expected_value: json!("LIMIT"), + }, + StatementTestCase { + description: "LIMIT LIMIT 1 is an alias, then a row limit (recorded as accepted)", + sql: "SELECT a FROM ( SELECT 1 AS a ) LIMIT LIMIT 1 ;", + pointer: "/0/select/limit/this/literal/value", + expected_value: json!("1"), + }, + StatementTestCase { + description: "a bare OFFSET is an alias too (recorded as accepted)", + sql: "SELECT a FROM ( SELECT 1 AS a ) OFFSET ;", + pointer: "/0/select/from/expressions/0/subquery/alias/name", + expected_value: json!("OFFSET"), + }, + StatementTestCase { + description: "LIMIT with a row count stays a row limit", + sql: "SELECT a FROM ( SELECT 1 AS a ) LIMIT 5 ;", + pointer: "/0/select/from/expressions/0/subquery/alias", + expected_value: json!(null), + }, + ]; + + for test_case in test_cases { + assert_eq!( + statement_at(test_case.sql, test_case.pointer), + test_case.expected_value, + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlquack/src/analysis/tests/test_everyday_corpus.rs b/crates/sqlquack/src/analysis/tests/test_everyday_corpus.rs index 1d40f671e..92dd8884e 100644 --- a/crates/sqlquack/src/analysis/tests/test_everyday_corpus.rs +++ b/crates/sqlquack/src/analysis/tests/test_everyday_corpus.rs @@ -9,12 +9,10 @@ fn given_everyday_snowflake_queries_when_checked_and_converted_then_each_keeps_i let test_cases = [EverydayCorpusTestCase { description: "every corpora/everyday query parses, checks, converts or refuses as recorded, under the strict built-in policy and the repository policy", expected_mismatches: Vec::new(), - expected_rates: "287 of 436 strict, 356 of 436 repository", + expected_rates: "288 of 436 strict, 357 of 436 repository", // SQL Snowflake rejects (snowflake-receipts.json) that SQLQuack neither fails to parse nor // flags in its checks; each should become a parse or check error. - expected_rejections_not_caught: vec![ - "SELECT order_id FROM orders WHERE ordered_at > 5", - ], + expected_rejections_not_caught: vec!["SELECT order_id FROM orders WHERE ordered_at > 5"], }]; for test_case in test_cases {