Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion crates/sqlclean-cli/skill/references/docs/rules/reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
33 changes: 5 additions & 28 deletions crates/sqlclean/src/sql_lint/_helpers/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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<usize> {
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::<u64>() == Ok(0)
})
}

pub(super) fn is_query_from(tokens: &[Token], index: usize) -> bool {
tokens[index].token_type == TokenType::From
&& significant_before(tokens, index)
Expand Down
70 changes: 70 additions & 0 deletions crates/sqlclean/src/sql_lint/_helpers/terminal_shape.rs
Original file line number Diff line number Diff line change
@@ -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<Span> {
Expand Down Expand Up @@ -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<usize> {
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::<u64>() == 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
))
}
45 changes: 44 additions & 1 deletion crates/sqlclean/src/sql_lint/tests/test_lint_exemptions.rs
Original file line number Diff line number Diff line change
@@ -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]
Expand Down Expand Up @@ -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(())
}
3 changes: 2 additions & 1 deletion website/src/content/docs/docs/rules/reference.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Loading