Skip to content

Commit 1fed2d1

Browse files
committed
perf(rules): find unused CTEs without cloning each WITH query
SQLCLEAN005 cloned every query that carries a WITH clause, including all of its CTE bodies, only to drop the WITH and read the remaining table names. Count table references in the whole query and subtract those inside the WITH instead, so a name read both inside and outside still counts as used.
1 parent 24d11cc commit 1fed2d1

2 files changed

Lines changed: 49 additions & 53 deletions

File tree

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

Lines changed: 47 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1023,56 +1023,51 @@ fn projection_projects_star(tokens: &[Token], depths: &[usize], range: &Expressi
10231023
fn collect_unused_ctes(expression: &Expression, options: &FactBuildOptions<'_>) -> Vec<String> {
10241024
let mut unused: Vec<String> = Vec::new();
10251025
for node in expression.dfs() {
1026-
match node {
1027-
Expression::Select(select) if select.with.is_some() => {
1028-
let mut root = select.clone();
1029-
if let Some(with) = root.with.take() {
1030-
unused.extend(collect_unused_for_with(
1031-
&with,
1032-
&Expression::Select(root),
1033-
options,
1034-
));
1035-
}
1036-
}
1037-
Expression::Union(operation) if operation.with.is_some() => {
1038-
let mut root = operation.clone();
1039-
if let Some(with) = root.with.take() {
1040-
unused.extend(collect_unused_for_with(
1041-
&with,
1042-
&Expression::Union(root),
1043-
options,
1044-
));
1045-
}
1046-
}
1047-
Expression::Intersect(operation) if operation.with.is_some() => {
1048-
let mut root = operation.clone();
1049-
if let Some(with) = root.with.take() {
1050-
unused.extend(collect_unused_for_with(
1051-
&with,
1052-
&Expression::Intersect(root),
1053-
options,
1054-
));
1055-
}
1056-
}
1057-
Expression::Except(operation) if operation.with.is_some() => {
1058-
let mut root = operation.clone();
1059-
if let Some(with) = root.with.take() {
1060-
unused.extend(collect_unused_for_with(
1061-
&with,
1062-
&Expression::Except(root),
1063-
options,
1064-
));
1065-
}
1066-
}
1067-
_ => {}
1026+
let with: Option<&With> = match node {
1027+
Expression::Select(select) => select.with.as_ref(),
1028+
Expression::Union(operation) => operation.with.as_ref(),
1029+
Expression::Intersect(operation) => operation.with.as_ref(),
1030+
Expression::Except(operation) => operation.with.as_ref(),
1031+
_ => None,
10681032
};
1033+
if let Some(with) = with {
1034+
unused.extend(collect_unused_for_with(
1035+
with,
1036+
&outside_with_table_names(node, with),
1037+
options,
1038+
));
1039+
}
10691040
}
10701041
unused
10711042
}
10721043

1044+
/// Table names `root` references outside its own `with`, counted so a name the WITH also uses still counts.
1045+
fn outside_with_table_names(root: &Expression, with: &With) -> HashSet<String> {
1046+
let mut counts: HashMap<String, usize> = HashMap::new();
1047+
for name in table_name_occurrences(root) {
1048+
*counts.entry(name).or_default() += 1;
1049+
}
1050+
let inside = with
1051+
.ctes
1052+
.iter()
1053+
.map(|cte| &cte.this)
1054+
.chain(with.search.as_deref());
1055+
for expression in inside {
1056+
for name in table_name_occurrences(expression) {
1057+
if let Some(count) = counts.get_mut(&name) {
1058+
*count -= 1;
1059+
}
1060+
}
1061+
}
1062+
counts
1063+
.into_iter()
1064+
.filter_map(|(name, count)| (count > 0).then_some(name))
1065+
.collect()
1066+
}
1067+
10731068
fn collect_unused_for_with(
10741069
with: &With,
1075-
root: &Expression,
1070+
root_tables: &HashSet<String>,
10761071
options: &FactBuildOptions<'_>,
10771072
) -> Vec<String> {
10781073
let names: HashSet<String> = with
@@ -1093,7 +1088,7 @@ fn collect_unused_for_with(
10931088
)
10941089
})
10951090
.collect();
1096-
let mut reachable: HashSet<String> = table_names(root).intersection(&names).cloned().collect();
1091+
let mut reachable: HashSet<String> = root_tables.intersection(&names).cloned().collect();
10971092
reachable.extend(
10981093
options
10991094
.externally_referenced_ctes
@@ -1126,13 +1121,14 @@ fn collect_unused_for_with(
11261121
}
11271122

11281123
fn table_names(expression: &Expression) -> HashSet<String> {
1129-
expression
1130-
.dfs()
1131-
.filter_map(|node| match node {
1132-
Expression::Table(table) => Some(table.name.name.to_ascii_lowercase()),
1133-
_ => None,
1134-
})
1135-
.collect()
1124+
table_name_occurrences(expression).collect()
1125+
}
1126+
1127+
fn table_name_occurrences(expression: &Expression) -> impl Iterator<Item = String> + '_ {
1128+
expression.dfs().filter_map(|node| match node {
1129+
Expression::Table(table) => Some(table.name.name.to_ascii_lowercase()),
1130+
_ => None,
1131+
})
11361132
}
11371133

11381134
fn null_comparison_fix(tokens: &[Token], span: Span) -> Option<LintEdit> {

‎crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,8 @@ fn given_sql_cases_when_linting_then_diagnostics_match() -> Result<(), String> {
133133
expected_anchors: &[("SQLCLEAN005", "unused")],
134134
},
135135
test_types::LintTestCase {
136-
description: "unreachable CTE chain",
137-
sql: "WITH first AS (SELECT 1), second AS (SELECT * FROM first) SELECT 1",
136+
description: "unreachable CTE chain beside a CTE both it and the main query read",
137+
sql: "WITH used AS (SELECT 1 AS id), first AS (SELECT * FROM used), second AS (SELECT * FROM first) SELECT id FROM used",
138138
expected_codes: &["SQLCLEAN005", "SQLCLEAN005"],
139139
expected_anchors: &[],
140140
},

0 commit comments

Comments
 (0)