diff --git a/corpora/polyglot-check/expected_differences.toml b/corpora/polyglot-check/expected_differences.toml index c24ae2cd7..c107bdab7 100644 --- a/corpora/polyglot-check/expected_differences.toml +++ b/corpora/polyglot-check/expected_differences.toml @@ -1320,6 +1320,31 @@ marker = "FILEFORMAT" sqlquack_node = "create_object" note = "a Snowflake CREATE FILE FORMAT parses into a FILE FORMAT object with its modifier and options; polyglot keeps it as raw text; crates/sqlquack-syntax/tests/parser/test_fixes.rs pins the tree" +# Calls polyglot keeps as functions where SQLQuack parses them into their own nodes. The check counts +# an entry only when the statement contains `marker` and the trees are equal once each listed node, +# holding only `expressions`, is written back as polyglot's call of `call` with those arguments. + +[[structured_call]] +id = "group_by_rollup_node" +marker = "ROLLUP(" +sqlquack_node = "rollup" +call = "ROLLUP" +note = "GROUP BY ROLLUP (...) is a grouping construct, not a function, so SQLQuack parses it into a rollup node that rules can match; crates/sqlquack-syntax/src/parser/tests/test_parser.rs pins the tree" + +[[structured_call]] +id = "group_by_cube_node" +marker = "CUBE(" +sqlquack_node = "cube" +call = "CUBE" +note = "GROUP BY CUBE (...) is a grouping construct, not a function, so SQLQuack parses it into a cube node that rules can match; crates/sqlquack-syntax/src/parser/tests/test_parser.rs pins the tree" + +[[structured_call]] +id = "group_by_grouping_sets_node" +marker = "GROUPINGSETS(" +sqlquack_node = "grouping_sets" +call = "GROUPING SETS" +note = "GROUP BY GROUPING SETS (...) is a grouping construct, not a function, so SQLQuack parses it into a grouping_sets node that rules can match; crates/sqlquack-syntax/src/parser/tests/test_parser.rs pins the tree" + [[rejection]] id = "snowflake_stage_reserved_name" sql = "SELECT @select" diff --git a/coverage/syntax/reviews/auditor-query.toml b/coverage/syntax/reviews/auditor-query.toml index 949465a1b..2090be814 100644 --- a/coverage/syntax/reviews/auditor-query.toml +++ b/coverage/syntax/reviews/auditor-query.toml @@ -244,3 +244,27 @@ note = "Read in full against the documentation page, the obligations, the cases "sf.query.pivot" = "0143158f5f51016a4e932fbde42fa1ab0786b9dbb4de7872980cb89c743174eb" "sf.query.with.select" = "19d0a58ffbccd906ccbd2e8249af5f8a8888210aaa8e4533a9eb663926af24d8" "sf.query.with.select.syntax" = "89bdc6f1c45a069f52f2d9b37fb295ba7a410535033e355d84472682cdb3583a" + +[[review]] +id = "auditor-query-011" +reviewer_lane = "auditor" +family = "query" +parser_tree = "eb83c300cc26fda9cb3d1c05a71441890754db24" +reviewed_utc = "2026-11-06T23:26:00Z" +verdict = "approved" +forms = [ + "sf.query.group-by-cube", + "sf.query.group-by-grouping-sets", + "sf.query.group-by-rollup", +] +note = "Re-read after D322 at 381ff30d5, which parses ROLLUP, CUBE and GROUPING SETS in GROUP BY as their own nodes instead of function calls. Definitions are unchanged; only the parse trees moved. Every obligation stays satisfied (55/55, 105/105, 55/55), nested and mixed lists keep their keys in order and GROUPING SETS keep the empty set, and conversion still refuses each node with SQ0002." + +[review.fingerprints] +"sf.query.group-by-cube" = "09ce7afcbbec7bf9b3f062ea5bc862886882fb90ef9db3eb4b2083071fee9683" +"sf.query.group-by-grouping-sets" = "ee103ed49142601fd101c1d2b7fb927579f189bf3dc520e35425b0e70eebc50b" +"sf.query.group-by-rollup" = "7fa47bdefde6d37bf4d4d2729e71bf478ae5a0fea1f1865585e01b0b03d511eb" + +[review.definitions] +"sf.query.group-by-cube" = "03f8aa5febed635b8e87af5eb3dd10f488541f4a63738fc1bf3aa51e092d63cc" +"sf.query.group-by-grouping-sets" = "c76492fc096009e540ab2a28806bad47e582d4af8cfa7e3f5c854cf8b1f60128" +"sf.query.group-by-rollup" = "a950177a2d84361481c1f0233e529a78469d24ab0baf26c3c25b6be9f956d1c8" diff --git a/coverage/translation/form-links.toml b/coverage/translation/form-links.toml index 2687cc10f..0923cfef5 100644 --- a/coverage/translation/form-links.toml +++ b/coverage/translation/form-links.toml @@ -655,7 +655,7 @@ forms = ["sf.expressions.bind-variables"] constructs = ["node:parameter", "spelling:positional_column"] forms = ["sf.query.select.select.selecting-specific-columns"] -# Casts to DECFLOAT, IPADDRESS, PERIOD and VECTOR, ROLLUP and CUBE in GROUP BY, the COLLATE operator, +# Casts to DECFLOAT, IPADDRESS, PERIOD and VECTOR, ROLLUP, CUBE and GROUPING SETS in GROUP BY, the COLLATE operator, # NTH_VALUE with its FROM and NULLS clauses, and unary plus. [[link]] constructs = ["cast:DECFLOAT"] @@ -674,13 +674,17 @@ constructs = ["cast:VECTOR"] forms = ["sf.expressions.data-types-vector"] [[link]] -constructs = ["fn:ROLLUP"] +constructs = ["node:rollup"] forms = ["sf.query.group-by-rollup"] [[link]] -constructs = ["fn:CUBE"] +constructs = ["node:cube"] forms = ["sf.query.group-by-cube"] +[[link]] +constructs = ["node:grouping_sets"] +forms = ["sf.query.group-by-grouping-sets"] + [[link]] constructs = ["node:collation"] forms = ["sf.functions.collate.collate.syntax"] diff --git a/crates/sqlquack-polyglot-check/src/expected/_helpers/nodes.rs b/crates/sqlquack-polyglot-check/src/expected/_helpers/nodes.rs index 6a5dc5ec1..4a0bd97df 100644 --- a/crates/sqlquack-polyglot-check/src/expected/_helpers/nodes.rs +++ b/crates/sqlquack-polyglot-check/src/expected/_helpers/nodes.rs @@ -1,4 +1,4 @@ -//! Trees that differ only by a wrapper node or a field SQLQuack keeps and polyglot drops or infers. +//! Trees that differ only by a wrapper node, a field or a call's node SQLQuack keeps and polyglot drops or infers. use serde_json::Value; @@ -166,3 +166,72 @@ fn all_as_key(mut fields: serde_json::Map) -> Value { } Value::Object(fields) } + +/// Whether both rendered parses are trees that are equal once SQLQuack's `(node, call)` nodes are polyglot's calls. +pub(crate) fn differ_only_by_structured_calls( + polyglot: &str, + sqlquack: &str, + calls: &[(&str, &str)], +) -> bool { + let (Some(theirs), Some(ours)) = ( + polyglot.strip_prefix(OK_PREFIX), + sqlquack.strip_prefix(OK_PREFIX), + ) else { + return false; + }; + match ( + serde_json::from_str::(theirs), + serde_json::from_str::(ours), + ) { + (Ok(theirs), Ok(ours)) => theirs != ours && as_calls(ours, calls) == theirs, + (Err(_), _) | (_, Err(_)) => false, + } +} + +/// `value` with every `{ node: { "expressions": arguments } }` replaced by polyglot's call of them. +fn as_calls(value: Value, calls: &[(&str, &str)]) -> Value { + match value { + Value::Object(object) => match called(&object, calls) { + Some((call, arguments)) => polyglot_call(call, as_calls(arguments, calls)), + None => Value::Object( + object + .into_iter() + .map(|(key, value)| (key, as_calls(value, calls))) + .collect(), + ), + }, + Value::Array(items) => Value::Array( + items + .into_iter() + .map(|value| as_calls(value, calls)) + .collect(), + ), + other => other, + } +} + +/// The call name and arguments of `object` if it is one listed node holding only `expressions`. +fn called<'c>( + object: &serde_json::Map, + calls: &[(&str, &'c str)], +) -> Option<(&'c str, Value)> { + let (node, inner) = object.iter().next().filter(|_| object.len() == 1)?; + let (_, call) = calls.iter().find(|(listed, _)| listed == node)?; + let inner = inner.as_object().filter(|inner| inner.len() == 1)?; + Some((call, inner.get(EXPRESSIONS_KEY)?.clone())) +} + +/// polyglot's tree of a plain call of `name` with `arguments`. +fn polyglot_call(name: &str, arguments: Value) -> Value { + serde_json::json!({ + "function": { + "name": name, + "args": arguments, + "distinct": false, + "trailing_comments": [], + "use_bracket_syntax": false, + "no_parens": false, + "quoted": false, + } + }) +} diff --git a/crates/sqlquack-polyglot-check/src/expected/main/load_expected.rs b/crates/sqlquack-polyglot-check/src/expected/main/load_expected.rs index c3e7ff19c..08c67780f 100644 --- a/crates/sqlquack-polyglot-check/src/expected/main/load_expected.rs +++ b/crates/sqlquack-polyglot-check/src/expected/main/load_expected.rs @@ -18,6 +18,7 @@ pub fn load_expected(path: &Path) -> Result { kept_fields: file.kept_fields, kept_alls: file.kept_alls, structured: file.structured, + structured_calls: file.structured_calls, ..ExpectedDifferences::default() }; for spelling in file.spellings { diff --git a/crates/sqlquack-polyglot-check/src/expected/models.rs b/crates/sqlquack-polyglot-check/src/expected/models.rs index 02ae6ebf0..8b158ab33 100644 --- a/crates/sqlquack-polyglot-check/src/expected/models.rs +++ b/crates/sqlquack-polyglot-check/src/expected/models.rs @@ -1,7 +1,8 @@ //! Expected differences. use crate::expected::_helpers::nodes::{ - differ_only_by_kept_all, differ_only_by_kept_field, differ_only_by_kept_node, is_opaque, + differ_only_by_kept_all, differ_only_by_kept_field, differ_only_by_kept_node, + differ_only_by_structured_calls, is_opaque, }; use crate::expected::_helpers::quoting::{compact, differ_only_by_kept_quoting}; use crate::expected::constants::{ERROR_PREFIX, OK_PREFIX}; @@ -37,6 +38,25 @@ pub struct ExpectedFile { /// One entry per reviewed statement polyglot keeps as a command and SQLQuack structures. #[serde(default, rename = "structured")] pub structured: Vec, + /// One entry per reviewed call polyglot keeps as a function and SQLQuack parses into a node. + #[serde(default, rename = "structured_call")] + pub structured_calls: Vec, +} + +/// A call polyglot keeps as a function, such as `ROLLUP(...)`, that SQLQuack parses into its own node. +#[derive(Clone, Debug, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct StructuredCall { + /// A lower-case identifier. + pub id: String, + /// Text the statement contains, with whitespace removed and ignoring case, such as `ROLLUP(`. + pub marker: String, + /// SQLQuack's node, such as `rollup`; its only field is `expressions`, polyglot's arguments. + pub sqlquack_node: String, + /// The function name polyglot gives the call, such as `ROLLUP`. + pub call: String, + /// Why SQLQuack's node is right; a test in sqlquack-syntax pins its tree. + pub note: String, } /// A statement polyglot keeps as an opaque command or raw SQL and SQLQuack parses into a structured node. @@ -168,6 +188,8 @@ pub struct ExpectedDifferences { pub kept_alls: Vec, /// The reviewed statements polyglot keeps as a command and SQLQuack structures. pub structured: Vec, + /// The reviewed calls polyglot keeps as functions and SQLQuack parses into nodes. + pub structured_calls: Vec, } impl ParserFix { @@ -261,6 +283,29 @@ impl ExpectedDifferences { }) } + /// The first entry explaining a tree that differs only by calls SQLQuack parses into nodes. + pub fn structured_call_for( + &self, + sql: &str, + polyglot: &str, + sqlquack: &str, + ) -> Option<&StructuredCall> { + let compact = compact(sql); + let present: Vec<&StructuredCall> = self + .structured_calls + .iter() + .filter(|entry| compact.contains(&entry.marker.to_ascii_uppercase())) + .collect(); + let calls: Vec<(&str, &str)> = present + .iter() + .map(|entry| (entry.sqlquack_node.as_str(), entry.call.as_str())) + .collect(); + present + .first() + .copied() + .filter(|_| differ_only_by_structured_calls(polyglot, sqlquack, &calls)) + } + /// Whether keeping `name` where polyglot makes `polyglot` of it is listed. pub fn allows(&self, name: &str, polyglot: &str) -> bool { self.allowed diff --git a/crates/sqlquack-polyglot-check/src/runs/main/run_corpus.rs b/crates/sqlquack-polyglot-check/src/runs/main/run_corpus.rs index 384fe3a78..fb1b63636 100644 --- a/crates/sqlquack-polyglot-check/src/runs/main/run_corpus.rs +++ b/crates/sqlquack-polyglot-check/src/runs/main/run_corpus.rs @@ -68,7 +68,14 @@ pub fn run_corpus(entries: &[CorpusEntry], expected: &ExpectedDifferences) -> Ru .kept_all_for(&entry.sql, &polyglot, &sqlquack) .map(|entry| entry.id.clone()) }); - let structured = expected.structured_for(&entry.sql, &polyglot, &sqlquack); + let structured = expected + .structured_for(&entry.sql, &polyglot, &sqlquack) + .map(|entry| entry.id.clone()) + .or_else(|| { + expected + .structured_call_for(&entry.sql, &polyglot, &sqlquack) + .map(|entry| entry.id.clone()) + }); match ( fix, rejection, @@ -94,7 +101,7 @@ pub fn run_corpus(entries: &[CorpusEntry], expected: &ExpectedDifferences) -> Ru *summary.kept_fields.entry(kept_field).or_default() += 1; } (None, None, None, None, None, Some(structured)) => { - *summary.structured.entry(structured.id.clone()).or_default() += 1; + *summary.structured.entry(structured).or_default() += 1; } (None, None, None, None, None, None) => summary.differences.push(Finding { from, diff --git a/crates/sqlquack-syntax/src/parser/_helpers/queries/clauses.rs b/crates/sqlquack-syntax/src/parser/_helpers/queries/clauses.rs index 1ce6c1bf5..7a540f4ba 100644 --- a/crates/sqlquack-syntax/src/parser/_helpers/queries/clauses.rs +++ b/crates/sqlquack-syntax/src/parser/_helpers/queries/clauses.rs @@ -1,6 +1,5 @@ //! Parser queries: clauses. -use crate::expressions::functions::models::Function; use crate::expressions::names::models::BooleanLiteral; use crate::expressions::names::models::Identifier; use crate::expressions::names::models::Null; @@ -8,6 +7,7 @@ use crate::expressions::operators::main::maybe_rebalance_boolean_chain::maybe_re use crate::expressions::operators::models::Alias; use crate::expressions::operators::models::BinaryOp; use crate::expressions::operators::models::Cube; +use crate::expressions::operators::models::GroupingSets; use crate::expressions::operators::models::ResultSerialization; use crate::expressions::operators::models::Rollup; use crate::expressions::operators::models::WithFill; @@ -120,58 +120,25 @@ impl Parser { self.expect_token(TokenType::LParen)?; let args = self.parse_grouping_sets_args()?; self.expect_token(TokenType::RParen)?; - Expression::Function(Box::new(Function { - on_error: None, - qualified_name: Vec::new(), - source_dialect: None, - - name: "GROUPING SETS".to_string(), - args, - distinct: false, - trailing_comments: Vec::new(), - use_bracket_syntax: false, - no_parens: false, - quoted: false, - span: SourceSpan::default(), - inferred_type: None, + Expression::GroupingSets(Box::new(GroupingSets { + options: None, + expressions: args, })) } else if self.match_token(TokenType::Cube) { self.expect_token(TokenType::LParen)?; let args = self.parse_expression_list()?; self.expect_token(TokenType::RParen)?; - Expression::Function(Box::new(Function { - on_error: None, - qualified_name: Vec::new(), - source_dialect: None, - - name: "CUBE".to_string(), - args, - distinct: false, - trailing_comments: Vec::new(), - use_bracket_syntax: false, - no_parens: false, - quoted: false, - span: SourceSpan::default(), - inferred_type: None, + Expression::Cube(Box::new(Cube { + options: None, + expressions: args, })) } else if self.match_token(TokenType::Rollup) { self.expect_token(TokenType::LParen)?; let args = self.parse_expression_list()?; self.expect_token(TokenType::RParen)?; - Expression::Function(Box::new(Function { - on_error: None, - qualified_name: Vec::new(), - source_dialect: None, - - name: "ROLLUP".to_string(), - args, - distinct: false, - trailing_comments: Vec::new(), - use_bracket_syntax: false, - no_parens: false, - quoted: false, - span: SourceSpan::default(), - inferred_type: None, + Expression::Rollup(Box::new(Rollup { + options: None, + expressions: args, })) } else { self.parse_expression()? @@ -255,58 +222,25 @@ impl Parser { self.expect_token(TokenType::LParen)?; let inner_args = self.parse_grouping_sets_args()?; self.expect_token(TokenType::RParen)?; - Expression::Function(Box::new(Function { - on_error: None, - qualified_name: Vec::new(), - source_dialect: None, - - name: "GROUPING SETS".to_string(), - args: inner_args, - distinct: false, - trailing_comments: Vec::new(), - use_bracket_syntax: false, - no_parens: false, - quoted: false, - span: SourceSpan::default(), - inferred_type: None, + Expression::GroupingSets(Box::new(GroupingSets { + options: None, + expressions: inner_args, })) } else if self.match_token(TokenType::Cube) { self.expect_token(TokenType::LParen)?; let inner_args = self.parse_expression_list()?; self.expect_token(TokenType::RParen)?; - Expression::Function(Box::new(Function { - on_error: None, - qualified_name: Vec::new(), - source_dialect: None, - - name: "CUBE".to_string(), - args: inner_args, - distinct: false, - trailing_comments: Vec::new(), - use_bracket_syntax: false, - no_parens: false, - quoted: false, - span: SourceSpan::default(), - inferred_type: None, + Expression::Cube(Box::new(Cube { + options: None, + expressions: inner_args, })) } else if self.match_token(TokenType::Rollup) { self.expect_token(TokenType::LParen)?; let inner_args = self.parse_expression_list()?; self.expect_token(TokenType::RParen)?; - Expression::Function(Box::new(Function { - on_error: None, - qualified_name: Vec::new(), - source_dialect: None, - - name: "ROLLUP".to_string(), - args: inner_args, - distinct: false, - trailing_comments: Vec::new(), - use_bracket_syntax: false, - no_parens: false, - quoted: false, - span: SourceSpan::default(), - inferred_type: None, + Expression::Rollup(Box::new(Rollup { + options: None, + expressions: inner_args, })) } else if self.check(TokenType::LParen) { self.skip(); diff --git a/crates/sqlquack-syntax/src/parser/tests/test_parser.rs b/crates/sqlquack-syntax/src/parser/tests/test_parser.rs index 04cfc5f2c..889a3bf10 100644 --- a/crates/sqlquack-syntax/src/parser/tests/test_parser.rs +++ b/crates/sqlquack-syntax/src/parser/tests/test_parser.rs @@ -555,6 +555,34 @@ fn given_ddl_and_pivots_when_parsing_then_each_statement_has_its_node() { pointer: "/0/select/group_by/expressions/2", expected_kind: Ok("rollup"), }, + ParseKindTestCase { + description: "a Snowflake ROLLUP list is a rollup, not a function call", + dialect: Some(DialectType::Snowflake), + sql: "SELECT a, b, SUM(c) FROM t GROUP BY ROLLUP (a, b)", + pointer: "/0/select/group_by/expressions/0", + expected_kind: Ok("rollup"), + }, + ParseKindTestCase { + description: "a Snowflake CUBE list is a cube, not a function call", + dialect: Some(DialectType::Snowflake), + sql: "SELECT a, b, SUM(c) FROM t GROUP BY CUBE (a, b)", + pointer: "/0/select/group_by/expressions/0", + expected_kind: Ok("cube"), + }, + ParseKindTestCase { + description: "Snowflake GROUPING SETS are grouping sets, not a function call", + dialect: Some(DialectType::Snowflake), + sql: "SELECT a, b, SUM(c) FROM t GROUP BY GROUPING SETS ((a), (b), ())", + pointer: "/0/select/group_by/expressions/0", + expected_kind: Ok("grouping_sets"), + }, + ParseKindTestCase { + description: "a ROLLUP nested in GROUPING SETS is a rollup", + dialect: Some(DialectType::Snowflake), + sql: "SELECT a, b FROM t GROUP BY GROUPING SETS (ROLLUP (a, b), (b))", + pointer: "/0/select/group_by/expressions/0/grouping_sets/expressions/0", + expected_kind: Ok("rollup"), + }, ]; for test_case in test_cases { @@ -1022,6 +1050,20 @@ fn given_statements_when_parsing_then_their_parts_are_kept() { pointer: "/0/select/group_by/expressions/2/rollup/expressions", expected_value: Ok(json!([])), }, + ParseValueTestCase { + description: "a Snowflake ROLLUP keeps its keys in order", + dialect: Some(DialectType::Snowflake), + sql: "SELECT a, b, SUM(c) FROM t GROUP BY ROLLUP (a, b)", + pointer: "/0/select/group_by/expressions/0/rollup/expressions/1/column/name/name", + expected_value: Ok(json!("b")), + }, + ParseValueTestCase { + description: "Snowflake GROUPING SETS keep the empty set", + dialect: Some(DialectType::Snowflake), + sql: "SELECT a, b, SUM(c) FROM t GROUP BY GROUPING SETS ((a), (b), ())", + pointer: "/0/select/group_by/expressions/0/grouping_sets/expressions/2/tuple/expressions", + expected_value: Ok(json!([])), + }, ]; for test_case in test_cases {