From d157b5cabf6f5103e2d1eb992a003ae5adc965ce Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 6 Nov 2026 22:37:54 +0000 Subject: [PATCH 1/3] Let comments never change whether a statement converts (D327) A comment straight after SELECT set post_select_comments, which no rule read, so the whole statement refused (602 of the playground's 3,263 generated statements). It is now cosmetic like every other comment field. A comment after an expression wrapped it in an annotated node, so a rule needing a literal, such as ROUND's scale, saw a wrapper and refused; a wrapper that holds only comments is now unwrapped. Both are dropped from the output, as comments elsewhere are. --- .../src/conversion/_helpers/support/tree.rs | 22 ++++++++++++++++++- .../src/analysis/tests/test_analyze.rs | 21 ++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs b/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs index 32afd251b..b0d1ffb1b 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs @@ -18,6 +18,7 @@ const COSMETIC_FIELDS: &[&str] = &[ "comments", "leading_comments", "pre_alias_comments", + "post_select_comments", "left_comments", "operator_comments", "right_comments", @@ -194,13 +195,32 @@ pub(crate) fn integer_literal(node: &Value) -> Option { /// The tree conversion reads, positions stripped and quoted identifiers folded if case is ignored. pub(crate) fn session_tree(positioned: &Value, ignore_quoted_case: bool) -> Result { - let mut tree = strip_positions(positioned); + let mut tree = without_comment_wrappers(&strip_positions(positioned)); if ignore_quoted_case { fold_quoted(&mut tree)?; } Ok(tree) } +/// `value` with each node that only wrapped a comment replaced by the node it wrapped. +fn without_comment_wrappers(value: &Value) -> Value { + match value { + Value::Object(fields) => match fields.get("annotated").and_then(Value::as_object) { + Some(annotated) if fields.len() == 1 && annotated.len() == 1 => annotated + .get("this") + .map_or_else(|| value.clone(), without_comment_wrappers), + _ => Value::Object( + fields + .iter() + .map(|(key, field)| (key.clone(), without_comment_wrappers(field))) + .collect(), + ), + }, + Value::Array(items) => Value::Array(items.iter().map(without_comment_wrappers).collect()), + other => other.clone(), + } +} + /// Upper-cases the name of each quoted identifier under `value`. fn fold_quoted(value: &mut Value) -> Result<(), String> { match value { diff --git a/crates/sqlquack/src/analysis/tests/test_analyze.rs b/crates/sqlquack/src/analysis/tests/test_analyze.rs index 84eccf154..6f5c6dad8 100644 --- a/crates/sqlquack/src/analysis/tests/test_analyze.rs +++ b/crates/sqlquack/src/analysis/tests/test_analyze.rs @@ -92,6 +92,27 @@ fn given_sql_and_a_schema_when_analyzing_then_it_converts_or_refuses_with_a_cate converted: true, }, }, + AnalyzeTestCase { + description: "a block comment straight after SELECT is dropped", + sql: "SELECT /* c */ amount AS v FROM orders", + expected_summary: converted(), + }, + AnalyzeTestCase { + description: "a line comment straight after SELECT is dropped", + sql: "SELECT -- note\n amount AS v FROM orders", + expected_summary: converted(), + }, + AnalyzeTestCase { + description: "a comment after SELECT inside a CTE and a subquery is dropped", + sql: "WITH x AS (SELECT /* c */ amount FROM orders) \ + SELECT v FROM (SELECT /* d */ amount AS v FROM x) AS s", + expected_summary: converted(), + }, + AnalyzeTestCase { + description: "a comment after a literal argument leaves the literal a literal", + sql: "SELECT ROUND(amount, 1 /* c */) AS v FROM orders", + expected_summary: converted(), + }, AnalyzeTestCase { description: "comments after select items and inside expressions are dropped", sql: "SELECT amount /* net */ + 1 AS gross, -- trailing\n customer_id -- last\n \ From 7392627b89f7612b45eae4e9640b78aee086432f Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 6 Nov 2026 23:13:58 +0000 Subject: [PATCH 2/3] Refuse an unaliased select item whose text holds a comment (D327) Unwrapping comment-only wrappers dropped the comment from an unaliased literal's written text, so SELECT 'x' /* c */ took the name 'X', which no receipt records. Main had the same gap. The select item's own wrapper now stays, and the projection rule refuses it with SQ0009 unless it wraps a column, star or alias, as the column-name rule's conditions state. --- .../src/conversion/_helpers/queries/query.rs | 15 ++++- .../src/conversion/_helpers/support/tree.rs | 56 ++++++++++++++++--- .../src/analysis/tests/test_analyze.rs | 5 ++ 3 files changed, 66 insertions(+), 10 deletions(-) diff --git a/crates/sqlquack-rules/src/conversion/_helpers/queries/query.rs b/crates/sqlquack-rules/src/conversion/_helpers/queries/query.rs index 9f31153c1..4adc5243c 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/queries/query.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/queries/query.rs @@ -607,7 +607,20 @@ fn convert_projection( let _rule = site.context.log.rule(COMMENT); let annotated = payload_object(payload, &construct)?; require_understood(annotated, &["this", "trailing_comments"], &construct)?; - convert_projection(field(annotated, "this", &construct)?, site, named) + let this = field(annotated, "this", &construct)?; + match node_kind(this) { + Some((kind, _)) if named && !matches!(kind, "star" | "alias" | "column") => { + // An unaliased item's name is its text, and no receipt records one holding a comment. + let refusal = missing( + Construct::new(ConstructFamily::Syntax, UNALIASED_OUTPUT_NAME_CONSTRUCT), + format!( + "Snowflake names this `{kind}` column after its text, comment included" + ), + ); + Err(site.context.locator.placed(refusal, this)) + } + _ => convert_projection(this, site, named), + } } Some(("alias", payload)) => { let _rule = site.context.log.rule(ALIAS); diff --git a/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs b/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs index b0d1ffb1b..77fb2e7db 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs @@ -203,24 +203,62 @@ pub(crate) fn session_tree(positioned: &Value, ignore_quoted_case: bool) -> Resu } /// `value` with each node that only wrapped a comment replaced by the node it wrapped. +/// +/// A select item's own wrapper stays: the comment is part of an unaliased item's written text, +/// which names its column, so the projection rule reads the wrapper itself. fn without_comment_wrappers(value: &Value) -> Value { + unwrap_comments(value, false) +} + +fn unwrap_comments(value: &Value, keep_wrapper: bool) -> Value { match value { Value::Object(fields) => match fields.get("annotated").and_then(Value::as_object) { - Some(annotated) if fields.len() == 1 && annotated.len() == 1 => annotated - .get("this") - .map_or_else(|| value.clone(), without_comment_wrappers), - _ => Value::Object( - fields - .iter() - .map(|(key, field)| (key.clone(), without_comment_wrappers(field))) - .collect(), - ), + Some(annotated) if !keep_wrapper && fields.len() == 1 && annotated.len() == 1 => { + annotated + .get("this") + .map_or_else(|| value.clone(), without_comment_wrappers) + } + _ => { + let select = fields.len() == 1 && fields.contains_key("select"); + Value::Object( + fields + .iter() + .map(|(key, field)| { + let field = match (select, field.as_object()) { + (true, Some(payload)) => Value::Object( + payload + .iter() + .map(|(name, clause)| { + (name.clone(), unwrap_clause(name, clause)) + }) + .collect(), + ), + _ => without_comment_wrappers(field), + }; + (key.clone(), field) + }) + .collect(), + ) + } }, Value::Array(items) => Value::Array(items.iter().map(without_comment_wrappers).collect()), other => other.clone(), } } +/// A SELECT clause with comment wrappers unwrapped, except around the select items themselves. +fn unwrap_clause(name: &str, clause: &Value) -> Value { + match (name, clause) { + ("expressions", Value::Array(items)) => Value::Array( + items + .iter() + .map(|item| unwrap_comments(item, true)) + .collect(), + ), + _ => without_comment_wrappers(clause), + } +} + /// Upper-cases the name of each quoted identifier under `value`. fn fold_quoted(value: &mut Value) -> Result<(), String> { match value { diff --git a/crates/sqlquack/src/analysis/tests/test_analyze.rs b/crates/sqlquack/src/analysis/tests/test_analyze.rs index 6f5c6dad8..74a28b0e9 100644 --- a/crates/sqlquack/src/analysis/tests/test_analyze.rs +++ b/crates/sqlquack/src/analysis/tests/test_analyze.rs @@ -113,6 +113,11 @@ fn given_sql_and_a_schema_when_analyzing_then_it_converts_or_refuses_with_a_cate sql: "SELECT ROUND(amount, 1 /* c */) AS v FROM orders", expected_summary: converted(), }, + AnalyzeTestCase { + description: "an unaliased literal carrying a comment has no recorded column name", + sql: "SELECT 'x' /* c */ FROM orders", + expected_summary: refused(RefusalCategory::MissingRule), + }, AnalyzeTestCase { description: "comments after select items and inside expressions are dropped", sql: "SELECT amount /* net */ + 1 AS gross, -- trailing\n customer_id -- last\n \ From 58731ecd0f60c403bcbf176b62c655b301777faf Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 6 Nov 2026 23:19:43 +0000 Subject: [PATCH 3/3] Split the comment unwrap into named steps for fensu (D327) No behaviour change: the select-item wrapper rule from the previous commit now lives in unwrap_field, and the inline comment and long doc comment go. --- .../src/conversion/_helpers/queries/query.rs | 1 - .../src/conversion/_helpers/support/tree.rs | 45 +++++++++---------- 2 files changed, 20 insertions(+), 26 deletions(-) diff --git a/crates/sqlquack-rules/src/conversion/_helpers/queries/query.rs b/crates/sqlquack-rules/src/conversion/_helpers/queries/query.rs index 4adc5243c..2723d2c36 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/queries/query.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/queries/query.rs @@ -610,7 +610,6 @@ fn convert_projection( let this = field(annotated, "this", &construct)?; match node_kind(this) { Some((kind, _)) if named && !matches!(kind, "star" | "alias" | "column") => { - // An unaliased item's name is its text, and no receipt records one holding a comment. let refusal = missing( Construct::new(ConstructFamily::Syntax, UNALIASED_OUTPUT_NAME_CONSTRUCT), format!( diff --git a/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs b/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs index 77fb2e7db..e9752cea2 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs @@ -203,13 +203,11 @@ pub(crate) fn session_tree(positioned: &Value, ignore_quoted_case: bool) -> Resu } /// `value` with each node that only wrapped a comment replaced by the node it wrapped. -/// -/// A select item's own wrapper stays: the comment is part of an unaliased item's written text, -/// which names its column, so the projection rule reads the wrapper itself. fn without_comment_wrappers(value: &Value) -> Value { unwrap_comments(value, false) } +/// `value` with its comment wrappers unwrapped, its own kept if `keep_wrapper`. fn unwrap_comments(value: &Value, keep_wrapper: bool) -> Value { match value { Value::Object(fields) => match fields.get("annotated").and_then(Value::as_object) { @@ -218,34 +216,31 @@ fn unwrap_comments(value: &Value, keep_wrapper: bool) -> Value { .get("this") .map_or_else(|| value.clone(), without_comment_wrappers) } - _ => { - let select = fields.len() == 1 && fields.contains_key("select"); - Value::Object( - fields - .iter() - .map(|(key, field)| { - let field = match (select, field.as_object()) { - (true, Some(payload)) => Value::Object( - payload - .iter() - .map(|(name, clause)| { - (name.clone(), unwrap_clause(name, clause)) - }) - .collect(), - ), - _ => without_comment_wrappers(field), - }; - (key.clone(), field) - }) - .collect(), - ) - } + _ => Value::Object( + fields + .iter() + .map(|(key, field)| (key.clone(), unwrap_field(key, field))) + .collect(), + ), }, Value::Array(items) => Value::Array(items.iter().map(without_comment_wrappers).collect()), other => other.clone(), } } +/// A node's field unwrapped; a SELECT keeps its select items' wrappers, which name their columns. +fn unwrap_field(key: &str, field: &Value) -> Value { + match (key, field) { + ("select", Value::Object(clauses)) => Value::Object( + clauses + .iter() + .map(|(name, clause)| (name.clone(), unwrap_clause(name, clause))) + .collect(), + ), + _ => without_comment_wrappers(field), + } +} + /// A SELECT clause with comment wrappers unwrapped, except around the select items themselves. fn unwrap_clause(name: &str, clause: &Value) -> Value { match (name, clause) {