Skip to content

Commit aed227a

Browse files
authored
fix(rules): end the SQLCLEAN053 ON scan at a PIVOT USING list (#24)
* fix(rules): end the SQLCLEAN053 ON scan at a PIVOT USING list A DuckDB PIVOT ... ON list is followed by USING aggregates, which the ON clause scan treated as aggregates inside a join condition. * perf(rules): find SET assignments in one forward pass The null comparison check scanned backwards from every comparison to find its clause, which is quadratic in long queries.
1 parent 65eb3f3 commit aed227a

3 files changed

Lines changed: 34 additions & 13 deletions

File tree

‎crates/sqlclean/src/sql_lint/_helpers/engine.rs‎

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1876,34 +1876,40 @@ fn null_comparison_spans(tokens: &[Token]) -> Vec<Span> {
18761876
TokenType::Gte,
18771877
];
18781878
let depths = token_depths(tokens);
1879+
let assignments: Vec<bool> = assignment_operators(tokens, &depths);
18791880
tokens
18801881
.iter()
18811882
.enumerate()
18821883
.filter(|(_, token)| comparison_types.contains(&token.token_type))
1883-
.filter(|(index, _)| !is_assignment_operator(tokens, &depths, *index))
1884+
.filter(|(index, _)| !assignments[*index])
18841885
.filter(|(index, _)| {
18851886
adjacent_null(tokens, *index, false) || adjacent_null(tokens, *index, true)
18861887
})
18871888
.map(|(_, token)| token.span)
18881889
.collect()
18891890
}
18901891

1891-
fn is_assignment_operator(tokens: &[Token], depths: &[usize], operator_index: usize) -> bool {
1892-
let depth = depths[operator_index];
1893-
for index in (0..operator_index).rev() {
1894-
if depths[index] != depth {
1895-
continue;
1892+
/// For each token, whether its nearest same-depth clause opener is `SET`, found in one forward pass.
1893+
fn assignment_operators(tokens: &[Token], depths: &[usize]) -> Vec<bool> {
1894+
let mut in_set: Vec<bool> = Vec::new();
1895+
let mut assignments: Vec<bool> = Vec::with_capacity(tokens.len());
1896+
for (token, &depth) in tokens.iter().zip(depths) {
1897+
if in_set.len() <= depth {
1898+
in_set.resize(depth + 1, false);
18961899
}
1897-
match tokens[index].token_type {
1898-
TokenType::Where | TokenType::Having | TokenType::Qualify | TokenType::On => {
1899-
return false;
1900-
}
1901-
TokenType::Set => return true,
1902-
TokenType::Select | TokenType::Semicolon => return false,
1900+
assignments.push(in_set[depth]);
1901+
match token.token_type {
1902+
TokenType::Set => in_set[depth] = true,
1903+
TokenType::Where
1904+
| TokenType::Having
1905+
| TokenType::Qualify
1906+
| TokenType::On
1907+
| TokenType::Select
1908+
| TokenType::Semicolon => in_set[depth] = false,
19031909
_ => {}
19041910
}
19051911
}
1906-
false
1912+
assignments
19071913
}
19081914

19091915
fn adjacent_null(tokens: &[Token], operator_index: usize, forward: bool) -> bool {

‎crates/sqlclean/src/sql_quality/_helpers/safety.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -702,6 +702,7 @@ fn ends_clause(significant: &Significant<'_>, index: usize, comma_ends: bool) ->
702702
| TokenType::Cross
703703
| TokenType::Natural
704704
| TokenType::Set
705+
| TokenType::Using
705706
),
706707
None => true,
707708
}

‎crates/sqlclean/src/sql_quality/tests/test_safety_rules.rs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,20 @@ fn given_safety_rule_inputs_when_linting_then_exactly_the_mistakes_are_reported(
171171
rule: "SQLCLEAN053",
172172
expected_anchors: &[],
173173
},
174+
SafetyRuleTestCase {
175+
description: "DuckDB PIVOT USING aggregates end the pivot ON clause",
176+
dialect: "duckdb",
177+
sql: "PIVOT orders ON status IN ('placed') USING MAX(amount), SUM(amount) GROUP BY customer_id",
178+
rule: "SQLCLEAN053",
179+
expected_anchors: &[],
180+
},
181+
SafetyRuleTestCase {
182+
description: "a join ON aggregate is still reported beside a pivot",
183+
dialect: "duckdb",
184+
sql: "SELECT o.id FROM orders AS o JOIN (PIVOT items ON kind USING MAX(qty)) AS p ON p.id = o.id AND MIN(o.amount) > 0",
185+
rule: "SQLCLEAN053",
186+
expected_anchors: &["MIN(o.amount)"],
187+
},
174188
SafetyRuleTestCase {
175189
description: "multi-argument MAX is the scalar function in SQLite",
176190
dialect: "sqlite",

0 commit comments

Comments
 (0)