From b864b57367d3448a9d968305bf2724c683300ec2 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 11 Nov 2026 17:14:18 +0000 Subject: [PATCH 1/3] fix(check): refuse == as Snowflake does, with a new SQ0206 Snowflake parses == and then rejects it when compiling, as a call to an unknown function (errno 2139, recorded in the everyday corpus for WHERE status == 'shipped'). SQLQuack read it as = and converted it. A check finding now names it "Unknown function ==" with help to compare with =, and refuses the statement before conversion. Parsing still accepts it, as Snowflake does. --- corpora/everyday/queries.toml | 4 +- .../src/diagnostics/codes.lock | 1 + .../src/diagnostics/constants.rs | 13 ++++++ .../snowflake/refusals/_helpers/rejections.rs | 42 +++++++++++++++++++ .../refusals/main/snowflake_rejections.rs | 17 +++++++- .../snowflake/refusals/tests/test_refusals.rs | 23 ++++++++++ .../analysis/tests/test_everyday_corpus.rs | 1 - crates/sqlquack/tests/cli/test_check.rs | 8 ++++ crates/sqlquack/tests/cli/test_convert.rs | 7 ++++ .../docs/docs/diagnostics/reference.mdx | 15 +++++++ 10 files changed, 127 insertions(+), 4 deletions(-) diff --git a/corpora/everyday/queries.toml b/corpora/everyday/queries.toml index 7d754c888..326e8a279 100644 --- a/corpora/everyday/queries.toml +++ b/corpora/everyday/queries.toml @@ -2970,8 +2970,8 @@ expect_repository = "parse SQ0001 at 47..49" [[query]] category = "invalid_syntax" sql = '''SELECT order_id FROM orders WHERE status == 'shipped'''' -expect = "translated" -expect_repository = "translated" +expect = "check SQ0206 at 41..43" +expect_repository = "check SQ0206 at 41..43" snowflake = "rejects 2139" [[query]] diff --git a/crates/sqlquack-diagnostics/src/diagnostics/codes.lock b/crates/sqlquack-diagnostics/src/diagnostics/codes.lock index 2d0cfa435..1fd46aa60 100644 --- a/crates/sqlquack-diagnostics/src/diagnostics/codes.lock +++ b/crates/sqlquack-diagnostics/src/diagnostics/codes.lock @@ -22,3 +22,4 @@ SQ0203 qualify_without_window SQ0204 star_filter_drops_named_column SQ0211 unsupported_type_literal SQ0205 window_without_over +SQ0206 double_equals diff --git a/crates/sqlquack-diagnostics/src/diagnostics/constants.rs b/crates/sqlquack-diagnostics/src/diagnostics/constants.rs index 7706cf72d..98ec5709a 100644 --- a/crates/sqlquack-diagnostics/src/diagnostics/constants.rs +++ b/crates/sqlquack-diagnostics/src/diagnostics/constants.rs @@ -375,6 +375,19 @@ pub const CODES: &[CodeEntry] = &[ there. Add the OVER clause that says which rows the function ranks or reads.", example: r#"echo "SELECT CUME_DIST() AS d FROM orders" | sqlquack check"#, }, + CodeEntry { + code: "SQ0206", + name: "double_equals", + alias: None, + title: "Unknown function ==", + help: Some("compare with `=`"), + explanation: "Snowflake has no `==` operator. The statement parses, but Snowflake reads \ + `==` as a call to a function of that name and rejects the query when compiling it \ + (Snowflake error 2139, unknown function), so the statement never runs there. Other \ + dialects read `==` as `=`; SQLQuack does not, because that would convert SQL \ + Snowflake rejects. Write `=` instead.", + example: r#"echo "SELECT order_id FROM orders WHERE status == 'shipped'" | sqlquack check"#, + }, CodeEntry { code: "SQ0211", name: "unsupported_type_literal", diff --git a/crates/sqlquack-diagnostics/src/snowflake/refusals/_helpers/rejections.rs b/crates/sqlquack-diagnostics/src/snowflake/refusals/_helpers/rejections.rs index 8e02da82a..3070d02a7 100644 --- a/crates/sqlquack-diagnostics/src/snowflake/refusals/_helpers/rejections.rs +++ b/crates/sqlquack-diagnostics/src/snowflake/refusals/_helpers/rejections.rs @@ -6,11 +6,53 @@ use sqlquack_syntax::expressions::types::constants::{ SNOWFLAKE_UNPARAMETERIZED_LITERAL_TYPES, SNOWFLAKE_UNTYPED_LITERAL_TYPES, }; use sqlquack_syntax::expressions::types::models::DataType; +use sqlquack_syntax::tokens::main::node_location::node_location; +use sqlquack_syntax::tokens::models::SourceLines; use sqlquack_syntax::{Expression, ExpressionWalk}; /// Window functions Snowflake refuses without OVER (error 2064, missing window specification). const WINDOW_ONLY_CALLS: [&str; 3] = ["CUME_DIST", "PERCENT_RANK", "NTH_VALUE"]; +/// The equality operator of other dialects, which Snowflake reads as a call to an unknown function. +pub(crate) const DOUBLE_EQUALS: &str = "=="; + +/// The range of `==` in an equality (Snowflake error 2139), skipping unranged literals' quotes. +pub(crate) fn double_equals( + node: &Expression, + text: &[char], + lines: &SourceLines, +) -> Option<(usize, usize)> { + let Expression::Eq(operation) = node else { + return None; + }; + let whole = node_location(node, lines)?; + let from = node_location(&operation.left, lines).map_or(whole.start, |left| left.end); + let to = node_location(&operation.right, lines) + .map_or(whole.end, |right| right.start) + .min(text.len()); + let mut offset = from; + while offset < to { + let next = text.get(offset + 1).copied(); + offset = match (text[offset], next) { + ('=', Some('=')) => return Some((offset, offset + 2)), + ('=', _) => return None, + ('/', Some('*')) => after(text, offset + 2, &['*', '/']), + ('-', Some('-')) => after(text, offset + 2, &['\n']), + ('$', Some('$')) => after(text, offset + 2, &['$', '$']), + (quote @ ('\'' | '"'), _) => after(text, offset + 1, &[quote]), + _ => offset + 1, + }; + } + None +} + +/// The offset just past the first `close` at or after `from`, or the end of `text`. +fn after(text: &[char], from: usize, close: &[char]) -> usize { + (from..text.len()) + .find(|&offset| text[offset..].starts_with(close)) + .map_or(text.len(), |offset| offset + close.len()) +} + /// The rejection a node causes, if any, given the calls an OVER clause applies to (`windowed`). pub(crate) fn rejection( node: &Expression, diff --git a/crates/sqlquack-diagnostics/src/snowflake/refusals/main/snowflake_rejections.rs b/crates/sqlquack-diagnostics/src/snowflake/refusals/main/snowflake_rejections.rs index fdd028b35..73d52576d 100644 --- a/crates/sqlquack-diagnostics/src/snowflake/refusals/main/snowflake_rejections.rs +++ b/crates/sqlquack-diagnostics/src/snowflake/refusals/main/snowflake_rejections.rs @@ -7,7 +7,7 @@ use sqlquack_syntax::{Expression, ExpressionWalk}; use crate::classification::models::{Construct, ConstructFamily}; use crate::refusals::models::{Refusal, RefusalCategory}; use crate::snowflake::refusals::_helpers::clause_location::clause_location; -use crate::snowflake::refusals::_helpers::rejections::rejection; +use crate::snowflake::refusals::_helpers::rejections::{DOUBLE_EQUALS, double_equals, rejection}; use sqlquack_syntax::tokens::main::node_location::node_location; use sqlquack_syntax::tokens::models::SourceLines; @@ -24,9 +24,24 @@ pub fn snowflake_rejections( _ => None, }) .collect(); + let written_double = source.contains(DOUBLE_EQUALS); statement .dfs() .filter_map(|node| { + if written_double + && let Some((start, end)) = double_equals( + node, + characters.get_or_init(|| source.chars().collect()), + lines, + ) + { + return Some(Refusal { + construct: Some(Construct::new(ConstructFamily::Syntax, "double_equals")), + location: Some(lines.source_location(start, end)), + category: RefusalCategory::SnowflakeRejects, + reason: "Snowflake reads == as a call to an unknown function".to_owned(), + }); + } let (name, reason) = rejection(node, &windowed)?; let text = characters.get_or_init(|| source.chars().collect()); Some(Refusal { diff --git a/crates/sqlquack-diagnostics/src/snowflake/refusals/tests/test_refusals.rs b/crates/sqlquack-diagnostics/src/snowflake/refusals/tests/test_refusals.rs index d1d7790b5..a0b16c585 100644 --- a/crates/sqlquack-diagnostics/src/snowflake/refusals/tests/test_refusals.rs +++ b/crates/sqlquack-diagnostics/src/snowflake/refusals/tests/test_refusals.rs @@ -135,6 +135,29 @@ fn given_sql_snowflake_parses_then_rejects_when_refusing_then_each_rejection_is_ expected_rejections: vec!["window_without_over"], expected_codes: vec!["SQ0205"], }, + RejectionTestCase { + description: "== is rejected wherever it compares, after a comment or around parentheses", + sql: "SELECT order_id FROM orders WHERE status == 'shipped'; \ + SELECT 1 == 1 AS same; \ + SELECT 'a=b' == status AS same FROM orders; \ + SELECT (price) /* note */ == (2) AS two FROM orders", + expected_rejections: vec![ + "double_equals", + "double_equals", + "double_equals", + "double_equals", + ], + expected_codes: vec!["SQ0206", "SQ0206", "SQ0206", "SQ0206"], + }, + RejectionTestCase { + description: "a single =, == inside a string or a comment, and >= or != are left to conversion", + sql: "SELECT order_id FROM orders WHERE status = 'a == b'; \ + SELECT price /* == */ = 2 AS two FROM orders; \ + SELECT 'x==' = status AS same FROM orders; \ + SELECT order_id FROM orders WHERE price >= 2 AND price != 3", + expected_rejections: vec![], + expected_codes: vec![], + }, RejectionTestCase { description: "a star without ILIKE, or ILIKE without REPLACE or RENAME, is left to conversion", sql: "SELECT * REPLACE (price * 2 AS price) FROM orders; \ diff --git a/crates/sqlquack/src/analysis/tests/test_everyday_corpus.rs b/crates/sqlquack/src/analysis/tests/test_everyday_corpus.rs index 56e6fd707..19a5fd81c 100644 --- a/crates/sqlquack/src/analysis/tests/test_everyday_corpus.rs +++ b/crates/sqlquack/src/analysis/tests/test_everyday_corpus.rs @@ -16,7 +16,6 @@ fn given_everyday_snowflake_queries_when_checked_and_converted_then_none_regress expected_rejections_not_caught: vec![ "SELECT COUNT(*) FILTER (WHERE status = 'shipped') AS shipped FROM orders", "SELECT order_id, total FROM orders PIVOT (SUM(total) FOR status IN ('shipped', 'pending')) AS p", - "SELECT order_id FROM orders WHERE status == 'shipped'", "SELECT order_id FROM orders WHERE status IN 'shipped'", "SELECT order_id FROM orders WHERE total >> 10", "SELECT order_id FROM orders WHERE ordered_at > 5", diff --git a/crates/sqlquack/tests/cli/test_check.rs b/crates/sqlquack/tests/cli/test_check.rs index c3ee33f45..84bbf59ef 100644 --- a/crates/sqlquack/tests/cli/test_check.rs +++ b/crates/sqlquack/tests/cli/test_check.rs @@ -88,6 +88,14 @@ fn given_snowflake_sql_when_checked_then_problems_are_coded_and_the_exit_status_ expected_stdout_prefix: "", expected_stderr_mention: "error[SQ0211]", }, + CheckCommandTestCase { + description: "== is SQ0206, named as Snowflake does, and exits 1", + arguments: &["check"], + sql: "SELECT order_id FROM orders WHERE status == 'shipped'", + expected_exit_code: Some(1), + expected_stdout_prefix: "", + expected_stderr_mention: "error[SQ0206]: Unknown function ==", + }, CheckCommandTestCase { description: "a window function without OVER is SQ0205 and exits 1", arguments: &["check"], diff --git a/crates/sqlquack/tests/cli/test_convert.rs b/crates/sqlquack/tests/cli/test_convert.rs index a0b9e3582..7241b2e29 100644 --- a/crates/sqlquack/tests/cli/test_convert.rs +++ b/crates/sqlquack/tests/cli/test_convert.rs @@ -116,6 +116,13 @@ fn given_snowflake_sql_when_converting_then_duckdb_sql_or_named_refusals_are_pri mention: "done, converted", expected_outcome: convert_outcome(0, true), }, + ConvertTestCase { + description: "== is refused before conversion, as Snowflake rejects it", + sql: "WITH t AS (SELECT 1 AS order_id) SELECT order_id FROM t WHERE order_id == 1", + flags: &[], + mention: "error[SQ0206]: Unknown function ==", + expected_outcome: convert_outcome(1, false), + }, ConvertTestCase { description: "a statement that converts prints DuckDB SQL and exits 0", sql: "SELECT 1 + 2 AS total", diff --git a/website/src/content/docs/docs/diagnostics/reference.mdx b/website/src/content/docs/docs/diagnostics/reference.mdx index 6c66ec627..aad930114 100644 --- a/website/src/content/docs/docs/diagnostics/reference.mdx +++ b/website/src/content/docs/docs/diagnostics/reference.mdx @@ -34,6 +34,7 @@ renumbered. See [Reading diagnostics](/docs/diagnostics/overview/) for the outpu | [`SQ0203`](#sq0203) | `qualify_without_window` | QUALIFY needs a window function | | [`SQ0204`](#sq0204) | `star_filter_drops_named_column` | REPLACE or RENAME names a column the star's ILIKE pattern removed | | [`SQ0205`](#sq0205) | `window_without_over` | A window function needs an OVER clause | +| [`SQ0206`](#sq0206) | `double_equals` | Unknown function == | | [`SQ0211`](#sq0211) | `unsupported_type_literal` | Snowflake has no literal of this date-time type | ## Conversion @@ -338,6 +339,20 @@ CUME_DIST, PERCENT_RANK and NTH_VALUE are window functions only: Snowflake rejec echo "SELECT CUME_DIST() AS d FROM orders" | sqlquack check ``` +### SQ0206 + +**Unknown function ==** (`double_equals`) + +Snowflake has no `==` operator. The statement parses, but Snowflake reads `==` as a call to a function of that name and rejects the query when compiling it (Snowflake error 2139, unknown function), so the statement never runs there. Other dialects read `==` as `=`; SQLQuack does not, because that would convert SQL Snowflake rejects. Write `=` instead. + +**Help:** compare with `=` + +**Example:** + +```sh +echo "SELECT order_id FROM orders WHERE status == 'shipped'" | sqlquack check +``` + ### SQ0211 **Snowflake has no literal of this date-time type** (`unsupported_type_literal`) From 32359fc01b67b50f2e7b8ecc801da6a01a47ab79 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 11 Nov 2026 17:14:19 +0000 Subject: [PATCH 2/3] feat(convert): convert TOP and FETCH FIRST to LIMIT TOP n and FETCH { FIRST | NEXT } n { ROW | ROWS } [ ONLY ] keep the first n rows exactly as LIMIT n does, after ORDER BY and DISTINCT. The recorded top-n and limit.fetch cases now replay as matching. PERCENT, WITH TIES and a query that combines LIMIT, TOP or FETCH stay refused, since LIMIT cannot express them. --- corpora/everyday/queries.toml | 8 +- coverage/sql-size/baseline.json | 8 ++ coverage/translation/results-baseline.json | 34 ++++---- .../src/snowflake/catalogue/constants.rs | 4 +- .../conversion/_helpers/queries/clauses.rs | 63 ++++++++++++--- .../conversion/_helpers/queries/query.rs | 2 + .../src/snowflake/conversion/constants.rs | 2 +- crates/sqlquack/tests/cli.rs | 2 + crates/sqlquack/tests/cli/test_row_limits.rs | 78 +++++++++++++++++++ 9 files changed, 166 insertions(+), 35 deletions(-) create mode 100644 crates/sqlquack/tests/cli/test_row_limits.rs diff --git a/corpora/everyday/queries.toml b/corpora/everyday/queries.toml index 326e8a279..c117cc99e 100644 --- a/corpora/everyday/queries.toml +++ b/corpora/everyday/queries.toml @@ -318,8 +318,8 @@ snowflake = "rejects 1003" [[query]] category = "select_star" sql = '''SELECT TOP 5 * FROM orders''' -expect = "refused SQ0002 node:select at 0..26" -expect_repository = "refused SQ0002 node:select at 0..26" +expect = "translated" +expect_repository = "translated" [[query]] category = "select_star" @@ -553,8 +553,8 @@ expect_repository = "translated" [[query]] category = "columns" sql = '''SELECT order_id FROM orders FETCH FIRST 10 ROWS ONLY''' -expect = "refused SQ0002 node:select at 0..52" -expect_repository = "refused SQ0002 node:select at 0..52" +expect = "translated" +expect_repository = "translated" snowflake = "compiles" [[query]] diff --git a/coverage/sql-size/baseline.json b/coverage/sql-size/baseline.json index 3c26794db..63d9df5d6 100644 --- a/coverage/sql-size/baseline.json +++ b/coverage/sql-size/baseline.json @@ -9262,6 +9262,10 @@ "translation/query/join.join.toml#t004_null_key": 1487, "translation/query/join.join.toml#t005_self_join": 1082, "translation/query/join.join.toml#t006_null_key": 1102, + "translation/query/limit.fetch.toml#t001_offset_fetch": 887, + "translation/query/limit.fetch.toml#t002_first_row": 880, + "translation/query/limit.fetch.toml#t003_fetch_zero": 878, + "translation/query/limit.fetch.toml#t004_null_key": 927, "translation/query/limit.limit.toml#t001_offset": 887, "translation/query/limit.limit.toml#t002_zero": 878, "translation/query/operators-query.except.toml#t001_rows": 1206, @@ -9299,6 +9303,10 @@ "translation/query/select.select.toml#t003_exclude_two": 999, "translation/query/select.select.toml#t004_alias_star": 1535, "translation/query/select.select.toml#t005_null_values": 1039, + "translation/query/top-n.toml#t001_top": 927, + "translation/query/top-n.toml#t002_top_zero": 878, + "translation/query/top-n.toml#t003_top_distinct": 869, + "translation/query/top-n.toml#t004_null_key": 928, "translation/query/values.toml#t001_aliases": 1082, "translation/query/values.toml#t002_common_type": 1094, "translation/query/values.toml#t003_null_value": 1114, diff --git a/coverage/translation/results-baseline.json b/coverage/translation/results-baseline.json index f4d73234e..05983bf5f 100644 --- a/coverage/translation/results-baseline.json +++ b/coverage/translation/results-baseline.json @@ -12042,10 +12042,10 @@ "translation/query/join.join.toml#t004_null_key": "matching", "translation/query/join.join.toml#t005_self_join": "matching", "translation/query/join.join.toml#t006_null_key": "matching", - "translation/query/limit.fetch.toml#t001_offset_fetch": "refused", - "translation/query/limit.fetch.toml#t002_first_row": "refused", - "translation/query/limit.fetch.toml#t003_fetch_zero": "refused", - "translation/query/limit.fetch.toml#t004_null_key": "refused", + "translation/query/limit.fetch.toml#t001_offset_fetch": "matching", + "translation/query/limit.fetch.toml#t002_first_row": "matching", + "translation/query/limit.fetch.toml#t003_fetch_zero": "matching", + "translation/query/limit.fetch.toml#t004_null_key": "matching", "translation/query/limit.limit.toml#t001_offset": "matching", "translation/query/limit.limit.toml#t002_zero": "matching", "translation/query/limit.limit.toml#t003_null_count": "refused", @@ -12146,10 +12146,10 @@ "translation/query/select.select.toml#t017_ilike_replace_rename_null_values": "refused", "translation/query/select.select.toml#t018_replace_rename": "refused", "translation/query/select.select.toml#t019_replace_rename_null_values": "refused", - "translation/query/top-n.toml#t001_top": "refused", - "translation/query/top-n.toml#t002_top_zero": "refused", - "translation/query/top-n.toml#t003_top_distinct": "refused", - "translation/query/top-n.toml#t004_null_key": "refused", + "translation/query/top-n.toml#t001_top": "matching", + "translation/query/top-n.toml#t002_top_zero": "matching", + "translation/query/top-n.toml#t003_top_distinct": "matching", + "translation/query/top-n.toml#t004_null_key": "matching", "translation/query/unpivot.toml#t001_basic": "refused", "translation/query/unpivot.toml#t002_include_nulls": "refused", "translation/query/values.toml#t001_aliases": "matching", @@ -14890,7 +14890,7 @@ "sf.query.select.select": "refused", "sf.query.select.select.selecting-specific-columns": "matching", "sf.query.semantic-view": "unmeasured", - "sf.query.top-n": "refused", + "sf.query.top-n": "matching", "sf.query.unpivot": "refused", "sf.query.values": "refused", "sf.query.where": "refused", @@ -19031,10 +19031,10 @@ "sf.query.join.join.t004": "met", "sf.query.join.join.t005": "met", "sf.query.join.join.t006": "met", - "sf.query.limit.fetch.t001": "unmet", - "sf.query.limit.fetch.t002": "unmet", - "sf.query.limit.fetch.t003": "unmet", - "sf.query.limit.fetch.t004": "unmet", + "sf.query.limit.fetch.t001": "met", + "sf.query.limit.fetch.t002": "met", + "sf.query.limit.fetch.t003": "met", + "sf.query.limit.fetch.t004": "met", "sf.query.limit.limit.t001": "met", "sf.query.limit.limit.t002": "met", "sf.query.limit.limit.t003": "unmet", @@ -19144,10 +19144,10 @@ "sf.query.select.select.t017": "unmet", "sf.query.select.select.t018": "unmet", "sf.query.select.select.t019": "unmet", - "sf.query.top-n.t001": "unmet", - "sf.query.top-n.t002": "unmet", - "sf.query.top-n.t003": "unmet", - "sf.query.top-n.t004": "unmet", + "sf.query.top-n.t001": "met", + "sf.query.top-n.t002": "met", + "sf.query.top-n.t003": "met", + "sf.query.top-n.t004": "met", "sf.query.unpivot.t001": "unmet", "sf.query.unpivot.t002": "unmet", "sf.query.values.t001": "met", diff --git a/crates/sqlquack-convert/src/snowflake/catalogue/constants.rs b/crates/sqlquack-convert/src/snowflake/catalogue/constants.rs index 2a39dc41c..20c0874d0 100644 --- a/crates/sqlquack-convert/src/snowflake/catalogue/constants.rs +++ b/crates/sqlquack-convert/src/snowflake/catalogue/constants.rs @@ -178,8 +178,8 @@ pub const RULES: &[RuleSpec] = &[ }, RuleSpec { id: LIMIT, - constructs: &["clause:limit"], - conditions: "Integer literals.", + constructs: &["clause:limit", "clause:top", "clause:fetch"], + conditions: "Integer literals. TOP and FETCH { FIRST | NEXT } { ROW | ROWS } [ ONLY ] keep the first n rows as LIMIT does, after ORDER BY and DISTINCT, as recorded; with PERCENT or WITH TIES, or with more than one of LIMIT, TOP and FETCH, they are refused.", failures: &[], }, RuleSpec { diff --git a/crates/sqlquack-convert/src/snowflake/conversion/_helpers/queries/clauses.rs b/crates/sqlquack-convert/src/snowflake/conversion/_helpers/queries/clauses.rs index 3346c638f..9a6adbf57 100644 --- a/crates/sqlquack-convert/src/snowflake/conversion/_helpers/queries/clauses.rs +++ b/crates/sqlquack-convert/src/snowflake/conversion/_helpers/queries/clauses.rs @@ -29,6 +29,9 @@ use crate::snowflake::conversion::constants::{ TYPE_SUFFIX_UNREMOVED, VARIANT_HELD_UNREAD, }; +/// The clauses that keep a query's first rows, by field. +const ROW_LIMIT_CLAUSES: [&str; 3] = ["limit", "top", "fetch"]; + /// A clause's condition node, such as WHERE's. pub(crate) fn clause_condition<'a>(select: &'a Payload, clause: &str) -> Option<&'a Value> { select @@ -264,21 +267,59 @@ pub(crate) fn order_and_limit( } sql.push_str(&format!(" ORDER BY {}", items.join(", "))); } - for (clause, keyword) in [("limit", "LIMIT"), ("offset", "OFFSET")] { - if let Some(value) = query.get(clause).filter(|value| !value.is_null()) { - let _rule = site.context.log.rule(LIMIT); - let count = value.get("this").and_then(integer_literal).ok_or_else(|| { - outside_conditions( - node_construct(clause), - format!("{keyword} must be an integer literal"), - ) - })?; - sql.push_str(&format!(" {keyword} {count}")); - } + if let Some(count) = row_limit(query, site)? { + sql.push_str(&format!(" LIMIT {count}")); + } + if let Some(value) = query.get("offset").filter(|value| !value.is_null()) { + let _rule = site.context.log.rule(LIMIT); + let count = value.get("this").and_then(integer_literal).ok_or_else(|| { + outside_conditions( + node_construct("offset"), + "OFFSET must be an integer literal".to_owned(), + ) + })?; + sql.push_str(&format!(" OFFSET {count}")); } Ok(sql) } +/// The count of rows a query keeps from LIMIT, TOP or FETCH, which Snowflake reads alike, as recorded; None without one. +fn row_limit(query: &Payload, site: Site<'_>) -> Result, Refusal> { + let mut written = ROW_LIMIT_CLAUSES + .into_iter() + .filter(|clause| query.get(*clause).is_some_and(|value| !value.is_null())); + let Some(clause) = written.next() else { + return Ok(None); + }; + let construct = node_construct(clause); + if written.next().is_some() { + return Err(outside_conditions( + construct, + "more than one of LIMIT, TOP and FETCH, which no recording shows".to_owned(), + )); + } + let _rule = site.context.log.rule(LIMIT); + let keyword = clause.to_ascii_uppercase(); + let object = payload_object(&query[clause], &construct)?; + if flag(object, "percent") || flag(object, "with_ties") { + return Err(outside_conditions( + construct, + format!("{keyword} with PERCENT or WITH TIES, which LIMIT does not express"), + )); + } + let count_field = match clause { + "fetch" => "count", + _ => "this", + }; + let count = object + .get(count_field) + .and_then(integer_literal) + .ok_or_else(|| { + outside_conditions(construct, format!("{keyword} must be an integer literal")) + })?; + Ok(Some(count)) +} + /// Whether an ORDER BY only keeps the input fixture's rows in order, by its ordering column. fn fixture_ordering(order: &Value, site: Site<'_>) -> bool { let Some(fixture) = &site.context.options.fixture else { 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 14985124d..36fca4c10 100644 --- a/crates/sqlquack-convert/src/snowflake/conversion/_helpers/queries/query.rs +++ b/crates/sqlquack-convert/src/snowflake/conversion/_helpers/queries/query.rs @@ -75,6 +75,8 @@ const SELECT_FIELDS: &[&str] = &[ "qualify", "order_by", "limit", + "top", + "fetch", "offset", "distinct", "with", diff --git a/crates/sqlquack-convert/src/snowflake/conversion/constants.rs b/crates/sqlquack-convert/src/snowflake/conversion/constants.rs index 1fdb55c7c..d323ccd6f 100644 --- a/crates/sqlquack-convert/src/snowflake/conversion/constants.rs +++ b/crates/sqlquack-convert/src/snowflake/conversion/constants.rs @@ -553,7 +553,7 @@ pub const HAVING: &str = "rule.having"; pub const QUALIFY: &str = "rule.qualify"; /// ORDER BY with Snowflake's NULL placement. pub const ORDER_BY: &str = "rule.order_by"; -/// LIMIT and OFFSET. +/// LIMIT, TOP, FETCH and OFFSET. pub const LIMIT: &str = "rule.limit"; /// `*` and `t.*`. pub const STAR: &str = "rule.star"; diff --git a/crates/sqlquack/tests/cli.rs b/crates/sqlquack/tests/cli.rs index 04834d202..e0f7d85f3 100644 --- a/crates/sqlquack/tests/cli.rs +++ b/crates/sqlquack/tests/cli.rs @@ -32,6 +32,8 @@ mod test_project; mod test_read_check; #[path = "cli/test_review.rs"] mod test_review; +#[path = "cli/test_row_limits.rs"] +mod test_row_limits; #[path = "cli/test_run.rs"] mod test_run; #[path = "cli/test_types.rs"] diff --git a/crates/sqlquack/tests/cli/test_row_limits.rs b/crates/sqlquack/tests/cli/test_row_limits.rs new file mode 100644 index 000000000..02d69b7b6 --- /dev/null +++ b/crates/sqlquack/tests/cli/test_row_limits.rs @@ -0,0 +1,78 @@ +//! TOP and FETCH: the first rows they keep, and the forms LIMIT cannot express. + +use crate::helpers::{convert_outcome, converted_first_value, run_convert}; +use crate::test_types::{ConvertTestCase, ValueTestCase}; + +#[test] +fn given_top_or_fetch_that_limit_cannot_express_when_converting_then_it_is_refused() { + let test_cases = [ + ConvertTestCase { + description: "TOP with PERCENT refuses: LIMIT keeps a count, not a share", + sql: "WITH t AS (SELECT 1 AS order_id) SELECT TOP 10 PERCENT order_id FROM t", + flags: &[], + mention: "TOP with PERCENT or WITH TIES, which LIMIT does not express", + expected_outcome: convert_outcome(1, false), + }, + ConvertTestCase { + description: "FETCH WITH TIES refuses: LIMIT drops the tied rows", + sql: "WITH t AS (SELECT 1 AS order_id) \ + SELECT order_id FROM t ORDER BY order_id FETCH FIRST 2 ROWS WITH TIES", + flags: &[], + mention: "FETCH with PERCENT or WITH TIES, which LIMIT does not express", + expected_outcome: convert_outcome(1, false), + }, + ConvertTestCase { + description: "TOP together with LIMIT refuses: no recording shows both", + sql: "WITH t AS (SELECT 1 AS order_id) SELECT TOP 2 order_id FROM t LIMIT 3", + flags: &[], + mention: "more than one of LIMIT, TOP and FETCH", + expected_outcome: convert_outcome(1, false), + }, + ]; + for test_case in test_cases { + assert_eq!( + run_convert(test_case.sql, test_case.flags, test_case.mention), + test_case.expected_outcome, + "{}", + test_case.description + ); + } +} + +#[test] +fn given_top_or_fetch_when_converted_and_run_on_duckdb_then_the_first_rows_are_kept() { + let test_cases = [ + ValueTestCase { + description: "TOP keeps the first rows after ORDER BY", + sql: "WITH t AS (SELECT column1 AS a FROM VALUES ('a'), ('a'), ('b'), ('c')) \ + SELECT MAX(a) AS m FROM (SELECT TOP 3 a FROM t ORDER BY a)", + expected_value: "b", + }, + ValueTestCase { + description: "DISTINCT TOP counts distinct rows", + sql: "WITH t AS (SELECT column1 AS a FROM VALUES ('a'), ('a'), ('b'), ('c')) \ + SELECT MAX(a) AS m FROM (SELECT DISTINCT TOP 2 a FROM t ORDER BY a)", + expected_value: "b", + }, + ValueTestCase { + description: "OFFSET ... FETCH NEXT ... ROWS ONLY skips, then keeps", + sql: "WITH t AS (SELECT column1 AS a FROM VALUES ('a'), ('a'), ('b'), ('c')) \ + SELECT MIN(a) AS m FROM (SELECT a FROM t ORDER BY a OFFSET 2 ROWS FETCH NEXT 2 ROWS ONLY)", + expected_value: "b", + }, + ValueTestCase { + description: "FETCH FIRST 1 ROW without ONLY keeps one row", + sql: "WITH t AS (SELECT column1 AS a FROM VALUES ('a'), ('a'), ('b'), ('c')) \ + SELECT MAX(a) AS m FROM (SELECT a FROM t ORDER BY a FETCH FIRST 1 ROW)", + expected_value: "a", + }, + ]; + for test_case in test_cases { + assert_eq!( + converted_first_value(test_case.sql), + test_case.expected_value, + "{}", + test_case.description + ); + } +} From 1fd11a595755a8257c555a5b9b47b51ada2a99e0 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 11 Nov 2026 17:55:22 +0000 Subject: [PATCH 3/3] test(analysis): expect TOP to convert and == to be refused as Snowflake rejects it The analysis test still expected TOP to be refused as a missing rule. TOP now converts as LIMIT, TOP PERCENT is refused, and == is refused before conversion. --- .../src/analysis/tests/test_analyze.rs | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/crates/sqlquack/src/analysis/tests/test_analyze.rs b/crates/sqlquack/src/analysis/tests/test_analyze.rs index 9951010c1..a01a52684 100644 --- a/crates/sqlquack/src/analysis/tests/test_analyze.rs +++ b/crates/sqlquack/src/analysis/tests/test_analyze.rs @@ -209,6 +209,11 @@ fn given_sql_and_a_schema_when_analyzing_then_it_converts_or_refuses_with_a_cate converted: false, }, }, + AnalyzeTestCase { + description: "== is refused before conversion, as Snowflake rejects it", + sql: "SELECT amount FROM orders WHERE amount == 1", + expected_summary: refused(RefusalCategory::SnowflakeRejects), + }, AnalyzeTestCase { description: "a relation the schema does not type is an unresolved name", sql: "SELECT region FROM stores", @@ -240,14 +245,14 @@ fn given_sql_and_a_schema_when_analyzing_then_it_converts_or_refuses_with_a_cate }, }, AnalyzeTestCase { - description: "TOP, which no rule converts, is refused as a missing rule", - sql: "SELECT TOP 5 order_id FROM orders", - expected_summary: AnalysisSummary { - parsed: true, - statements: 1, - categories: vec![RefusalCategory::MissingRule], - converted: false, - }, + description: "TOP converts as LIMIT", + sql: "SELECT TOP 5 amount FROM orders ORDER BY amount", + expected_summary: converted(), + }, + AnalyzeTestCase { + description: "TOP with PERCENT, which LIMIT does not express, is refused", + sql: "SELECT TOP 10 PERCENT amount FROM orders", + expected_summary: refused(RefusalCategory::TypeCondition), }, AnalyzeTestCase { description: "unparseable SQL is one parse-fidelity refusal",