diff --git a/crates/sqlclean-cli/skill/references/docs/rules/reference.md b/crates/sqlclean-cli/skill/references/docs/rules/reference.md index b9b6a19..3f03d68 100644 --- a/crates/sqlclean-cli/skill/references/docs/rules/reference.md +++ b/crates/sqlclean-cli/skill/references/docs/rules/reference.md @@ -213,7 +213,8 @@ FROM customer_orders ``` `SQLCLEAN004` reports the same problem for `LIMIT` and `OFFSET`; see the example in -[Rules](overview.md). +[Rules](overview.md). It skips a query that selects only constants, such as +`SELECT 1 AS present FROM orders LIMIT 1`, because every row it could return is identical. ## SQLCLEAN019: NULL in NOT IN diff --git a/crates/sqlclean/src/sql_lint/_helpers/engine.rs b/crates/sqlclean/src/sql_lint/_helpers/engine.rs index 4d28192..93b4225 100644 --- a/crates/sqlclean/src/sql_lint/_helpers/engine.rs +++ b/crates/sqlclean/src/sql_lint/_helpers/engine.rs @@ -17,7 +17,10 @@ use crate::sql_lint::_helpers::join_predicates::{ condition_tokens, cross_join_diagnostics, trivial_on_condition, }; use crate::sql_lint::_helpers::parsing::parse; -use crate::sql_lint::_helpers::terminal_shape::collect_terminal_shape_facts; +use crate::sql_lint::_helpers::terminal_shape::{ + collect_terminal_shape_facts, first_row_selection_clause, limits_to_no_rows, + projects_only_constants, +}; const NULL_COMPARISON: LintRuleMetadata = LintRuleMetadata { code: "SQLCLEAN001", @@ -539,6 +542,7 @@ fn collect_token_query_facts( if order.is_none() && let Some(index) = limit && !limits_to_no_rows(tokens, index) + && !projects_only_constants(tokens, &direct, index) { facts.unordered_limits.push(tokens[index].span); } @@ -704,33 +708,6 @@ fn first_type(tokens: &[Token], indices: &[usize], token_type: TokenType) -> Opt .find(|&index| tokens[index].token_type == token_type) } -fn first_row_selection_clause(tokens: &[Token], direct: &[usize]) -> Option { - let from = direct - .iter() - .copied() - .find(|&index| is_query_from(tokens, index)); - direct.iter().copied().find(|&index| { - matches!( - tokens[index].token_type, - TokenType::Limit | TokenType::Offset - ) && from.is_none_or(|from_index| index > from_index) - && significant_before(tokens, index).is_none_or(|previous| { - !matches!( - tokens[previous].token_type, - TokenType::Colon | TokenType::DColon | TokenType::Dot | TokenType::DotColon - ) - }) - }) -} - -/// Whether a row-selection clause is `LIMIT 0`, which selects no rows in any order. -fn limits_to_no_rows(tokens: &[Token], index: usize) -> bool { - tokens[index].token_type == TokenType::Limit - && tokens.get(index + 1).is_some_and(|count| { - count.token_type == TokenType::Number && count.text.parse::() == Ok(0) - }) -} - pub(super) fn is_query_from(tokens: &[Token], index: usize) -> bool { tokens[index].token_type == TokenType::From && significant_before(tokens, index) diff --git a/crates/sqlclean/src/sql_lint/_helpers/terminal_shape.rs b/crates/sqlclean/src/sql_lint/_helpers/terminal_shape.rs index 73637fa..3c377af 100644 --- a/crates/sqlclean/src/sql_lint/_helpers/terminal_shape.rs +++ b/crates/sqlclean/src/sql_lint/_helpers/terminal_shape.rs @@ -1,5 +1,7 @@ use polyglot_sql::tokens::{Span, Token, TokenType}; +use crate::sql_lint::_helpers::engine::{is_query_from, significant_before}; + const CEREMONIAL_SELECT_LITERAL: &str = "1"; pub(super) fn final_cte_name_spans(tokens: &[Token], depths: &[usize]) -> Vec { @@ -301,3 +303,71 @@ fn is_terminal_from(tokens: &[Token], significant: &[usize], index: usize) -> bo .and_then(|position| position.checked_sub(1)) .is_none_or(|position| tokens[significant[position]].token_type != TokenType::Distinct) } + +pub(super) fn first_row_selection_clause(tokens: &[Token], direct: &[usize]) -> Option { + let from = direct + .iter() + .copied() + .find(|&index| is_query_from(tokens, index)); + direct.iter().copied().find(|&index| { + matches!( + tokens[index].token_type, + TokenType::Limit | TokenType::Offset + ) && from.is_none_or(|from_index| index > from_index) + && significant_before(tokens, index).is_none_or(|previous| { + !matches!( + tokens[previous].token_type, + TokenType::Colon | TokenType::DColon | TokenType::Dot | TokenType::DotColon + ) + }) + }) +} + +/// Whether a row-selection clause is `LIMIT 0`, which selects no rows in any order. +pub(super) fn limits_to_no_rows(tokens: &[Token], index: usize) -> bool { + tokens[index].token_type == TokenType::Limit + && tokens.get(index + 1).is_some_and(|count| { + count.token_type == TokenType::Number && count.text.parse::() == Ok(0) + }) +} + +/// Whether the projection before `limit` holds only constants, so every row it could select is identical. +pub(super) fn projects_only_constants(tokens: &[Token], direct: &[usize], limit: usize) -> bool { + let Some(&first) = direct.first() else { + return false; + }; + let end: usize = direct + .iter() + .copied() + .find(|&index| is_query_from(tokens, index)) + .unwrap_or(limit); + (first..end).all(|index| match tokens[index].token_type { + TokenType::Var | TokenType::Identifier | TokenType::QuotedIdentifier => { + names_output_or_type(tokens, index) + } + TokenType::Star | TokenType::Select => false, + _ => true, + }) +} + +/// Whether the name at `index` is an alias or a type after `AS`, or a bare alias after a value. +fn names_output_or_type(tokens: &[Token], index: usize) -> bool { + let Some(previous) = index.checked_sub(1).map(|previous| &tokens[previous]) else { + return false; + }; + let followed_by_list_end: bool = tokens + .get(index + 1) + .is_none_or(|next| matches!(next.token_type, TokenType::Comma | TokenType::From)); + previous.token_type == TokenType::As + || (followed_by_list_end + && matches!( + previous.token_type, + TokenType::Number + | TokenType::String + | TokenType::Null + | TokenType::True + | TokenType::False + | TokenType::Parameter + | TokenType::RParen + )) +} diff --git a/crates/sqlclean/src/sql_lint/tests/test_lint_exemptions.rs b/crates/sqlclean/src/sql_lint/tests/test_lint_exemptions.rs index 94768c3..c872c27 100644 --- a/crates/sqlclean/src/sql_lint/tests/test_lint_exemptions.rs +++ b/crates/sqlclean/src/sql_lint/tests/test_lint_exemptions.rs @@ -1,8 +1,9 @@ use serde_json::{Value, json}; use crate::sql_lint::main::engine::lint_json; +use crate::sql_lint::tests::helpers; use crate::sql_lint::tests::test_types::{ - DynamicOutputStarLintTestCase, SqlMacroOutputLintTestCase, + DynamicOutputStarLintTestCase, LintTestCase, SqlMacroOutputLintTestCase, }; #[test] @@ -128,3 +129,45 @@ unrelated AS (SELECT * FROM pivot_input) SELECT * FROM final", } Ok(()) } + +#[test] +fn given_constant_projection_when_linting_row_selection_then_only_column_dependent_rows_fail() +-> Result<(), String> { + let test_cases = [ + LintTestCase { + description: "constant-only projection selects identical rows in any order", + sql: "SELECT CAST(100 AS DOUBLE) AS measured_value, CAST(NULL AS INTEGER), 0 samples FROM items LIMIT 1", + expected_codes: &[], + expected_anchors: &[], + }, + LintTestCase { + description: "a column reference inside a cast still makes row selection nondeterministic", + sql: "SELECT CAST(id AS DOUBLE) AS measured_value FROM items LIMIT 1", + expected_codes: &["SQLCLEAN004"], + expected_anchors: &[("SQLCLEAN004", "LIMIT")], + }, + LintTestCase { + description: "a function call in a constant-looking projection still needs ordering", + sql: "SELECT now() AS observed_at FROM items OFFSET 1", + expected_codes: &["SQLCLEAN004"], + expected_anchors: &[("SQLCLEAN004", "OFFSET")], + }, + ]; + for test_case in &test_cases { + let diagnostics = helpers::diagnostics(test_case.sql)?; + let found: Vec<(&str, &str)> = diagnostics + .iter() + .filter_map(|item| { + let start = item["start"].as_u64()? as usize; + let end = item["end"].as_u64()? as usize; + Some((item["code"].as_str()?, &test_case.sql[start..end])) + }) + .collect(); + assert_eq!( + found, test_case.expected_anchors, + "{}", + test_case.description + ); + } + Ok(()) +} diff --git a/website/src/content/docs/docs/rules/reference.mdx b/website/src/content/docs/docs/rules/reference.mdx index f75475a..f6cc252 100644 --- a/website/src/content/docs/docs/rules/reference.mdx +++ b/website/src/content/docs/docs/rules/reference.mdx @@ -199,7 +199,8 @@ FROM customer_orders ``` `SQLCLEAN004` reports the same problem for `LIMIT` and `OFFSET`; see the example in -[Rules](/docs/rules/overview/). +[Rules](/docs/rules/overview/). It skips a query that selects only constants, such as +`SELECT 1 AS present FROM orders LIMIT 1`, because every row it could return is identical. ## SQLCLEAN019: NULL in NOT IN