diff --git a/crates/sqlquack-convert/src/snowflake/conversion/_helpers/queries/query.rs b/crates/sqlquack-convert/src/snowflake/conversion/_helpers/queries/query.rs index ba93276db..14985124d 100644 --- a/crates/sqlquack-convert/src/snowflake/conversion/_helpers/queries/query.rs +++ b/crates/sqlquack-convert/src/snowflake/conversion/_helpers/queries/query.rs @@ -4,7 +4,8 @@ use serde_json::{Map, Value, json}; use sqlquack_blocks::blocks::main::quoted_identifier::quoted_identifier; use sqlquack_diagnostics::classification::models::{Construct, ConstructFamily}; use sqlquack_diagnostics::diagnostics::constants::{ - REPEATED_RESULT_NAME, UNALIASED_OUTPUT_NAME_CONSTRUCT, + REPEATED_RESULT_NAME, UNALIASED_OUTPUT_NAME_CONSTRUCT, UNALIASED_QUOTED_NAME, + UNALIASED_TEXT_NAME, }; use sqlquack_diagnostics::provenance::constants::QUERY_CONSTRUCT; use sqlquack_diagnostics::provenance::models::ShapeArgument; @@ -34,6 +35,7 @@ use crate::snowflake::conversion::_helpers::support::tree::{ function_construct, identifier_name, list, node_construct, node_kind, payload_object, require_understood, }; +use crate::snowflake::conversion::constants::UNALIASED_QUOTED_KINDS; use crate::snowflake::conversion::constants::WINDOW_MARKER; use crate::snowflake::conversion::constants::{ ALIAS, COMMENT, COMMON_TABLE, DISTINCT, GENERATOR_SEQUENCE_COLUMN, GROUP_BY, HAVING, @@ -694,7 +696,10 @@ fn convert_projection( location: site.context.locator.locate_written(item), ..missing( Construct::new(ConstructFamily::Syntax, UNALIASED_OUTPUT_NAME_CONSTRUCT), - format!("Snowflake names this `{kind}` column after its text"), + match UNALIASED_QUOTED_KINDS.contains(&kind) { + true => UNALIASED_QUOTED_NAME.to_owned(), + false => UNALIASED_TEXT_NAME.to_owned(), + }, ) }), } diff --git a/crates/sqlquack-convert/src/snowflake/conversion/constants.rs b/crates/sqlquack-convert/src/snowflake/conversion/constants.rs index 2210b477b..1fdb55c7c 100644 --- a/crates/sqlquack-convert/src/snowflake/conversion/constants.rs +++ b/crates/sqlquack-convert/src/snowflake/conversion/constants.rs @@ -1384,3 +1384,6 @@ pub(crate) const CORRELATED_DERIVED: &str = pub(crate) const AMBIGUOUS_SUFFIX: &str = "is ambiguous"; /// A JSON-held VARIANT as a sort key, in ARRAY_SORT's recorded order of kinds. pub const VARIANT_ORDER_KEY: &str = "rule.variant_order_key"; + +/// The select item kinds whose Snowflake name keeps quotes or a path: literals and paths into semi-structured values. +pub(crate) const UNALIASED_QUOTED_KINDS: &[&str] = &["j_s_o_n_extract", "literal", "subscript"]; diff --git a/crates/sqlquack-diagnostics/src/diagnostics/constants.rs b/crates/sqlquack-diagnostics/src/diagnostics/constants.rs index bdbdaf0c2..7706cf72d 100644 --- a/crates/sqlquack-diagnostics/src/diagnostics/constants.rs +++ b/crates/sqlquack-diagnostics/src/diagnostics/constants.rs @@ -12,6 +12,13 @@ pub const CASE_ONLY_MATCH_HELP: &str = "Snowflake folds unquoted names to upper quote the name in the SQL to keep its case"; /// The words of an unresolved name's reason when two result columns share a name. pub const REPEATED_RESULT_NAME: &str = "repeats an earlier result column's name"; +/// The label of an unaliased item Snowflake names after its text. +pub const UNALIASED_TEXT_NAME: &str = "Snowflake names this column after its text"; +/// The label of an unaliased literal or path, whose Snowflake name keeps its quotes or path, so no alias is suggested. +pub const UNALIASED_QUOTED_NAME: &str = + "Snowflake names this column after its full text, quotes and path included"; +/// The help for an unaliased literal or path. +pub const UNALIASED_QUOTED_NAME_HELP: &str = "add an alias, which names the column in both engines"; /// The help for such a name, replacing SQ0004's own. pub const REPEATED_RESULT_NAME_HELP: &str = "give each result column its own alias, such as `SELECT 1 AS first, 1 AS second`"; diff --git a/crates/sqlquack-diagnostics/src/diagnostics/main/diagnose.rs b/crates/sqlquack-diagnostics/src/diagnostics/main/diagnose.rs index 8f3083a64..bbb682f2f 100644 --- a/crates/sqlquack-diagnostics/src/diagnostics/main/diagnose.rs +++ b/crates/sqlquack-diagnostics/src/diagnostics/main/diagnose.rs @@ -10,8 +10,8 @@ use crate::diagnostics::constants::{ CASE_ONLY_MATCH, CASE_ONLY_MATCH_HELP, CONVERSION_BUDGET, DIVERGENCE_REFUSED, FUNCTION_OPERAND_HELP, GUARD_PREFIX, JINJA_HELP, MISSING_RULE, NOT_EXPRESSIBLE, PARSE_FIDELITY, REPEATED_RESULT_NAME, REPEATED_RESULT_NAME_HELP, SUGGESTED_NAME_MAX_CHARS, TYPE_CONDITION, - UNALIASED_OUTPUT_NAME, UNPARSED_PREFIX, UNRESOLVED_NAME, ZONED_LOCAL_TIME, - ZONED_LOCAL_TIME_HELP, ZONED_LOCAL_TIME_NOTE, + UNALIASED_OUTPUT_NAME, UNALIASED_QUOTED_NAME, UNALIASED_QUOTED_NAME_HELP, UNPARSED_PREFIX, + UNRESOLVED_NAME, ZONED_LOCAL_TIME, ZONED_LOCAL_TIME_HELP, ZONED_LOCAL_TIME_NOTE, }; use crate::diagnostics::models::Diagnostic; use crate::refusals::models::{Refusal, RefusalCategory}; @@ -52,7 +52,9 @@ pub fn diagnose(refusals: &[Refusal], source: &str) -> Vec { { diagnostic.help = Some(FUNCTION_OPERAND_HELP.to_owned()); } - if code == UNALIASED_OUTPUT_NAME + if code == UNALIASED_OUTPUT_NAME && refusal.reason.contains(UNALIASED_QUOTED_NAME) { + diagnostic.help = Some(UNALIASED_QUOTED_NAME_HELP.to_owned()); + } else if code == UNALIASED_OUTPUT_NAME && let Some(name) = diagnostic .span .and_then(|span| suggested_name(source, span.start, span.end)) diff --git a/crates/sqlquack-syntax-snowflake/src/parser/_helpers/expressions/primary.rs b/crates/sqlquack-syntax-snowflake/src/parser/_helpers/expressions/primary.rs index 24b124043..f7c24d367 100644 --- a/crates/sqlquack-syntax-snowflake/src/parser/_helpers/expressions/primary.rs +++ b/crates/sqlquack-syntax-snowflake/src/parser/_helpers/expressions/primary.rs @@ -1,6 +1,7 @@ //! Parser expressions: primary. use crate::parser::constants::ARRAY; +use crate::parser::constants::CONDITION_CLAUSES; use crate::parser::constants::MAP; use crate::parser::constants::ODBC_DATE_ESCAPE; use crate::parser::constants::ODBC_TIMESTAMP_ESCAPE; @@ -441,6 +442,14 @@ impl Parser { return self.maybe_parse_subscript(col); } + if self.is_at_end() || self.check(TokenType::Semicolon) { + let previous = self.previous().text.to_ascii_uppercase(); + let expected = match CONDITION_CLAUSES.contains(&previous.as_str()) { + true => "a condition", + false => "an expression", + }; + return Err(self.parse_error(format!("Expected {expected} after {previous}"))); + } Err(self.parse_error(format!("Unexpected token: {:?}", self.peek().token_type))) } diff --git a/crates/sqlquack-syntax-snowflake/src/parser/_helpers/statements/analyze.rs b/crates/sqlquack-syntax-snowflake/src/parser/_helpers/statements/analyze.rs index f7a3176dc..e9e6195cb 100644 --- a/crates/sqlquack-syntax-snowflake/src/parser/_helpers/statements/analyze.rs +++ b/crates/sqlquack-syntax-snowflake/src/parser/_helpers/statements/analyze.rs @@ -122,7 +122,7 @@ impl Parser { false } - /// Whether a keyword may be an identifier: structural ones (FROM, WHERE, ...) may not. + /// Whether a keyword may be an identifier: structural ones (FROM, WHERE, ...) may not, nor GROUP or ORDER before BY, which start a clause. pub(crate) fn is_safe_keyword_as_identifier(&self) -> bool { if self.is_at_end() { return false; @@ -156,8 +156,10 @@ impl Parser { | TokenType::Lateral | TokenType::Natural ); + let starts_clause = matches!(token_type, TokenType::Group | TokenType::Order) + && self.check_next(TokenType::By); - self.peek().token_type.is_keyword() && !is_structural + self.peek().token_type.is_keyword() && !is_structural && !starts_clause } /// Whether the token here ends the expression, so IS or KEEP is an alias, not an operator. diff --git a/crates/sqlquack-syntax-snowflake/src/parser/constants.rs b/crates/sqlquack-syntax-snowflake/src/parser/constants.rs index ddadcf326..b86e98f10 100644 --- a/crates/sqlquack-syntax-snowflake/src/parser/constants.rs +++ b/crates/sqlquack-syntax-snowflake/src/parser/constants.rs @@ -191,6 +191,9 @@ pub(crate) const SET: &str = "SET"; /// The text "SET MASKING POLICY", as comparisons test it. pub(crate) const SET_MASKING_POLICY: &str = "SET MASKING POLICY"; +/// The clauses whose operand is a condition, as an error names what is missing after one. +pub(crate) const CONDITION_CLAUSES: &[&str] = &["HAVING", "ON", "QUALIFY", "WHERE"]; + /// The text "SET TAG", as comparisons test it. pub(crate) const SET_TAG: &str = "SET TAG"; diff --git a/crates/sqlquack-syntax-snowflake/tests/parser.rs b/crates/sqlquack-syntax-snowflake/tests/parser.rs index 8bdf79284..a118c77b0 100644 --- a/crates/sqlquack-syntax-snowflake/tests/parser.rs +++ b/crates/sqlquack-syntax-snowflake/tests/parser.rs @@ -8,6 +8,8 @@ mod test_fixes; mod test_located; #[path = "parser/test_lossless.rs"] mod test_lossless; +#[path = "parser/test_parse_errors.rs"] +mod test_parse_errors; #[path = "parser/test_scripting.rs"] mod test_scripting; #[path = "parser/test_snowflake_alter.rs"] diff --git a/crates/sqlquack-syntax-snowflake/tests/parser/test_parse_errors.rs b/crates/sqlquack-syntax-snowflake/tests/parser/test_parse_errors.rs new file mode 100644 index 000000000..c36132424 --- /dev/null +++ b/crates/sqlquack-syntax-snowflake/tests/parser/test_parse_errors.rs @@ -0,0 +1,106 @@ +use sqlquack_syntax::{CallMode, Dialect, ParseOptions}; +use sqlquack_syntax_snowflake::parse_statements_with; + +use crate::test_types::{ParseErrorTestCase, ParseMessageTestCase}; + +#[test] +fn given_group_or_order_by_after_an_operator_when_parsing_snowflake_then_the_error_is_at_the_clause( +) { + let test_cases = [ + ParseErrorTestCase { + description: "AND before GROUP BY on the next line fails at GROUP, not at BY", + sql: "SELECT customer_id, SUM(total) AS revenue\nFROM orders\nWHERE status = 'shipped' AND\nGROUP BY customer_id", + expected_error_at: Some("GROUP"), + }, + ParseErrorTestCase { + description: "AND before ORDER BY on the same line fails at ORDER", + sql: "SELECT order_id FROM orders WHERE total > 1 AND ORDER BY order_id", + expected_error_at: Some("ORDER"), + }, + ParseErrorTestCase { + description: "NOT before GROUP BY fails at GROUP", + sql: "SELECT status FROM orders WHERE NOT\ngroup by status", + expected_error_at: Some("group"), + }, + ParseErrorTestCase { + description: "GROUP BY and ORDER BY after WHERE still parse", + sql: "SELECT status FROM orders WHERE total > 1\nGROUP BY status\nORDER BY status", + expected_error_at: None, + }, + ParseErrorTestCase { + description: "a quoted GROUP column still parses", + sql: "SELECT \"GROUP\" FROM orders ORDER BY 1", + expected_error_at: None, + }, + ]; + + for test_case in test_cases { + let error_at = parse_statements_with( + test_case.sql, + Dialect::Snowflake, + &ParseOptions::default(), + CallMode::Faithful, + ) + .err() + .map(|error| { + let start = error.start().unwrap_or_default(); + let end = error.end().unwrap_or_default(); + test_case.sql.get(start..end).unwrap_or_default().to_owned() + }); + assert_eq!( + error_at.as_deref(), + test_case.expected_error_at, + "{}", + test_case.description + ); + } +} + +#[test] +fn given_a_statement_ending_where_an_expression_belongs_when_parsing_snowflake_then_the_error_names_what_is_missing( +) { + let test_cases = [ + ParseMessageTestCase { + description: "an empty WHERE asks for a condition", + sql: "SELECT * FROM orders WHERE", + expected_message: "Expected a condition after WHERE", + }, + ParseMessageTestCase { + description: "an empty WHERE before a semicolon asks for a condition", + sql: "SELECT * FROM orders WHERE ;", + expected_message: "Expected a condition after WHERE", + }, + ParseMessageTestCase { + description: "an empty HAVING asks for a condition", + sql: "SELECT status FROM orders GROUP BY status HAVING", + expected_message: "Expected a condition after HAVING", + }, + ParseMessageTestCase { + description: "a join's empty ON asks for a condition", + sql: "SELECT 1 FROM orders o JOIN customers c ON", + expected_message: "Expected a condition after ON", + }, + ParseMessageTestCase { + description: "a comparison with no right side asks for an expression", + sql: "SELECT 1 FROM orders WHERE total >", + expected_message: "Expected an expression after >", + }, + ]; + + for test_case in test_cases { + let message = parse_statements_with( + test_case.sql, + Dialect::Snowflake, + &ParseOptions::default(), + CallMode::Faithful, + ) + .err() + .map(|error| error.to_string()) + .unwrap_or_default(); + assert!( + message.contains(test_case.expected_message), + "{}: {message}", + test_case.description + ); + } +} diff --git a/crates/sqlquack-syntax-snowflake/tests/parser/test_types.rs b/crates/sqlquack-syntax-snowflake/tests/parser/test_types.rs index d7908b555..a81b8a118 100644 --- a/crates/sqlquack-syntax-snowflake/tests/parser/test_types.rs +++ b/crates/sqlquack-syntax-snowflake/tests/parser/test_types.rs @@ -67,6 +67,18 @@ pub(crate) struct ScriptingVerdictTestCase { pub(crate) expected_error: Option<&'static str>, } +pub(crate) struct ParseErrorTestCase { + pub(crate) description: &'static str, + pub(crate) sql: &'static str, + pub(crate) expected_error_at: Option<&'static str>, +} + +pub(crate) struct ParseMessageTestCase { + pub(crate) description: &'static str, + pub(crate) sql: &'static str, + pub(crate) expected_message: &'static str, +} + pub(crate) struct ScriptingDialectTestCase { pub(crate) description: &'static str, pub(crate) sql: &'static str, diff --git a/crates/sqlquack/tests/cli/test_convert.rs b/crates/sqlquack/tests/cli/test_convert.rs index c2839610a..a0b9e3582 100644 --- a/crates/sqlquack/tests/cli/test_convert.rs +++ b/crates/sqlquack/tests/cli/test_convert.rs @@ -1566,7 +1566,7 @@ fn given_refused_sql_when_converting_then_text_and_json_carry_the_same_coded_dia expected_text: concat!( "error[SQ0009]: This column needs an alias, because SQLQuack can't be sure of the name ", "Snowflake gives it\n --> :1:18\n |\n1 | SELECT 'a' AS a, UPPER('é')\n", - " | ^^^^^^^^^^ Snowflake names this `function` column after its text\n", + " | ^^^^^^^^^^ Snowflake names this column after its text\n", " |\n = help: add an alias, such as `AS total`, which names the column in both engines\n", "sqlquack convert: refused, 1 refusal\n", ), diff --git a/crates/sqlquack/tests/cli/test_output_names.rs b/crates/sqlquack/tests/cli/test_output_names.rs index c34eb74f3..d207b7da3 100644 --- a/crates/sqlquack/tests/cli/test_output_names.rs +++ b/crates/sqlquack/tests/cli/test_output_names.rs @@ -489,8 +489,8 @@ fn given_a_refused_result_name_when_converting_then_the_diagnostic_underlines_th expected_text: concat!( "error[SQ0009]: This column needs an alias, because SQLQuack can't be sure of the name ", "Snowflake gives it\n --> :1:18\n |\n1 | SELECT order_id, 'it''s' FROM orders\n", - " | ^^^^^^^ Snowflake names this `literal` column after its text\n", - " |\n = help: add an alias, such as `AS \"'IT''S'\"`, which names the column in both engines\n", + " | ^^^^^^^ Snowflake names this column after its full text, quotes and ", + "path included\n |\n = help: add an alias, which names the column in both engines\n", "sqlquack convert: refused, 1 refusal\n", ), expected_span: ("SQ0009", 17, 24),