From 1fed2d1df4b106d16e6b7d3e63f756b8ea1f6e4e Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Mon, 2 Nov 2026 03:43:24 +0000 Subject: [PATCH] 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. --- .../sqlclean/src/sql_lint/_helpers/engine.rs | 98 +++++++++---------- .../src/sql_lint/tests/test_sql_lint.rs | 4 +- 2 files changed, 49 insertions(+), 53 deletions(-) diff --git a/crates/sqlclean/src/sql_lint/_helpers/engine.rs b/crates/sqlclean/src/sql_lint/_helpers/engine.rs index 2a9d874..b0d9ca7 100644 --- a/crates/sqlclean/src/sql_lint/_helpers/engine.rs +++ b/crates/sqlclean/src/sql_lint/_helpers/engine.rs @@ -1023,56 +1023,51 @@ fn projection_projects_star(tokens: &[Token], depths: &[usize], range: &Expressi fn collect_unused_ctes(expression: &Expression, options: &FactBuildOptions<'_>) -> Vec { let mut unused: Vec = Vec::new(); for node in expression.dfs() { - match node { - Expression::Select(select) if select.with.is_some() => { - let mut root = select.clone(); - if let Some(with) = root.with.take() { - unused.extend(collect_unused_for_with( - &with, - &Expression::Select(root), - options, - )); - } - } - Expression::Union(operation) if operation.with.is_some() => { - let mut root = operation.clone(); - if let Some(with) = root.with.take() { - unused.extend(collect_unused_for_with( - &with, - &Expression::Union(root), - options, - )); - } - } - Expression::Intersect(operation) if operation.with.is_some() => { - let mut root = operation.clone(); - if let Some(with) = root.with.take() { - unused.extend(collect_unused_for_with( - &with, - &Expression::Intersect(root), - options, - )); - } - } - Expression::Except(operation) if operation.with.is_some() => { - let mut root = operation.clone(); - if let Some(with) = root.with.take() { - unused.extend(collect_unused_for_with( - &with, - &Expression::Except(root), - options, - )); - } - } - _ => {} + let with: 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(), + _ => None, }; + if let Some(with) = with { + unused.extend(collect_unused_for_with( + with, + &outside_with_table_names(node, with), + options, + )); + } } unused } +/// Table names `root` references outside its own `with`, counted so a name the WITH also uses still counts. +fn outside_with_table_names(root: &Expression, with: &With) -> HashSet { + let mut counts: HashMap = HashMap::new(); + for name in table_name_occurrences(root) { + *counts.entry(name).or_default() += 1; + } + let inside = with + .ctes + .iter() + .map(|cte| &cte.this) + .chain(with.search.as_deref()); + for expression in inside { + for name in table_name_occurrences(expression) { + if let Some(count) = counts.get_mut(&name) { + *count -= 1; + } + } + } + counts + .into_iter() + .filter_map(|(name, count)| (count > 0).then_some(name)) + .collect() +} + fn collect_unused_for_with( with: &With, - root: &Expression, + root_tables: &HashSet, options: &FactBuildOptions<'_>, ) -> Vec { let names: HashSet = with @@ -1093,7 +1088,7 @@ fn collect_unused_for_with( ) }) .collect(); - let mut reachable: HashSet = table_names(root).intersection(&names).cloned().collect(); + let mut reachable: HashSet = root_tables.intersection(&names).cloned().collect(); reachable.extend( options .externally_referenced_ctes @@ -1126,13 +1121,14 @@ fn collect_unused_for_with( } fn table_names(expression: &Expression) -> HashSet { - expression - .dfs() - .filter_map(|node| match node { - Expression::Table(table) => Some(table.name.name.to_ascii_lowercase()), - _ => None, - }) - .collect() + table_name_occurrences(expression).collect() +} + +fn table_name_occurrences(expression: &Expression) -> impl Iterator + '_ { + expression.dfs().filter_map(|node| match node { + Expression::Table(table) => Some(table.name.name.to_ascii_lowercase()), + _ => None, + }) } fn null_comparison_fix(tokens: &[Token], span: Span) -> Option { diff --git a/crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs b/crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs index ae92f58..3b72db4 100644 --- a/crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs +++ b/crates/sqlclean/src/sql_lint/tests/test_sql_lint.rs @@ -133,8 +133,8 @@ fn given_sql_cases_when_linting_then_diagnostics_match() -> Result<(), String> { expected_anchors: &[("SQLCLEAN005", "unused")], }, test_types::LintTestCase { - description: "unreachable CTE chain", - sql: "WITH first AS (SELECT 1), second AS (SELECT * FROM first) SELECT 1", + description: "unreachable CTE chain beside a CTE both it and the main query read", + sql: "WITH used AS (SELECT 1 AS id), first AS (SELECT * FROM used), second AS (SELECT * FROM first) SELECT id FROM used", expected_codes: &["SQLCLEAN005", "SQLCLEAN005"], expected_anchors: &[], },