From e0bcf811447715239431290d919953aa63cd9c04 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sat, 31 Oct 2026 11:14:23 +0000 Subject: [PATCH 1/2] feat(rules): report queries that cannot do what they appear to Add eight report-only Rules on the parsed statements and tokens: duplicate CTE names (SQC046), CASE WHEN NULL and IN (NULL) (SQC047), literal-zero division (SQC048), unfiltered UPDATE or DELETE (SQC049, opt-in), INSERT without a column list (SQC050, opt-in), NATURAL JOIN (SQC051), reversed literal BETWEEN bounds (SQC052) and aggregates or windows in WHERE, ON or GROUP BY (SQC053). --- .../skill/references/docs/quickstart.md | 2 +- .../skill/references/docs/rules/overview.md | 2 +- .../skill/references/docs/rules/reference.md | 53 +- .../sqlclean/src/sql_lint/_helpers/engine.rs | 18 +- .../src/sql_quality/_helpers/safety.rs | 708 ++++++++++++++++++ crates/sqlclean/src/sql_quality/constants.rs | 40 + .../src/sql_quality/main/diagnostics.rs | 3 +- crates/sqlclean/src/sql_quality/mod.rs | 2 + crates/sqlclean/src/sql_quality/tests.rs | 2 + .../sql_quality/tests/test_safety_rules.rs | 193 +++++ .../src/sql_quality/tests/test_types.rs | 8 + website/src/content/docs/docs/quickstart.mdx | 2 +- .../src/content/docs/docs/rules/overview.mdx | 2 +- .../src/content/docs/docs/rules/reference.mdx | 52 +- 14 files changed, 1078 insertions(+), 9 deletions(-) create mode 100644 crates/sqlclean/src/sql_quality/_helpers/safety.rs create mode 100644 crates/sqlclean/src/sql_quality/tests/test_safety_rules.rs diff --git a/crates/sqlclean-cli/skill/references/docs/quickstart.md b/crates/sqlclean-cli/skill/references/docs/quickstart.md index a0714e2..d08c635 100644 --- a/crates/sqlclean-cli/skill/references/docs/quickstart.md +++ b/crates/sqlclean-cli/skill/references/docs/quickstart.md @@ -172,7 +172,7 @@ dialect = "duckdb" select = ["SQC"] ``` -`SQC` is a prefix, so it selects all 45 built-in Rules. Three of the opt-in structure Rules now +`SQC` is a prefix, so it selects all 53 built-in Rules. Three of the opt-in structure Rules now report the file: ```text diff --git a/crates/sqlclean-cli/skill/references/docs/rules/overview.md b/crates/sqlclean-cli/skill/references/docs/rules/overview.md index 5c13b1b..4794250 100644 --- a/crates/sqlclean-cli/skill/references/docs/rules/overview.md +++ b/crates/sqlclean-cli/skill/references/docs/rules/overview.md @@ -19,7 +19,7 @@ Online: https://sqlclean.io/docs/rules/overview/ - Continue A Rule is one check over SQL. It reports a finding at an exact position, with a message, a -remediation, and sometimes a fix. SQLClean has 45 built-in Rules with `SQC###` codes, and runs +remediation, and sometimes a fix. SQLClean has 53 built-in Rules with `SQC###` codes, and runs [custom Rules](../custom-rules/overview.md) written in Python with `XSQC` codes. Consider a query that returns an arbitrary hundred rows: diff --git a/crates/sqlclean-cli/skill/references/docs/rules/reference.md b/crates/sqlclean-cli/skill/references/docs/rules/reference.md index eff1381..2dec3ac 100644 --- a/crates/sqlclean-cli/skill/references/docs/rules/reference.md +++ b/crates/sqlclean-cli/skill/references/docs/rules/reference.md @@ -13,10 +13,11 @@ Online: https://sqlclean.io/docs/rules/reference/ - SQC005: unused CTEs - SQC018: unstable row numbers - SQC019: NULL in NOT IN +- SQC046 to SQC053: queries that cannot do what they say - SQC039: inline subqueries - SQC034, SQC035 and SQC041: query structure -SQLClean has 45 built-in Rules. `Default` Rules run when `[rules] select` is not set; the others +SQLClean has 53 built-in Rules. `Default` Rules run when `[rules] select` is not set; the others run when `select` names them by code or prefix. A Rule with a `Safe` fix is fixed by `sqlclean fix`; an `Unsafe` fix also needs `--unsafe-fixes`. See [Selecting Rules](overview.md#selecting-rules) and @@ -71,6 +72,14 @@ Run `sqlclean rules ` for a Rule's remediation. | `SQC043` | Ranking window sorts by more expressions than max_ranking_order_by | No | — | | `SQC044` | String literal is longer than max_literal_length | No | — | | `SQC045` | JSON payload is parsed more than once in one SELECT | No | — | +| `SQC046` | WITH defines the same CTE name more than once | Yes | — | +| `SQC047` | CASE WHEN NULL or IN (NULL) never matches | Yes | — | +| `SQC048` | Expression divides by a literal zero | Yes | — | +| `SQC049` | UPDATE or DELETE has no limiting predicate and changes every row | No | — | +| `SQC050` | INSERT does not list its target columns | No | — | +| `SQC051` | NATURAL JOIN matches on every column the relations share | Yes | — | +| `SQC052` | BETWEEN lower bound is greater than its upper bound | Yes | — | +| `SQC053` | Aggregate or window function is used in WHERE, ON or GROUP BY | Yes | — | `SQC999` is the [parse error](overview.md#parse-errors-sqc999) finding. It is not a selectable Rule. @@ -225,6 +234,48 @@ error[SQC019]: NOT IN list contains NULL and can reject every row = help: Remove NULL from the list or express the anti-join with a NULL-safe NOT EXISTS. ``` +## SQC046 to SQC053: queries that cannot do what they say + +These Rules report a query that runs differently from how it reads. None of them has a fix, +because only the author knows the intended query. + +```sql +WITH paid AS ( + SELECT order_id, amount FROM orders WHERE amount BETWEEN 100 AND 10 -- SQC052 +), + +paid AS ( -- SQC046 + SELECT order_id, amount / 0 AS ratio FROM orders -- SQC048 +) + +SELECT + customer_id, + CASE region WHEN NULL THEN 'unknown' END AS region_label -- SQC047 +FROM paid +NATURAL JOIN customers -- SQC051 +WHERE SUM(amount) > 100 -- SQC053 +GROUP BY customer_id +``` + +- `SQC046` compares CTE names within one `WITH` the way the dialect resolves them, so quoted + `"paid"` and `"PAID"` are different names in PostgreSQL but the same name in DuckDB. A nested + `WITH` may reuse an outer name. +- `SQC047` reports `CASE x WHEN NULL` and `x IN (NULL)`, which compare with `=` and so never + match. `NOT IN` lists with NULL are `SQC019`. +- `SQC048` reports `/`, `%`, `//` and `MOD` with a literal zero divisor. Depending on the dialect + the query fails or returns NULL. +- `SQC052` compares number and `DATE 'YYYY-MM-DD'` bounds exactly and skips `BETWEEN SYMMETRIC`. + A reversed `NOT BETWEEN` is reported too, because it matches every non-NULL value. +- `SQC053` reports aggregates in `WHERE`, `ON` and `GROUP BY`, and window functions anywhere in + those clauses. Aggregates in `HAVING`, windows in `QUALIFY`, `FILTER (WHERE ...)` and + subqueries pass. + +Two write-safety Rules are opt-in, because changing every row is sometimes intended: + +- `SQC049` reports an `UPDATE` or `DELETE` whose rows nothing limits. A `WHERE` without a column + or subquery, such as `WHERE TRUE`, does not count; a join condition or a `LIMIT` does. +- `SQC050` reports `INSERT` without a column list. `DEFAULT VALUES` and DuckDB's `BY NAME` pass. + ## SQC039: inline subqueries A subquery in `FROM` hides a step of the query. Name it as a CTE instead: diff --git a/crates/sqlclean/src/sql_lint/_helpers/engine.rs b/crates/sqlclean/src/sql_lint/_helpers/engine.rs index eb5212f..639a90b 100644 --- a/crates/sqlclean/src/sql_lint/_helpers/engine.rs +++ b/crates/sqlclean/src/sql_lint/_helpers/engine.rs @@ -238,7 +238,7 @@ const FINAL_CTE_NAME: LintRuleMetadata = LintRuleMetadata { remediation: "Name the last top-level CTE final and rename any other final CTE; SQC035 checks the terminal SELECT.", }; -pub(crate) const DEFAULT_RULES: [&str; 13] = [ +pub(crate) const DEFAULT_RULES: [&str; 19] = [ NULL_COMPARISON.code, IMPLICIT_CARTESIAN_JOIN.code, JOIN_WITHOUT_CONDITION.code, @@ -252,9 +252,15 @@ pub(crate) const DEFAULT_RULES: [&str; 13] = [ NULL_NOT_IN.code, SET_ARITY_MISMATCH.code, INLINE_QUERY_RELATION.code, + crate::sql_quality::constants::DUPLICATE_CTE_NAME.code, + crate::sql_quality::constants::NULL_MATCH.code, + crate::sql_quality::constants::LITERAL_ZERO_DIVISION.code, + crate::sql_quality::constants::NATURAL_JOIN.code, + crate::sql_quality::constants::REVERSED_BETWEEN.code, + crate::sql_quality::constants::MISPLACED_AGGREGATE.code, ]; -const ALL_RULE_METADATA: [&LintRuleMetadata; 45] = [ +const ALL_RULE_METADATA: [&LintRuleMetadata; 53] = [ &NULL_COMPARISON, &IMPLICIT_CARTESIAN_JOIN, &JOIN_WITHOUT_CONDITION, @@ -300,6 +306,14 @@ const ALL_RULE_METADATA: [&LintRuleMetadata; 45] = [ &crate::sql_quality::constants::RANKING_SORT_CAP, &crate::sql_quality::constants::LONG_LITERAL, &crate::sql_quality::constants::REPEATED_JSON_PARSE, + &crate::sql_quality::constants::DUPLICATE_CTE_NAME, + &crate::sql_quality::constants::NULL_MATCH, + &crate::sql_quality::constants::LITERAL_ZERO_DIVISION, + &crate::sql_quality::constants::UNFILTERED_WRITE, + &crate::sql_quality::constants::INSERT_WITHOUT_COLUMNS, + &crate::sql_quality::constants::NATURAL_JOIN, + &crate::sql_quality::constants::REVERSED_BETWEEN, + &crate::sql_quality::constants::MISPLACED_AGGREGATE, ]; fn is_ceremonial_cte_name(name: &str) -> bool { diff --git a/crates/sqlclean/src/sql_quality/_helpers/safety.rs b/crates/sqlclean/src/sql_quality/_helpers/safety.rs new file mode 100644 index 0000000..4e90292 --- /dev/null +++ b/crates/sqlclean/src/sql_quality/_helpers/safety.rs @@ -0,0 +1,708 @@ +//! SQC046 to SQC053: report-only safety Rules for queries that cannot do what they appear to. + +use std::borrow::Cow; +use std::collections::HashSet; + +use polyglot_sql::expressions::{Delete, Identifier, Insert, Join, Update, With}; +use polyglot_sql::tokens::{Token, TokenType}; +use polyglot_sql::traversal::is_aggregate; +use polyglot_sql::{DialectType, Expression, ExpressionWalk}; + +use crate::sql_lint::models::{LintDiagnostic, LintRuleMetadata}; +use crate::sql_quality::constants::{ + DUPLICATE_CTE_NAME, INSERT_WITHOUT_COLUMNS, LITERAL_ZERO_DIVISION, MISPLACED_AGGREGATE, + NATURAL_JOIN, NULL_MATCH, REVERSED_BETWEEN, UNFILTERED_WRITE, +}; +use crate::sql_quality::models::QualityRequest; +use crate::sql_quality::syntax::is_trivia; + +const RENAME_REFUSAL: &str = "Renaming a CTE requires choosing which definition its readers mean"; +const NULL_TEST_REFUSAL: &str = "Rewriting to IS NULL changes what the query returns"; +const DIVISOR_REFUSAL: &str = "Choosing a divisor or a NULL guard requires the intended result"; +const WRITE_SCOPE_REFUSAL: &str = "Only the author knows which rows the statement should change"; +const COLUMN_LIST_REFUSAL: &str = "Listing the target columns requires the table's schema"; +const JOIN_CONDITION_REFUSAL: &str = "Writing the join condition requires both relations' schemas"; +const BOUNDS_REFUSAL: &str = "Swapping the bounds changes what the query returns"; +const AGGREGATE_REFUSAL: &str = + "Moving the expression to HAVING, QUALIFY or an outer query requires restructuring the query"; +const ASYMMETRIC: &str = "ASYMMETRIC"; +const MODULO_FUNCTION: &str = "MOD"; +const SINGLE_ARGUMENT_AGGREGATES: [&str; 2] = ["MIN", "MAX"]; +const ISO_DATE_LENGTH: usize = 10; + +/// A finding's start and end character offsets. +type CharSpan = (usize, usize); + +/// Significant tokens with their parenthesis depth. +struct Significant<'a> { + tokens: Vec<&'a Token>, + depths: Vec, +} + +impl<'a> Significant<'a> { + fn new(tokens: &'a [Token]) -> Self { + let tokens: Vec<&Token> = tokens.iter().filter(|token| !is_trivia(token)).collect(); + let mut depths: Vec = Vec::with_capacity(tokens.len()); + let mut depth: usize = 0; + for token in &tokens { + if token.token_type == TokenType::RParen { + depth = depth.saturating_sub(1); + } + depths.push(depth); + if token.token_type == TokenType::LParen { + depth += 1; + } + } + Self { tokens, depths } + } + + fn kind(&self, index: usize) -> Option { + self.tokens.get(index).map(|token| token.token_type) + } + + fn is(&self, index: usize, kind: TokenType) -> bool { + self.kind(index) == Some(kind) + } + + fn text_is(&self, index: usize, text: &str) -> bool { + self.tokens + .get(index) + .is_some_and(|token| token.text.eq_ignore_ascii_case(text)) + } + + fn close(&self, open: usize) -> Option { + (open + 1..self.tokens.len()).find(|&index| { + self.is(index, TokenType::RParen) && self.depths[index] == self.depths[open] + }) + } + + fn span(&self, first: usize, last: usize) -> CharSpan { + (self.tokens[first].span.start, self.tokens[last].span.end) + } + + /// Top-level comma positions inside the parentheses that open at `open`. + fn arguments_commas(&self, open: usize, close: usize) -> Vec { + (open + 1..close) + .filter(|&index| { + self.is(index, TokenType::Comma) && self.depths[index] == self.depths[open] + 1 + }) + .collect() + } + + /// Non-empty statements split at top-level semicolons, as index ranges. + fn statements(&self) -> Vec<(usize, usize)> { + let mut ranges: Vec<(usize, usize)> = Vec::new(); + let mut start: usize = 0; + for index in 0..=self.tokens.len() { + let ends = index == self.tokens.len() + || (self.is(index, TokenType::Semicolon) && self.depths[index] == 0); + if ends { + if index > start { + ranges.push((start, index)); + } + start = index + 1; + } + } + ranges + } +} + +/// A literal bound whose order is exact: a number, or an ISO `DATE` literal. +#[derive(PartialEq, PartialOrd)] +enum Bound { + Number(f64), + Date(String), +} + +pub(crate) fn diagnostics(request: &QualityRequest<'_>) -> Vec { + let significant = Significant::new(request.tokens); + let enabled = |rule: &LintRuleMetadata| request.enabled.contains(rule.code); + let mut found: Vec = Vec::new(); + let mut report = + |rule: &'static LintRuleMetadata, reason: &'static str, spans: Vec| { + found.extend(spans.into_iter().map(|(start, end)| LintDiagnostic { + code: rule.code, + message: Cow::Borrowed(rule.message), + remediation: Cow::Borrowed(rule.remediation), + start, + end, + fix: None, + fix_unavailable_reason: Some(reason), + })); + }; + if enabled(&DUPLICATE_CTE_NAME) { + report( + &DUPLICATE_CTE_NAME, + RENAME_REFUSAL, + duplicate_cte_spans(request.statements, request.dialect), + ); + } + if enabled(&NULL_MATCH) { + report( + &NULL_MATCH, + NULL_TEST_REFUSAL, + null_match_spans(&significant), + ); + } + if enabled(&LITERAL_ZERO_DIVISION) { + report( + &LITERAL_ZERO_DIVISION, + DIVISOR_REFUSAL, + zero_division_spans(&significant), + ); + } + if enabled(&UNFILTERED_WRITE) || enabled(&INSERT_WITHOUT_COLUMNS) { + let (writes, inserts) = statement_spans(&significant, request.statements); + if enabled(&UNFILTERED_WRITE) { + report(&UNFILTERED_WRITE, WRITE_SCOPE_REFUSAL, writes); + } + if enabled(&INSERT_WITHOUT_COLUMNS) { + report(&INSERT_WITHOUT_COLUMNS, COLUMN_LIST_REFUSAL, inserts); + } + } + if enabled(&NATURAL_JOIN) { + report( + &NATURAL_JOIN, + JOIN_CONDITION_REFUSAL, + natural_join_spans(&significant), + ); + } + if enabled(&REVERSED_BETWEEN) { + report( + &REVERSED_BETWEEN, + BOUNDS_REFUSAL, + reversed_between_spans(&significant), + ); + } + if enabled(&MISPLACED_AGGREGATE) { + report( + &MISPLACED_AGGREGATE, + AGGREGATE_REFUSAL, + misplaced_aggregate_spans(&significant, &aggregate_names(request.statements)), + ); + } + found +} + +fn duplicate_cte_spans(statements: &[Expression], dialect: DialectType) -> Vec { + let mut spans: Vec = Vec::new(); + for statement in statements { + for with in statement.dfs().filter_map(with_clause) { + let mut seen: HashSet = HashSet::new(); + for cte in &with.ctes { + if !seen.insert(cte_key(&cte.alias, dialect)) + && let Some(span) = cte.alias.span + { + spans.push((span.start, span.end)); + } + } + } + } + spans +} + +fn with_clause(node: &Expression) -> Option<&With> { + match node { + Expression::Select(select) => select.with.as_ref(), + Expression::Union(operation) => operation.with.as_ref(), + Expression::Intersect(operation) => operation.with.as_ref(), + Expression::Except(operation) => operation.with.as_ref(), + Expression::Insert(insert) => insert.with.as_ref(), + Expression::Update(update) => update.with.as_ref(), + Expression::Delete(delete) => delete.with.as_ref(), + _ => None, + } +} + +/// A CTE name as the dialect resolves it, so only names the engine treats as equal collide. +fn cte_key(identifier: &Identifier, dialect: DialectType) -> String { + let quoted_names_ignore_case = matches!( + dialect, + DialectType::DuckDB + | DialectType::BigQuery + | DialectType::Databricks + | DialectType::Spark + | DialectType::Hive + | DialectType::SQLite + | DialectType::TSQL + | DialectType::Trino + | DialectType::Presto + | DialectType::Athena + ); + match dialect { + DialectType::ClickHouse => identifier.name.clone(), + _ if identifier.quoted && !quoted_names_ignore_case => identifier.name.clone(), + DialectType::Snowflake | DialectType::Oracle => identifier.name.to_uppercase(), + _ => identifier.name.to_lowercase(), + } +} + +/// `CASE x WHEN NULL` and a singleton positive `IN (NULL)`; SQC019 owns `NOT IN`. +fn null_match_spans(significant: &Significant<'_>) -> Vec { + let mut spans: Vec = Vec::new(); + for index in 0..significant.tokens.len() { + match significant.kind(index) { + Some(TokenType::In) + if !(index > 0 && significant.is(index - 1, TokenType::Not)) + && significant.is(index + 1, TokenType::LParen) + && significant.is(index + 2, TokenType::Null) + && significant.is(index + 3, TokenType::RParen) => + { + spans.push(significant.span(index, index + 3)); + } + Some(TokenType::Case) if !significant.is(index + 1, TokenType::When) => { + spans.extend(simple_case_null_spans(significant, index)); + } + _ => {} + } + } + spans +} + +fn simple_case_null_spans(significant: &Significant<'_>, case: usize) -> Vec { + let depth: usize = significant.depths[case]; + let mut nested: usize = 0; + let mut spans: Vec = Vec::new(); + for index in case + 1..significant.tokens.len() { + if significant.depths[index] < depth { + break; + } + if significant.depths[index] > depth { + continue; + } + match significant.kind(index) { + Some(TokenType::Case) => nested += 1, + Some(TokenType::End) if nested == 0 => break, + Some(TokenType::End) => nested -= 1, + Some(TokenType::When) + if nested == 0 + && significant.is(index + 1, TokenType::Null) + && significant.is(index + 2, TokenType::Then) => + { + spans.push(significant.span(index, index + 1)); + } + _ => {} + } + } + spans +} + +fn zero_division_spans(significant: &Significant<'_>) -> Vec { + let mut spans: Vec = Vec::new(); + for index in 0..significant.tokens.len() { + let operator = matches!( + significant.kind(index), + Some(TokenType::Slash | TokenType::Percent | TokenType::Div) + ); + if operator + && let Some((number, end)) = numeric_operand(significant, index + 1) + && is_zero(&significant.tokens[number].text) + && !binds_tighter(significant.kind(end + 1)) + { + spans.push(significant.span(index, end)); + } + if significant.text_is(index, MODULO_FUNCTION) + && significant.is(index + 1, TokenType::LParen) + && let Some(close) = significant.close(index + 1) + && let [comma] = significant.arguments_commas(index + 1, close)[..] + && let Some((number, end)) = numeric_operand(significant, comma + 1) + && is_zero(&significant.tokens[number].text) + && end + 1 == close + { + spans.push(significant.span(index, close)); + } + } + spans +} + +/// Operators that bind tighter than division, so a following one changes the divisor. +fn binds_tighter(kind: Option) -> bool { + matches!( + kind, + Some( + TokenType::DColon + | TokenType::DStar + | TokenType::Caret + | TokenType::LBracket + | TokenType::Dot + ) + ) +} + +/// Whether a number literal's digits are all zero, whatever its exponent. +fn is_zero(text: &str) -> bool { + let mantissa: &str = text.split(['e', 'E']).next().unwrap_or(text); + mantissa.chars().any(|character| character.is_ascii_digit()) + && mantissa + .chars() + .all(|character| matches!(character, '0' | '.' | '_')) +} + +/// An optionally signed and parenthesised number literal at `start`: its number token and last index. +fn numeric_operand(significant: &Significant<'_>, start: usize) -> Option<(usize, usize)> { + let mut index: usize = start; + let mut opens: usize = 0; + while significant.is(index, TokenType::LParen) { + opens += 1; + index += 1; + } + if matches!( + significant.kind(index), + Some(TokenType::Dash | TokenType::Plus) + ) { + index += 1; + } + if !significant.is(index, TokenType::Number) { + return None; + } + let mut end: usize = index; + for _ in 0..opens { + if !significant.is(end + 1, TokenType::RParen) { + return None; + } + end += 1; + } + Some((index, end)) +} + +/// A number or an ISO `DATE 'YYYY-MM-DD'` literal starting at `start`, and its last index. +fn bound(significant: &Significant<'_>, start: usize) -> Option<(Bound, usize)> { + if significant.is(start, TokenType::Date) && significant.is(start + 1, TokenType::String) { + let text: &str = &significant.tokens[start + 1].text; + let iso: bool = text.len() == ISO_DATE_LENGTH + && text + .char_indices() + .all(|(position, character)| match position { + 4 | 7 => character == '-', + _ => character.is_ascii_digit(), + }); + return iso.then(|| (Bound::Date(text.to_owned()), start + 1)); + } + let (number, end) = numeric_operand(significant, start)?; + let Ok(magnitude) = significant.tokens[number] + .text + .replace('_', "") + .parse::() + else { + return None; + }; + let negative: bool = significant.is(number - 1, TokenType::Dash); + Some(( + Bound::Number(if negative { -magnitude } else { magnitude }), + end, + )) +} + +fn reversed_between_spans(significant: &Significant<'_>) -> Vec { + let mut spans: Vec = Vec::new(); + for index in 0..significant.tokens.len() { + if !significant.is(index, TokenType::Between) { + continue; + } + let low_start: usize = index + 1 + usize::from(significant.text_is(index + 1, ASYMMETRIC)); + if let Some((low, low_end)) = bound(significant, low_start) + && significant.is(low_end + 1, TokenType::And) + && let Some((high, high_end)) = bound(significant, low_end + 2) + && ends_bound(significant.kind(high_end + 1)) + && matches!( + (&low, &high), + (Bound::Number(_), Bound::Number(_)) | (Bound::Date(_), Bound::Date(_)) + ) + && low > high + { + spans.push(significant.span(index, high_end)); + } + } + spans +} + +/// Tokens that bind looser than BETWEEN, so the upper bound ends before them. +fn ends_bound(kind: Option) -> bool { + match kind { + None => true, + Some(kind) => matches!( + kind, + TokenType::RParen + | TokenType::Comma + | TokenType::Semicolon + | TokenType::And + | TokenType::Or + | TokenType::Then + | TokenType::When + | TokenType::Else + | TokenType::End + | TokenType::As + | TokenType::From + | TokenType::Where + | TokenType::Group + | TokenType::Having + | TokenType::Qualify + | TokenType::Window + | TokenType::Order + | TokenType::Limit + | TokenType::Union + | TokenType::Except + | TokenType::Intersect + ), + } +} + +/// UPDATE or DELETE without a limiting predicate, and INSERT without a column list. +fn statement_spans( + significant: &Significant<'_>, + statements: &[Expression], +) -> (Vec, Vec) { + let mut writes: Vec = Vec::new(); + let mut inserts: Vec = Vec::new(); + let ranges: Vec<(usize, usize)> = significant.statements(); + if ranges.len() != statements.len() { + return (writes, inserts); + } + for (statement, (start, end)) in statements.iter().zip(ranges) { + let keyword: usize = (start..end) + .find(|&index| { + significant.depths[index] == 0 + && matches!( + significant.kind(index), + Some(TokenType::Update | TokenType::Delete | TokenType::Insert) + ) + }) + .unwrap_or(start); + let span = significant.span(keyword, keyword); + match statement { + Expression::Update(update) if !update_is_limited(update) => writes.push(span), + Expression::Delete(delete) if !delete_is_limited(delete) => writes.push(span), + Expression::Insert(insert) if lacks_column_list(insert) => inserts.push(span), + _ => {} + } + } + (writes, inserts) +} + +fn update_is_limited(update: &Update) -> bool { + update + .where_clause + .as_ref() + .is_some_and(|clause| reads_rows(&clause.this)) + || update.limit.is_some() + || update + .table_joins + .iter() + .chain(&update.from_joins) + .any(join_is_limiting) +} + +fn delete_is_limited(delete: &Delete) -> bool { + delete + .where_clause + .as_ref() + .is_some_and(|clause| reads_rows(&clause.this)) + || delete.limit.is_some() + || delete.joins.iter().any(join_is_limiting) +} + +/// Whether a predicate depends on the rows: it reads a column or a subquery, unlike `WHERE TRUE`. +fn reads_rows(predicate: &Expression) -> bool { + predicate.dfs().any(|node| { + matches!( + node, + Expression::Column(_) | Expression::Select(_) | Expression::Subquery(_) + ) + }) +} + +fn join_is_limiting(join: &Join) -> bool { + join.on.as_ref().is_some_and(reads_rows) || !join.using.is_empty() +} + +fn lacks_column_list(insert: &Insert) -> bool { + insert.columns.is_empty() + && !insert.default_values + && !insert.by_name + && insert.directory.is_none() + && (insert.query.is_some() || !insert.values.is_empty()) +} + +fn natural_join_spans(significant: &Significant<'_>) -> Vec { + let mut spans: Vec = Vec::new(); + for index in 0..significant.tokens.len() { + if !significant.is(index, TokenType::Natural) { + continue; + } + let mut join: usize = index + 1; + while matches!( + significant.kind(join), + Some( + TokenType::Left + | TokenType::Right + | TokenType::Full + | TokenType::Inner + | TokenType::Outer + ) + ) { + join += 1; + } + if significant.is(join, TokenType::Join) { + spans.push(significant.span(index, join)); + } + } + spans +} + +/// Function names the parser classified as aggregates, upper-cased without underscores. +fn aggregate_names(statements: &[Expression]) -> HashSet { + statements + .iter() + .flat_map(|statement| statement.dfs()) + .filter(|node| is_aggregate(node)) + .map(|node| match node { + Expression::AggregateFunction(function) => normalized_name(&function.name), + other => normalized_name(other.variant_name()), + }) + .collect() +} + +fn normalized_name(name: &str) -> String { + name.chars() + .filter(|character| *character != '_') + .flat_map(char::to_uppercase) + .collect() +} + +/// Aggregate and window calls directly in WHERE, ON or GROUP BY, outside nested queries. +fn misplaced_aggregate_spans( + significant: &Significant<'_>, + aggregates: &HashSet, +) -> Vec { + let mut spans: Vec = Vec::new(); + for index in 0..significant.tokens.len() { + let first: usize = match significant.kind(index) { + Some(TokenType::Where | TokenType::On) => index + 1, + Some(TokenType::Group) if significant.is(index + 1, TokenType::By) => index + 2, + _ => continue, + }; + spans.extend(clause_call_spans( + significant, + (first, significant.depths[index]), + significant.is(index, TokenType::On), + aggregates, + )); + } + spans +} + +fn clause_call_spans( + significant: &Significant<'_>, + (first, depth): (usize, usize), + comma_ends: bool, + aggregates: &HashSet, +) -> Vec { + let mut spans: Vec = Vec::new(); + let mut index: usize = first; + while index < significant.tokens.len() { + if significant.depths[index] < depth + || (significant.depths[index] == depth && ends_clause(significant, index, comma_ends)) + { + break; + } + if significant.is(index, TokenType::LParen) + && matches!( + significant.kind(index + 1), + Some(TokenType::Select | TokenType::With | TokenType::Values) + ) + { + index = significant + .close(index) + .map_or(significant.tokens.len(), |close| close + 1); + continue; + } + if let Some((close, end)) = call(significant, index) { + if significant.is(close + 1, TokenType::Over) { + spans.push(significant.span(index, end)); + index = end + 1; + continue; + } + if is_aggregate_call(significant, index, close, aggregates) { + spans.push(significant.span(index, close)); + index = close + 1; + continue; + } + } + index += 1; + } + spans +} + +/// A function call at `name`: its closing parenthesis and the end of any OVER specification. +fn call(significant: &Significant<'_>, name: usize) -> Option<(usize, usize)> { + let text: &str = &significant.tokens[name].text; + let is_name: bool = text + .chars() + .next() + .is_some_and(|character| character.is_ascii_alphabetic() || character == '_') + && text + .chars() + .all(|character| character.is_ascii_alphanumeric() || character == '_'); + if !is_name + || !significant.is(name + 1, TokenType::LParen) + || (name > 0 && significant.is(name - 1, TokenType::Dot)) + { + return None; + } + let close: usize = significant.close(name + 1)?; + let end: usize = if significant.is(close + 1, TokenType::Over) { + if significant.is(close + 2, TokenType::LParen) { + significant.close(close + 2)? + } else { + close + 2 + } + } else { + close + }; + (end < significant.tokens.len()).then_some((close, end)) +} + +fn is_aggregate_call( + significant: &Significant<'_>, + name: usize, + close: usize, + aggregates: &HashSet, +) -> bool { + let normalized: String = normalized_name(&significant.tokens[name].text); + aggregates.contains(&normalized) + && !(SINGLE_ARGUMENT_AGGREGATES.contains(&normalized.as_str()) + && !significant.arguments_commas(name + 1, close).is_empty()) +} + +/// Whether `index` ends the clause; only an ON condition ends at a comma, before the next relation. +fn ends_clause(significant: &Significant<'_>, index: usize, comma_ends: bool) -> bool { + match significant.kind(index) { + Some(TokenType::Group) => significant.is(index + 1, TokenType::By), + Some(TokenType::Comma) => comma_ends, + Some(kind) => matches!( + kind, + TokenType::Where + | TokenType::Having + | TokenType::Qualify + | TokenType::Window + | TokenType::Order + | TokenType::Limit + | TokenType::Union + | TokenType::Except + | TokenType::Intersect + | TokenType::Returning + | TokenType::Fetch + | TokenType::Offset + | TokenType::Semicolon + | TokenType::Join + | TokenType::Inner + | TokenType::Left + | TokenType::Right + | TokenType::Full + | TokenType::Cross + | TokenType::Natural + | TokenType::Set + ), + None => true, + } +} diff --git a/crates/sqlclean/src/sql_quality/constants.rs b/crates/sqlclean/src/sql_quality/constants.rs index 25ae410..2adffd4 100644 --- a/crates/sqlclean/src/sql_quality/constants.rs +++ b/crates/sqlclean/src/sql_quality/constants.rs @@ -20,6 +20,46 @@ pub(crate) const REPEATED_JSON_PARSE: LintRuleMetadata = LintRuleMetadata { message: "JSON payload is parsed more than once in one SELECT", remediation: "Parse the payload once in the CTE that reads it, for example `TRY_PARSE_JSON(value) AS payload`, and read every field from that column in a later step.", }; +pub(crate) const DUPLICATE_CTE_NAME: LintRuleMetadata = LintRuleMetadata { + code: "SQC046", + message: "WITH defines the same CTE name more than once", + remediation: "Give each CTE in one WITH clause its own name; engines reject the duplicate or silently read only one definition.", +}; +pub(crate) const NULL_MATCH: LintRuleMetadata = LintRuleMetadata { + code: "SQC047", + message: "CASE WHEN NULL or IN (NULL) never matches", + remediation: "A simple CASE compares with =, and x IN (NULL) means x = NULL, so neither is ever true. Test the value with IS NULL, for example CASE WHEN x IS NULL THEN ... END.", +}; +pub(crate) const LITERAL_ZERO_DIVISION: LintRuleMetadata = LintRuleMetadata { + code: "SQC048", + message: "Expression divides by a literal zero", + remediation: "Depending on the dialect this raises an error or returns NULL. Remove the division, or divide by NULLIF(divisor, 0) when the divisor can legitimately be zero.", +}; +pub(crate) const UNFILTERED_WRITE: LintRuleMetadata = LintRuleMetadata { + code: "SQC049", + message: "UPDATE or DELETE has no limiting predicate and changes every row", + remediation: "Add a WHERE condition that reads the target rows; a constant such as WHERE TRUE does not limit them. Use TRUNCATE, or an exception for this file, when changing every row is intended.", +}; +pub(crate) const INSERT_WITHOUT_COLUMNS: LintRuleMetadata = LintRuleMetadata { + code: "SQC050", + message: "INSERT does not list its target columns", + remediation: "List the target columns, for example INSERT INTO orders (order_id, status) SELECT ..., so values cannot shift into the wrong column when the table changes.", +}; +pub(crate) const NATURAL_JOIN: LintRuleMetadata = LintRuleMetadata { + code: "SQC051", + message: "NATURAL JOIN matches on every column the relations share", + remediation: "Join with an explicit ON or USING condition; adding a column with a shared name to either relation silently changes a NATURAL JOIN.", +}; +pub(crate) const REVERSED_BETWEEN: LintRuleMetadata = LintRuleMetadata { + code: "SQC052", + message: "BETWEEN lower bound is greater than its upper bound", + remediation: "BETWEEN low AND high is empty when low > high, so it never matches, and NOT BETWEEN matches every non-NULL value. Swap the bounds, or write BETWEEN SYMMETRIC where the dialect supports it.", +}; +pub(crate) const MISPLACED_AGGREGATE: LintRuleMetadata = LintRuleMetadata { + code: "SQC053", + message: "Aggregate or window function is used in WHERE, ON or GROUP BY", + remediation: "Rows are filtered and grouped before aggregates and windows are computed. Filter an aggregate in HAVING, filter a window result in QUALIFY or an outer query, and group by the underlying columns.", +}; pub(crate) const UNSTABLE_ROW_NUMBER_CODE: &str = "SQC018"; pub(crate) const UNPROVEN_RANKING_MESSAGE: &str = "Ranking window order is not proven unique within its partition"; diff --git a/crates/sqlclean/src/sql_quality/main/diagnostics.rs b/crates/sqlclean/src/sql_quality/main/diagnostics.rs index 76848cc..0dc8266 100644 --- a/crates/sqlclean/src/sql_quality/main/diagnostics.rs +++ b/crates/sqlclean/src/sql_quality/main/diagnostics.rs @@ -5,7 +5,7 @@ use crate::sql_quality::constants::{ }; use crate::sql_quality::keys::{KeyEnvironment, key_environment}; use crate::sql_quality::models::QualityRequest; -use crate::sql_quality::{cte_columns, json_parses, literals, ranking}; +use crate::sql_quality::{cte_columns, json_parses, literals, ranking, safety}; /// Run every enabled SQL quality Rule over one parsed body. pub(crate) fn diagnostics(request: &QualityRequest<'_>) -> Result, String> { @@ -19,6 +19,7 @@ pub(crate) fn diagnostics(request: &QualityRequest<'_>) -> Result Result<(), String> { + let test_cases = [ + SafetyRuleTestCase { + description: "unquoted CTE names collide regardless of case", + dialect: "duckdb", + sql: "WITH paid AS (SELECT 1 AS x), PAID AS (SELECT 2 AS x) SELECT x FROM paid", + rule: "SQC046", + expected_anchors: &["PAID"], + }, + SafetyRuleTestCase { + description: "quoted names that differ in case are distinct in PostgreSQL", + dialect: "postgres", + sql: r#"WITH "paid" AS (SELECT 1 AS x), "PAID" AS (SELECT 2 AS x) SELECT x FROM "paid""#, + rule: "SQC046", + expected_anchors: &[], + }, + SafetyRuleTestCase { + description: "a quoted name equals the folded unquoted name in Snowflake", + dialect: "snowflake", + sql: r#"WITH "PAID" AS (SELECT 1 AS x), paid AS (SELECT 2 AS x) SELECT x FROM paid"#, + rule: "SQC046", + expected_anchors: &["paid"], + }, + SafetyRuleTestCase { + description: "a nested WITH may reuse an outer CTE name", + dialect: "duckdb", + sql: "WITH paid AS (WITH paid AS (SELECT 1 AS x) SELECT x FROM paid) SELECT x FROM paid", + rule: "SQC046", + expected_anchors: &[], + }, + SafetyRuleTestCase { + description: "simple CASE compares with NULL, and a singleton IN list holds only NULL", + dialect: "duckdb", + sql: "SELECT CASE status WHEN 'open' THEN 1 WHEN NULL THEN 2 END AS s FROM orders WHERE region IN (NULL)", + rule: "SQC047", + expected_anchors: &["WHEN NULL", "IN (NULL)"], + }, + SafetyRuleTestCase { + description: "searched CASE, IS NULL, NOT IN and a longer IN list pass", + dialect: "duckdb", + sql: "SELECT CASE WHEN status IS NULL THEN 1 END AS s FROM orders WHERE region NOT IN (NULL) AND kind IN (1, NULL)", + rule: "SQC047", + expected_anchors: &[], + }, + SafetyRuleTestCase { + description: "only the CASE whose operand compares with NULL is reported", + dialect: "duckdb", + sql: "SELECT CASE status WHEN 'open' THEN CASE WHEN NULL THEN 1 END END AS a, CASE WHEN amount > 0 THEN CASE region WHEN NULL THEN 2 END END AS b FROM orders", + rule: "SQC047", + expected_anchors: &["WHEN NULL"], + }, + SafetyRuleTestCase { + description: "division and modulo by literal zero in operator and function form", + dialect: "duckdb", + sql: "SELECT amount / 0 AS a, amount % 0.0 AS b, MOD(amount, 0) AS c, amount / (-0) AS d FROM orders", + rule: "SQC048", + expected_anchors: &["/ 0", "% 0.0", "MOD(amount, 0)", "/ (-0)"], + }, + SafetyRuleTestCase { + description: "a non-zero or computed divisor passes", + dialect: "duckdb", + sql: "SELECT amount / 0.5 AS a, amount / 1e-400 AS e, amount / 0 ** 2 AS b, MOD(amount, 0 + 1) AS c, amount / NULLIF(quantity, 0) AS d FROM orders", + rule: "SQC048", + expected_anchors: &[], + }, + SafetyRuleTestCase { + description: "a missing or constant WHERE does not limit an UPDATE or DELETE", + dialect: "duckdb", + sql: "UPDATE orders SET status = 'closed'; DELETE FROM orders WHERE TRUE; DELETE FROM orders WHERE 1 = 1", + rule: "SQC049", + expected_anchors: &["UPDATE", "DELETE", "DELETE"], + }, + SafetyRuleTestCase { + description: "a row predicate or a subquery limits the write", + dialect: "duckdb", + sql: "UPDATE orders SET status = 'closed' WHERE closed_at IS NOT NULL; DELETE FROM orders WHERE EXISTS (SELECT 1 FROM refunds)", + rule: "SQC049", + expected_anchors: &[], + }, + SafetyRuleTestCase { + description: "a joined DELETE and a LIMIT bound the rows in MySQL", + dialect: "mysql", + sql: "DELETE o FROM orders o JOIN customers c ON c.id = o.customer_id; DELETE FROM orders LIMIT 10", + rule: "SQC049", + expected_anchors: &[], + }, + SafetyRuleTestCase { + description: "UPDATE FROM without WHERE, and DELETE after a WITH clause", + dialect: "postgres", + sql: "UPDATE orders SET status = 'closed' FROM customers; WITH stale AS (SELECT 1 AS id) DELETE FROM orders", + rule: "SQC049", + expected_anchors: &["UPDATE", "DELETE"], + }, + SafetyRuleTestCase { + description: "an INSERT without a column list in query and VALUES form", + dialect: "duckdb", + sql: "INSERT INTO orders SELECT * FROM staged_orders; INSERT INTO orders VALUES (1, 'open')", + rule: "SQC050", + expected_anchors: &["INSERT", "INSERT"], + }, + SafetyRuleTestCase { + description: "listed columns and BY NAME pass", + dialect: "duckdb", + sql: "INSERT INTO orders (order_id) SELECT order_id FROM staged_orders; INSERT INTO orders BY NAME SELECT 1 AS order_id", + rule: "SQC050", + expected_anchors: &[], + }, + SafetyRuleTestCase { + description: "DEFAULT VALUES needs no column list", + dialect: "postgres", + sql: "INSERT INTO orders DEFAULT VALUES", + rule: "SQC050", + expected_anchors: &[], + }, + SafetyRuleTestCase { + description: "NATURAL joins are reported with their join keywords", + dialect: "duckdb", + sql: "SELECT * FROM orders NATURAL JOIN customers NATURAL LEFT OUTER JOIN regions JOIN items USING (order_id)", + rule: "SQC051", + expected_anchors: &["NATURAL JOIN", "NATURAL LEFT OUTER JOIN"], + }, + SafetyRuleTestCase { + description: "reversed number, negative, decimal and date bounds, with or without NOT", + dialect: "duckdb", + sql: "SELECT * FROM orders WHERE a BETWEEN 10 AND 1 OR b NOT BETWEEN -1 AND -5 OR c BETWEEN 1.5 AND 1.25 OR d BETWEEN DATE '2026-02-01' AND DATE '2026-01-31'", + rule: "SQC052", + expected_anchors: &[ + "BETWEEN 10 AND 1", + "BETWEEN -1 AND -5", + "BETWEEN 1.5 AND 1.25", + "BETWEEN DATE '2026-02-01' AND DATE '2026-01-31'", + ], + }, + SafetyRuleTestCase { + description: "ASYMMETRIC keeps the ordered meaning in PostgreSQL", + dialect: "postgres", + sql: "SELECT * FROM orders WHERE a BETWEEN ASYMMETRIC 10 AND 1", + rule: "SQC052", + expected_anchors: &["BETWEEN ASYMMETRIC 10 AND 1"], + }, + SafetyRuleTestCase { + description: "ordered, equal, SYMMETRIC, computed, text and frame bounds pass", + dialect: "duckdb", + sql: "SELECT SUM(a) OVER (ORDER BY b ROWS BETWEEN 2 PRECEDING AND CURRENT ROW) AS s FROM orders WHERE a BETWEEN 9 AND 10 OR b BETWEEN -5 AND -1 OR c BETWEEN 5 AND 5 OR d BETWEEN SYMMETRIC 10 AND 1 OR e BETWEEN 10 AND 1 + 20 OR f BETWEEN 'b' AND 'a'", + rule: "SQC052", + expected_anchors: &[], + }, + SafetyRuleTestCase { + description: "aggregates and windows in WHERE, ON and after a GROUP BY comma", + dialect: "duckdb", + sql: "SELECT o.customer_id FROM orders AS o JOIN customers AS c ON c.id = o.customer_id AND COUNT(*) > 1 WHERE SUM(o.amount) > 100 AND ROW_NUMBER() OVER (PARTITION BY o.customer_id ORDER BY o.ordered_at) = 1 GROUP BY o.customer_id, MAX(o.amount)", + rule: "SQC053", + expected_anchors: &[ + "COUNT(*)", + "SUM(o.amount)", + "ROW_NUMBER() OVER (PARTITION BY o.customer_id ORDER BY o.ordered_at)", + "MAX(o.amount)", + ], + }, + SafetyRuleTestCase { + description: "HAVING, QUALIFY, FILTER, subqueries and unknown functions pass", + dialect: "duckdb", + sql: "SELECT customer_id, COUNT(*) FILTER (WHERE amount > 0) AS paid FROM orders WHERE amount > (SELECT AVG(amount) FROM orders) AND order_total(amount) > 0 GROUP BY customer_id HAVING COUNT(*) > 1 QUALIFY ROW_NUMBER() OVER (ORDER BY customer_id) = 1", + rule: "SQC053", + expected_anchors: &[], + }, + SafetyRuleTestCase { + description: "multi-argument MAX is the scalar function in SQLite", + dialect: "sqlite", + sql: "SELECT order_id FROM orders WHERE max(amount, refunded) > 100", + rule: "SQC053", + expected_anchors: &[], + }, + ]; + for test_case in &test_cases { + let diagnostics: Vec = + lint_dialect(test_case.sql, test_case.dialect, test_case.rule)?; + assert_eq!( + anchors(test_case.sql, &diagnostics), + test_case.expected_anchors, + "{}", + test_case.description + ); + } + Ok(()) +} diff --git a/crates/sqlclean/src/sql_quality/tests/test_types.rs b/crates/sqlclean/src/sql_quality/tests/test_types.rs index bc80b42..6d79a36 100644 --- a/crates/sqlclean/src/sql_quality/tests/test_types.rs +++ b/crates/sqlclean/src/sql_quality/tests/test_types.rs @@ -36,3 +36,11 @@ pub(crate) struct JsonParseScaleTestCase { pub(crate) expected_findings: usize, pub(crate) expected_max_seconds: f64, } + +pub(crate) struct SafetyRuleTestCase { + pub(crate) description: &'static str, + pub(crate) dialect: &'static str, + pub(crate) sql: &'static str, + pub(crate) rule: &'static str, + pub(crate) expected_anchors: &'static [&'static str], +} diff --git a/website/src/content/docs/docs/quickstart.mdx b/website/src/content/docs/docs/quickstart.mdx index 1702e59..973920c 100644 --- a/website/src/content/docs/docs/quickstart.mdx +++ b/website/src/content/docs/docs/quickstart.mdx @@ -160,7 +160,7 @@ dialect = "duckdb" select = ["SQC"] ``` -`SQC` is a prefix, so it selects all 45 built-in Rules. Three of the opt-in structure Rules now +`SQC` is a prefix, so it selects all 53 built-in Rules. Three of the opt-in structure Rules now report the file: ```text diff --git a/website/src/content/docs/docs/rules/overview.mdx b/website/src/content/docs/docs/rules/overview.mdx index c5b3905..5dd2b7e 100644 --- a/website/src/content/docs/docs/rules/overview.mdx +++ b/website/src/content/docs/docs/rules/overview.mdx @@ -7,7 +7,7 @@ import Card from '../../../../components/docs/Card.astro'; import CardGroup from '../../../../components/docs/CardGroup.astro'; A Rule is one check over SQL. It reports a finding at an exact position, with a message, a -remediation, and sometimes a fix. SQLClean has 45 built-in Rules with `SQC###` codes, and runs +remediation, and sometimes a fix. SQLClean has 53 built-in Rules with `SQC###` codes, and runs [custom Rules](/docs/custom-rules/overview/) written in Python with `XSQC` codes. Consider a query that returns an arbitrary hundred rows: diff --git a/website/src/content/docs/docs/rules/reference.mdx b/website/src/content/docs/docs/rules/reference.mdx index 6c7eae4..103091c 100644 --- a/website/src/content/docs/docs/rules/reference.mdx +++ b/website/src/content/docs/docs/rules/reference.mdx @@ -3,7 +3,7 @@ title: "Rule reference" description: "Every built-in Rule, whether it runs by default, and whether it has a fix." --- -SQLClean has 45 built-in Rules. `Default` Rules run when `[rules] select` is not set; the others +SQLClean has 53 built-in Rules. `Default` Rules run when `[rules] select` is not set; the others run when `select` names them by code or prefix. A Rule with a `Safe` fix is fixed by `sqlclean fix`; an `Unsafe` fix also needs `--unsafe-fixes`. See [Selecting Rules](/docs/rules/overview/#selecting-rules) and @@ -58,6 +58,14 @@ Run `sqlclean rules ` for a Rule's remediation. | `SQC043` | Ranking window sorts by more expressions than max_ranking_order_by | No | — | | `SQC044` | String literal is longer than max_literal_length | No | — | | `SQC045` | JSON payload is parsed more than once in one SELECT | No | — | +| `SQC046` | WITH defines the same CTE name more than once | Yes | — | +| `SQC047` | CASE WHEN NULL or IN (NULL) never matches | Yes | — | +| `SQC048` | Expression divides by a literal zero | Yes | — | +| `SQC049` | UPDATE or DELETE has no limiting predicate and changes every row | No | — | +| `SQC050` | INSERT does not list its target columns | No | — | +| `SQC051` | NATURAL JOIN matches on every column the relations share | Yes | — | +| `SQC052` | BETWEEN lower bound is greater than its upper bound | Yes | — | +| `SQC053` | Aggregate or window function is used in WHERE, ON or GROUP BY | Yes | — | `SQC999` is the [parse error](/docs/rules/overview/#parse-errors-sqc999) finding. It is not a selectable Rule. @@ -212,6 +220,48 @@ error[SQC019]: NOT IN list contains NULL and can reject every row = help: Remove NULL from the list or express the anti-join with a NULL-safe NOT EXISTS. ``` +## SQC046 to SQC053: queries that cannot do what they say + +These Rules report a query that runs differently from how it reads. None of them has a fix, +because only the author knows the intended query. + +```sql +WITH paid AS ( + SELECT order_id, amount FROM orders WHERE amount BETWEEN 100 AND 10 -- SQC052 +), + +paid AS ( -- SQC046 + SELECT order_id, amount / 0 AS ratio FROM orders -- SQC048 +) + +SELECT + customer_id, + CASE region WHEN NULL THEN 'unknown' END AS region_label -- SQC047 +FROM paid +NATURAL JOIN customers -- SQC051 +WHERE SUM(amount) > 100 -- SQC053 +GROUP BY customer_id +``` + +- `SQC046` compares CTE names within one `WITH` the way the dialect resolves them, so quoted + `"paid"` and `"PAID"` are different names in PostgreSQL but the same name in DuckDB. A nested + `WITH` may reuse an outer name. +- `SQC047` reports `CASE x WHEN NULL` and `x IN (NULL)`, which compare with `=` and so never + match. `NOT IN` lists with NULL are `SQC019`. +- `SQC048` reports `/`, `%`, `//` and `MOD` with a literal zero divisor. Depending on the dialect + the query fails or returns NULL. +- `SQC052` compares number and `DATE 'YYYY-MM-DD'` bounds exactly and skips `BETWEEN SYMMETRIC`. + A reversed `NOT BETWEEN` is reported too, because it matches every non-NULL value. +- `SQC053` reports aggregates in `WHERE`, `ON` and `GROUP BY`, and window functions anywhere in + those clauses. Aggregates in `HAVING`, windows in `QUALIFY`, `FILTER (WHERE ...)` and + subqueries pass. + +Two write-safety Rules are opt-in, because changing every row is sometimes intended: + +- `SQC049` reports an `UPDATE` or `DELETE` whose rows nothing limits. A `WHERE` without a column + or subquery, such as `WHERE TRUE`, does not count; a join condition or a `LIMIT` does. +- `SQC050` reports `INSERT` without a column list. `DEFAULT VALUES` and DuckDB's `BY NAME` pass. + ## SQC039: inline subqueries A subquery in `FROM` hides a step of the query. Name it as a CTE instead: From c3d2c658ebf17e273904195bd8018b75b93a4fa3 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sat, 31 Oct 2026 11:27:24 +0000 Subject: [PATCH 2/2] fix(rules): simplify the single-argument aggregate test for clippy --- crates/sqlclean/src/sql_quality/_helpers/safety.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/sqlclean/src/sql_quality/_helpers/safety.rs b/crates/sqlclean/src/sql_quality/_helpers/safety.rs index 4e90292..8a26587 100644 --- a/crates/sqlclean/src/sql_quality/_helpers/safety.rs +++ b/crates/sqlclean/src/sql_quality/_helpers/safety.rs @@ -670,8 +670,8 @@ fn is_aggregate_call( ) -> bool { let normalized: String = normalized_name(&significant.tokens[name].text); aggregates.contains(&normalized) - && !(SINGLE_ARGUMENT_AGGREGATES.contains(&normalized.as_str()) - && !significant.arguments_commas(name + 1, close).is_empty()) + && (!SINGLE_ARGUMENT_AGGREGATES.contains(&normalized.as_str()) + || significant.arguments_commas(name + 1, close).is_empty()) } /// Whether `index` ends the clause; only an ON condition ends at a comma, before the next relation.