From d08ce11d00595e6a0fa14a92dd045a7f7aee2637 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 27 Sep 2026 02:51:11 +0100 Subject: [PATCH 1/8] fix: report unused table aliases written without AS SQBRSQL023 only recognised aliases introduced by AS, so an unused implicit alias such as `FROM orders o` went unreported. Implicit aliases after relation names, subqueries and table functions now follow the same unused-alias contract. Refs CHI-376 --- .../src/sql_lint/_helpers/additional.rs | 64 ++++++-- .../src/sql_lint/tests.rs | 2 + .../sql_lint/tests/test_unused_table_alias.rs | 141 ++++++++++++++++++ .../sqlbuild/cli/commands/main/_test_types.py | 9 ++ .../sqlbuild/cli/commands/main/test_rules.py | 68 +++++++++ 5 files changed, 274 insertions(+), 10 deletions(-) create mode 100644 crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs diff --git a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs index 78977fb896..360f5fcfcc 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs @@ -1242,16 +1242,13 @@ fn unused_alias_spans(query: &QuerySlice<'_>) -> Vec { let query_end = *query_end; let depth = *depth; let mut spans: Vec = Vec::new(); - for window in relation_clause.windows(2) { - if tokens[window[0]].token_type != TokenType::As || !is_identifier(&tokens[window[1]]) { - continue; - } - if significant_after(tokens, window[1]) + for (span_start, alias_index) in relation_alias_positions(tokens, relation_clause) { + if significant_after(tokens, alias_index) .is_some_and(|index| tokens[index].token_type == TokenType::LParen) { continue; } - let alias = &tokens[window[1]].text; + let alias = &tokens[alias_index].text; let used = (query_start..query_end).any(|index| { depths[index] >= depth && tokens[index].text.eq_ignore_ascii_case(alias) @@ -1260,16 +1257,63 @@ fn unused_alias_spans(query: &QuerySlice<'_>) -> Vec { }); if !used { spans.push(Span { - start: tokens[window[0]].span.start, - end: tokens[window[1]].span.end, - line: tokens[window[0]].span.line, - column: tokens[window[0]].span.column, + start: tokens[span_start].span.start, + end: tokens[alias_index].span.end, + line: tokens[span_start].span.line, + column: tokens[span_start].span.column, }); } } spans } +fn relation_alias_positions(tokens: &[Token], relation_clause: &[usize]) -> Vec<(usize, usize)> { + let mut aliases: Vec<(usize, usize)> = Vec::new(); + let mut segment_start = 0_usize; + let mut in_condition = false; + for (position, &index) in relation_clause.iter().enumerate() { + if matches!(tokens[index].token_type, TokenType::Join | TokenType::Comma) { + segment_start = position + 1; + in_condition = false; + continue; + } + if in_condition || is_relation_condition_start(&tokens[index]) { + in_condition = true; + continue; + } + if tokens[index].token_type == TokenType::As { + if let Some(&alias) = relation_clause.get(position + 1) + && is_identifier(&tokens[alias]) + { + aliases.push((index, alias)); + } + continue; + } + if is_implicit_relation_alias( + tokens, + &relation_clause[segment_start..], + position - segment_start, + ) { + aliases.push((index, index)); + } + } + aliases +} + +fn is_implicit_relation_alias(tokens: &[Token], segment: &[usize], position: usize) -> bool { + if position == 0 || !is_identifier(&tokens[segment[position]]) { + return false; + } + let previous = &tokens[segment[position - 1]]; + if previous.token_type == TokenType::LParen { + return true; + } + is_identifier(previous) + && position + .checked_sub(QUALIFIER_PAIR_LENGTH) + .is_none_or(|before| tokens[segment[before]].token_type == TokenType::Dot) +} + fn null_rejected_left_join_spans( tokens: &[Token], direct: &[usize], diff --git a/crates/sqlbuild-rules-native/src/sql_lint/tests.rs b/crates/sqlbuild-rules-native/src/sql_lint/tests.rs index f9afcf7533..61539eb14a 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/tests.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/tests.rs @@ -12,5 +12,7 @@ mod helpers; mod sql_lint; #[path = "tests/test_types.rs"] mod test_types; +#[path = "tests/test_unused_table_alias.rs"] +mod unused_table_alias; #[path = "tests/test_values_formatter.rs"] mod values_formatter; diff --git a/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs b/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs new file mode 100644 index 0000000000..573119f283 --- /dev/null +++ b/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs @@ -0,0 +1,141 @@ +use crate::sql_lint::tests::{helpers, test_types}; + +#[test] +fn given_implicit_and_explicit_table_aliases_when_linting_then_unused_aliases_match() +-> Result<(), String> { + let test_cases = [ + test_types::AdditionalLintRuleTestCase { + description: "unused implicit table alias", + sql: "SELECT id FROM items unused", + rule: "SQBRSQL023", + expected_anchor: Some("unused"), + expected_replacement: Some(""), + }, + test_types::AdditionalLintRuleTestCase { + description: "unused implicit joined table alias", + sql: "SELECT o.id FROM orders o INNER JOIN customers unused ON o.customer_id = o.id", + rule: "SQBRSQL023", + expected_anchor: Some("unused"), + expected_replacement: Some(""), + }, + test_types::AdditionalLintRuleTestCase { + description: "unused implicit qualified table alias", + sql: "SELECT id FROM warehouse.analytics.items unused", + rule: "SQBRSQL023", + expected_anchor: Some("unused"), + expected_replacement: Some(""), + }, + test_types::AdditionalLintRuleTestCase { + description: "unused implicit subquery alias", + sql: "SELECT id FROM (SELECT id FROM items) unused", + rule: "SQBRSQL023", + expected_anchor: Some("unused"), + expected_replacement: Some(""), + }, + test_types::AdditionalLintRuleTestCase { + description: "unused implicit lateral table function alias", + sql: "SELECT src.id FROM items src, LATERAL FLATTEN(input => src.payload) unused", + rule: "SQBRSQL023", + expected_anchor: Some("unused"), + expected_replacement: Some(""), + }, + test_types::AdditionalLintRuleTestCase { + description: "used implicit table alias", + sql: "SELECT kept.id FROM items kept", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "used implicit joined table aliases", + sql: "SELECT o.id, c.name FROM orders o INNER JOIN customers c ON o.customer_id = c.id", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "used implicit qualified table alias", + sql: "SELECT kept.id FROM warehouse.analytics.items kept", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "used implicit subquery alias", + sql: "SELECT sub.id FROM (SELECT id FROM items) sub", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "used implicit lateral table function alias", + sql: "SELECT flattened.value FROM items src, LATERAL FLATTEN(input => src.payload) flattened", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "implicit alias used by correlated subquery", + sql: "SELECT id FROM items kept WHERE EXISTS (SELECT 1 FROM other WHERE other.id = kept.id)", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "implicit alias carrying a column list is retained", + sql: "SELECT a FROM items t(a)", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "qualified relation name is not an implicit alias", + sql: "SELECT id FROM warehouse.analytics.items", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "join modifier keywords are not implicit aliases", + sql: "SELECT o.id, c.name FROM orders o POSITIONAL JOIN customers c", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "join condition identifiers are not implicit aliases", + sql: "SELECT o.id, c.name FROM orders o INNER JOIN customers c USING (customer_id)", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + ]; + + for test_case in test_cases { + let diagnostics = helpers::diagnostics_for_rules(test_case.sql, &[test_case.rule])?; + assert_eq!( + diagnostics.len(), + usize::from(test_case.expected_anchor.is_some()), + "{}", + test_case.description + ); + let _ = test_case.expected_anchor.map(|expected_anchor| { + let diagnostic = &diagnostics[0]; + let start = diagnostic["start"].as_u64().unwrap_or_default() as usize; + let end = diagnostic["end"].as_u64().unwrap_or_default() as usize; + assert_eq!( + &test_case.sql[start..end], + expected_anchor, + "{}", + test_case.description + ); + assert_eq!( + diagnostic["fix"]["replacement"].as_str(), + test_case.expected_replacement, + "{}", + test_case.description + ); + }); + } + Ok(()) +} diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index cae9c3ac5e..3d77d8fd24 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -71,6 +71,15 @@ class RulesIntegrationTestCase: expected_code: str +@dataclass(frozen=True) +class ImplicitAliasRuleIntegrationTestCase: + """One model query and its expected unused-alias finding locations.""" + + description: str + query_sql: str + expected_locations: tuple[tuple[int, int], ...] + + @dataclass(frozen=True) class RulePassIntegrationTestCase: """One compiler-integrated Rules command expected to pass.""" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py index ec9bdab20f..d2a76ff179 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py @@ -19,6 +19,7 @@ from tests.integration.src.sqlbuild.cli.commands.main._test_types import ( DynamicPivotRulesIntegrationTestCase, ExplicitContractOutputRuleIntegrationTestCase, + ImplicitAliasRuleIntegrationTestCase, RulePassIntegrationTestCase, RulesIntegrationTestCase, TypedContractRuleIntegrationTestCase, @@ -956,6 +957,73 @@ def test_given_table_function_argument_when_running_alias_rule_then_left_alias_i assert payload["findings"] == [] +@pytest.mark.parametrize( + "test_case", + [ + ImplicitAliasRuleIntegrationTestCase( + description="unused implicit join alias is reported", + query_sql=( + "SELECT o.order_id\n" + 'FROM __ref("orders") o\n' + 'INNER JOIN __ref("customers") c ON o.customer_id = o.order_id\n' + ), + expected_locations=((4, 31),), + ), + ImplicitAliasRuleIntegrationTestCase( + description="unused implicit subquery alias is reported", + query_sql='SELECT order_id\nFROM (SELECT order_id FROM __ref("orders")) recent\n', + expected_locations=((3, 45),), + ), + ImplicitAliasRuleIntegrationTestCase( + description="implicit and explicit unused aliases are reported alike", + query_sql='SELECT order_id\nFROM __ref("orders") AS o, __ref("customers") c\n', + expected_locations=((3, 22), (3, 47)), + ), + ImplicitAliasRuleIntegrationTestCase( + description="used implicit aliases are clean", + query_sql=( + "SELECT o.order_id, c.customer_id\n" + 'FROM __ref("orders") o\n' + 'INNER JOIN __ref("customers") c ON o.customer_id = c.customer_id\n' + ), + expected_locations=(), + ), + ], + ids=lambda case: case.description, +) +def test_given_implicit_table_alias_when_running_alias_rule_then_usage_decides_finding( + test_case: ImplicitAliasRuleIntegrationTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + """Implicit aliases are held to the same unused-alias contract as AS aliases.""" + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n', encoding="utf-8" + ) + models: Path = tmp_path / "models" + models.mkdir() + (models / "orders.sql").write_text( + 'MODEL (description "Orders");\nSELECT 1 AS order_id, 2 AS customer_id\n', + encoding="utf-8", + ) + (models / "customers.sql").write_text( + 'MODEL (description "Customers");\nSELECT 2 AS customer_id\n', encoding="utf-8" + ) + (models / "order_customers.sql").write_text( + 'MODEL (description "Order customers");\n' + test_case.query_sql, encoding="utf-8" + ) + + exit_code: int = main(["--project-dir", str(tmp_path), "rules", "--json", "run", "SQBRSQL023"]) + + payload: dict[str, object] = json.loads(capsys.readouterr().out) + findings: list[dict[str, object]] = cast(list[dict[str, object]], payload["findings"]) + assert exit_code == int(bool(test_case.expected_locations)) + assert tuple((finding["line"], finding["column"]) for finding in findings) == ( + test_case.expected_locations + ) + assert {finding["path"] for finding in findings} <= {"models/order_customers.sql"} + + @pytest.mark.parametrize( "test_case", [RulesIntegrationTestCase("ceremonial SQL test passes terminal rule", 0, "SQBRSQL035")], From 4184396ec9819d0295ecef88d068d23e7af29e1b Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 27 Sep 2026 02:58:33 +0100 Subject: [PATCH 2/8] fix: report numeric BETWEEN bounds and IN lists as named decisions SQBRDECLARATION102 only inspected binary comparison operators, so literal BETWEEN bounds and numeric IN/NOT IN members bypassed the rule. Range and value-list predicates are now evaluated like the equivalent comparisons, and value lists point to a list or set constant used through IN @const(...). Refs CHI-397 --- .../src/engine/tests/test_evaluation.rs | 54 +++++++++++++ .../src/rules/_helpers/evaluation.rs | 57 +++++++++----- .../sqlbuild/cli/commands/main/_test_types.py | 11 +++ .../sqlbuild/cli/commands/main/test_rules.py | 78 +++++++++++++++++++ .../main/evaluate/test_evaluate.py | 18 +++++ 5 files changed, 200 insertions(+), 18 deletions(-) diff --git a/crates/sqlbuild-rules-native/src/engine/tests/test_evaluation.rs b/crates/sqlbuild-rules-native/src/engine/tests/test_evaluation.rs index 7b904bff6f..579941bbeb 100644 --- a/crates/sqlbuild-rules-native/src/engine/tests/test_evaluation.rs +++ b/crates/sqlbuild-rules-native/src/engine/tests/test_evaluation.rs @@ -954,6 +954,60 @@ fn given_numeric_comparisons_when_evaluating_named_decisions_then_only_authored_ authored_sql: ("SELECT CASE WHEN amount > 30 THEN 'large' END AS amount_30 FROM orders"), expected_fault_count: 1, }, + test_types::NumericDecisionTestCase { + description: "authored between bounds are a named decision finding", + query_sql: "SELECT code FROM orders WHERE TRY_CAST(code AS INTEGER) BETWEEN 30 AND 61", + authored_sql: "SELECT code FROM orders WHERE TRY_CAST(code AS INTEGER) BETWEEN 30 AND 61", + expected_fault_count: 1, + }, + test_types::NumericDecisionTestCase { + description: "authored not between bounds are a named decision finding", + query_sql: "SELECT code FROM orders WHERE amount NOT BETWEEN 5 AND 10", + authored_sql: "SELECT code FROM orders WHERE amount NOT BETWEEN 5 AND 10", + expected_fault_count: 1, + }, + test_types::NumericDecisionTestCase { + description: "canonical between bounds pass", + query_sql: "SELECT code FROM orders WHERE amount BETWEEN 0 AND 1", + authored_sql: "SELECT code FROM orders WHERE amount BETWEEN 0 AND 1", + expected_fault_count: 0, + }, + test_types::NumericDecisionTestCase { + description: "expanded between constants are not authored literal findings", + query_sql: "SELECT code FROM orders WHERE amount BETWEEN 30 AND 61", + authored_sql: "SELECT code FROM orders WHERE amount BETWEEN @const(\"_low\") AND @const(\"_high\")", + expected_fault_count: 0, + }, + test_types::NumericDecisionTestCase { + description: "authored numeric in list is a named decision finding", + query_sql: "SELECT code FROM orders WHERE TRY_CAST(code AS INTEGER) IN (30, 31, 45)", + authored_sql: "SELECT code FROM orders WHERE TRY_CAST(code AS INTEGER) IN (30, 31, 45)", + expected_fault_count: 1, + }, + test_types::NumericDecisionTestCase { + description: "authored numeric not in list is a named decision finding", + query_sql: "SELECT code FROM orders WHERE amount NOT IN (30, 31)", + authored_sql: "SELECT code FROM orders WHERE amount NOT IN (30, 31)", + expected_fault_count: 1, + }, + test_types::NumericDecisionTestCase { + description: "canonical numeric in list passes", + query_sql: "SELECT code FROM orders WHERE amount IN (-1, 0, 1)", + authored_sql: "SELECT code FROM orders WHERE amount IN (-1, 0, 1)", + expected_fault_count: 0, + }, + test_types::NumericDecisionTestCase { + description: "expanded list constant is not an authored literal finding", + query_sql: "SELECT code FROM orders WHERE amount IN (30, 31, 45)", + authored_sql: "SELECT code FROM orders WHERE amount IN @const(\"_codes\")", + expected_fault_count: 0, + }, + test_types::NumericDecisionTestCase { + description: "between in projected case is a named decision finding", + query_sql: "SELECT CASE WHEN amount BETWEEN 30 AND 61 THEN 'mid' END AS band FROM orders", + authored_sql: "SELECT CASE WHEN amount BETWEEN 30 AND 61 THEN 'mid' END AS band FROM orders", + expected_fault_count: 1, + }, test_types::NumericDecisionTestCase { description: "nested comparison reports only its leaf decision", query_sql: ("SELECT CASE WHEN amount > 5 THEN status END = 'ready' AS selected FROM orders"), diff --git a/crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs b/crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs index 01407def13..e67d3451a2 100644 --- a/crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs +++ b/crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs @@ -30,6 +30,7 @@ use std::ops::ControlFlow; const MIXED_STAR_REMEDIATION: &str = "SELECT *, a, b mixes a passthrough star with derived columns. The * exemption permits a lone SELECT * only. Move a and b into an earlier CTE so they are computed upstream, leaving the final select a pure SELECT *. If you need those columns in the output, drop the * exemption and enumerate every column explicitly."; const RULES_PARSER_RECURSION_LIMIT: usize = 128; +const NUMERIC_VALUE_LIST_REMEDIATION: &str = "Declare the values as a list or set CONSTANT and compare through [NOT] IN @const(\"\"); only -1, 0, and 1 are self-explanatory."; #[derive(Clone)] struct Position { @@ -42,6 +43,8 @@ struct ComparisonFact { position: Option, sql: String, equality: bool, + value_list: bool, + binary: bool, output_name: Option, columns: BTreeSet, string_literals: Vec, @@ -1226,7 +1229,9 @@ fn evaluate_literal_rules( ) { let facts = select_facts(&parsed.query); for comparison in facts.into_iter().flat_map(|select| select.comparisons) { - if let Some(rule) = selected.get("SQBRDECLARATION101") { + if comparison.binary + && let Some(rule) = selected.get("SQBRDECLARATION101") + { let enum_column = comparison .columns .iter() @@ -1265,7 +1270,9 @@ fn evaluate_literal_rules( rule, comparison.position.as_ref(), format!("non-canonical numeric comparison: {}", comparison.sql), - None, + comparison + .value_list + .then(|| NUMERIC_VALUE_LIST_REMEDIATION.to_owned()), )); } } @@ -1669,8 +1676,8 @@ fn comparison_facts(root: &Expr, source_context: &SourceContext, output: &mut Ve impl Visitor for Comparisons<'_> { type Break = (); fn pre_visit_expr(&mut self, expression: &Expr) -> ControlFlow { - if let Expr::BinaryOp { op, .. } = expression - && matches!( + let is_comparison = match expression { + Expr::BinaryOp { op, .. } => matches!( op, BinaryOperator::Eq | BinaryOperator::NotEq @@ -1678,8 +1685,11 @@ fn comparison_facts(root: &Expr, source_context: &SourceContext, output: &mut Ve | BinaryOperator::GtEq | BinaryOperator::Lt | BinaryOperator::LtEq - ) - { + ), + Expr::Between { .. } | Expr::InList { .. } => true, + _ => false, + }; + if is_comparison { self.output .push(comparison_fact(expression, self.source_context)); } @@ -1776,29 +1786,40 @@ fn comparison_fact(root: &Expr, source_context: &SourceContext) -> ComparisonFac Expr::BinaryOp { op: BinaryOperator::Eq, .. - } + } | Expr::InList { negated: false, .. } ), + value_list: matches!(root, Expr::InList { .. }), + binary: matches!(root, Expr::BinaryOp { .. }), source_context: source_context.clone(), ..ComparisonFact::default() }, }; let _ = root.visit(&mut visitor); - if let Expr::BinaryOp { left, right, .. } = root { - for operand in [left.as_ref(), right.as_ref()] { - if let Some(number) = numeric_value(operand) { - visitor.fact.numeric_literals.push(number); - } - if direct_column(operand).is_none() { - visitor.fact.modified_columns.extend(column_facts(operand)); - } - if direct_string_literal(operand).is_none() && contains_string_literal(operand) { - visitor.fact.modified_string_literal = true; - } + for operand in comparison_operands(root) { + if let Some(number) = numeric_value(operand) { + visitor.fact.numeric_literals.push(number); + } + if direct_column(operand).is_none() { + visitor.fact.modified_columns.extend(column_facts(operand)); + } + if direct_string_literal(operand).is_none() && contains_string_literal(operand) { + visitor.fact.modified_string_literal = true; } } visitor.fact } +fn comparison_operands(root: &Expr) -> Vec<&Expr> { + match root { + Expr::BinaryOp { left, right, .. } => vec![left.as_ref(), right.as_ref()], + Expr::Between { + expr, low, high, .. + } => vec![expr.as_ref(), low.as_ref(), high.as_ref()], + Expr::InList { expr, list, .. } => std::iter::once(expr.as_ref()).chain(list).collect(), + _ => Vec::new(), + } +} + fn direct_column(expression: &Expr) -> Option { match unwrap_nested(expression) { Expr::Identifier(value) => Some(ColumnFact { diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index 3d77d8fd24..f10b29960f 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -80,6 +80,17 @@ class ImplicitAliasRuleIntegrationTestCase: expected_locations: tuple[tuple[int, int], ...] +@dataclass(frozen=True) +class NumericRangeDecisionIntegrationTestCase: + """One numeric range or value-list predicate and its expected rule findings.""" + + description: str + predicate: str + expected_findings: tuple[tuple[str, str], ...] + expected_exit_code: int + constants: str = "" + + @dataclass(frozen=True) class RulePassIntegrationTestCase: """One compiler-integrated Rules command expected to pass.""" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py index d2a76ff179..a8aed10d13 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py @@ -20,6 +20,7 @@ DynamicPivotRulesIntegrationTestCase, ExplicitContractOutputRuleIntegrationTestCase, ImplicitAliasRuleIntegrationTestCase, + NumericRangeDecisionIntegrationTestCase, RulePassIntegrationTestCase, RulesIntegrationTestCase, TypedContractRuleIntegrationTestCase, @@ -159,6 +160,83 @@ def test_given_exact_internal_edge_exception_when_compiling_then_live_edge_passe ) +@pytest.mark.parametrize( + "test_case", + [ + NumericRangeDecisionIntegrationTestCase( + description="literal between bounds and value list are reported", + predicate=( + "TRY_CAST(code AS INTEGER) BETWEEN 30 AND 61\n" + " OR TRY_CAST(code AS INTEGER) NOT IN (30, 31, 45)" + ), + expected_findings=( + ( + "non-canonical numeric comparison: TRY_CAST(code AS INTEGER) BETWEEN 30 AND 61", + "Declare the threshold as a CONSTANT and compare through " + '@const(""); only -1, 0, and 1 are self-explanatory.', + ), + ( + "non-canonical numeric comparison: " + "TRY_CAST(code AS INTEGER) NOT IN (30, 31, 45)", + "Declare the values as a list or set CONSTANT and compare through " + '[NOT] IN @const(""); only -1, 0, and 1 are self-explanatory.', + ), + ), + expected_exit_code=1, + ), + NumericRangeDecisionIntegrationTestCase( + description="constant-backed between bounds and value list pass", + predicate=( + 'TRY_CAST(code AS INTEGER) BETWEEN @const("_low_code") AND @const("_high_code")\n' + ' OR TRY_CAST(code AS INTEGER) NOT IN @const("_held_codes")' + ), + expected_findings=(), + expected_exit_code=0, + constants=" constants (_low_code 30, _high_code 61, _held_codes [30, 31, 45]),\n", + ), + NumericRangeDecisionIntegrationTestCase( + description="canonical between bounds and value list pass", + predicate=( + "TRY_CAST(code AS INTEGER) BETWEEN 0 AND 1\n" + " OR TRY_CAST(code AS INTEGER) IN (-1, 0, 1)" + ), + expected_findings=(), + expected_exit_code=0, + ), + ], + ids=lambda case: case.description, +) +def test_given_numeric_range_or_value_list_when_compiling_then_literal_decisions_are_reported( + test_case: NumericRangeDecisionIntegrationTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + """BETWEEN bounds and IN members follow the same named-decision rule as comparisons.""" + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n\n[rules]\nselect = ["SQBRDECLARATION102"]\n', + encoding="utf-8", + ) + model: Path = tmp_path / "models" / "orders.sql" + model.parent.mkdir() + model.write_text( + "MODEL (\n" + ' description "Orders",\n' + test_case.constants + ");\n\n" + "SELECT code\n" + "FROM (SELECT '31' AS code) AS items\n" + f"WHERE {test_case.predicate}\n", + encoding="utf-8", + ) + + exit_code: int = main(["--project-dir", str(tmp_path), "compile", "--json"]) + result: dict[str, object] = json.loads(capsys.readouterr().out) + diagnostics: list[dict[str, object]] = cast(list[dict[str, object]], result["diagnostics"]) + + assert exit_code == test_case.expected_exit_code + assert tuple((diagnostic["message"], diagnostic["help"]) for diagnostic in diagnostics) == ( + test_case.expected_findings + ) + + @pytest.mark.parametrize( "test_case", [RulePassIntegrationTestCase("constant-backed numeric decision", 0, ())], diff --git a/tests/unit/src/sqlbuild/rule_engine/main/evaluate/test_evaluate.py b/tests/unit/src/sqlbuild/rule_engine/main/evaluate/test_evaluate.py index f3e97417cc..5d9d979b7d 100644 --- a/tests/unit/src/sqlbuild/rule_engine/main/evaluate/test_evaluate.py +++ b/tests/unit/src/sqlbuild/rule_engine/main/evaluate/test_evaluate.py @@ -206,6 +206,24 @@ select=("SQBRDECLARATION102",), expected_codes=("SQBRDECLARATION102",), ), + PolicyEvaluationTestCase( + description="numeric between bounds fault", + model_name="commerce__mart__orders", + relative_path="models/mart/commerce__mart__orders.sql", + sql="SELECT id FROM items WHERE item_count BETWEEN 30 AND 61", + config_values={"materialized": "table", "contract": "enforced"}, + select=("SQBRDECLARATION102",), + expected_codes=("SQBRDECLARATION102",), + ), + PolicyEvaluationTestCase( + description="numeric value list faults", + model_name="commerce__mart__orders", + relative_path="models/mart/commerce__mart__orders.sql", + sql="SELECT id FROM items WHERE item_count IN (30, 31, 45)", + config_values={"materialized": "table", "contract": "enforced"}, + select=("SQBRDECLARATION102",), + expected_codes=("SQBRDECLARATION102",), + ), PolicyEvaluationTestCase( description="canonical numeric decision passes", model_name="commerce__mart__orders", From 043d68b836890d358b9388a41c92cba99996e79a Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 27 Sep 2026 03:16:06 +0100 Subject: [PATCH 3/8] fix: treat dollar-quoted strings as opaque literals in compiler scanners The compiler's reference, macro, declaration, interpolation and resolution scanners only recognised quote characters, so an apostrophe inside a $$...$$ literal opened a phantom quote and compilation failed with an unclosed quoted string, while reference-shaped text inside dollar quotes could be extracted as a dependency. The Python scanners and the native compiler scan policy now skip $$ and $tag$ literals like single-quoted strings. Refs CHI-375 --- .../src/compiler/tests/helpers.rs | 9 +++ .../src/compiler/tests/test_sql_references.rs | 7 +- .../src/engine/tests/test_sql_scanning.rs | 2 +- .../src/rules/_helpers/evaluation.rs | 1 + .../src/sql_scan/main/non_code_end.rs | 29 ++++++++ .../src/sql_scan/models.rs | 6 +- .../src/sql_scan/tests/test_sql_scan.rs | 40 ++++++++++- .../compile/_helpers/refs/references.py | 2 +- .../_helpers/render/cursor_intrinsics.py | 4 +- .../compile/_helpers/render/declarations.py | 2 +- .../compile/_helpers/render/macros.py | 2 +- .../compile/_helpers/render/sql_vars.py | 2 +- .../compile/_helpers/sql_tests/core.py | 3 +- src/sqlbuild/compiler/compile/constants.py | 2 +- .../compiler/planner/_helpers/resolve/refs.py | 2 +- .../references/_helpers/resolution.py | 2 +- .../sql_analysis/_helpers/scanning.py | 67 +++++++++++++------ .../compiler/sql_analysis/constants.py | 2 + .../sqlbuild/cli/commands/main/_test_types.py | 9 +++ .../main/test_dollar_quoted_literals.py | 66 ++++++++++++++++++ .../compile/_helpers/test_sql_references.py | 24 ++++++- .../sql_analysis/_helpers/test_scanning.py | 31 +++++++++ 22 files changed, 281 insertions(+), 33 deletions(-) create mode 100644 tests/integration/src/sqlbuild/cli/commands/main/test_dollar_quoted_literals.py diff --git a/crates/sqlbuild-rules-native/src/compiler/tests/helpers.rs b/crates/sqlbuild-rules-native/src/compiler/tests/helpers.rs index d72e8bf7ac..f62533ed44 100644 --- a/crates/sqlbuild-rules-native/src/compiler/tests/helpers.rs +++ b/crates/sqlbuild-rules-native/src/compiler/tests/helpers.rs @@ -67,6 +67,14 @@ pub(crate) fn comments_and_quoted_text_hide_references() -> bool { == Some(vec![("ref".to_owned(), "orders".to_owned(), None, None)]) } +pub(crate) fn dollar_quoted_text_hides_references() -> bool { + extract(concat!( + "SELECT $$Customer's order -- __ref(\"ignored\") $5$$ AS label, ", + "$tag$ $$ __seed(\"also_ignored\") $tag$ AS note, price$1$ ", + "FROM __ref(orders)" + )) == Some(vec![("ref".to_owned(), "orders".to_owned(), None, None)]) +} + pub(crate) fn complex_or_malformed_sql_requests_fallback() -> bool { [ "SELECT * FROM __table_fn(\"orders\")(1)", @@ -74,6 +82,7 @@ pub(crate) fn complex_or_malformed_sql_requests_fallback() -> bool { "SELECT * FROM __ref(\"orders\"", "SELECT * FROM __ref(\"orders\") /* unterminated", "SELECT * FROM __ref(\"orders\") WHERE note = 'unterminated", + "SELECT * FROM __ref(\"orders\") WHERE note = $$unterminated", "SELECT * FROM __ref(örders)", ] .into_iter() diff --git a/crates/sqlbuild-rules-native/src/compiler/tests/test_sql_references.rs b/crates/sqlbuild-rules-native/src/compiler/tests/test_sql_references.rs index cda5c3de96..50ecf02ba2 100644 --- a/crates/sqlbuild-rules-native/src/compiler/tests/test_sql_references.rs +++ b/crates/sqlbuild-rules-native/src/compiler/tests/test_sql_references.rs @@ -1,6 +1,6 @@ use crate::compiler::tests::helpers::{ comments_and_quoted_text_hide_references, complex_or_malformed_sql_requests_fallback, - simple_references_preserve_authored_order, + dollar_quoted_text_hides_references, simple_references_preserve_authored_order, }; use crate::compiler::tests::test_types::StaticSqlOperationTestCase; @@ -17,6 +17,11 @@ fn given_sql_reference_cases_when_extracting_then_expected_behavior_holds() { run: comments_and_quoted_text_hide_references, expected_success: true, }, + StaticSqlOperationTestCase { + description: "dollar-quoted text hides embedded references", + run: dollar_quoted_text_hides_references, + expected_success: true, + }, StaticSqlOperationTestCase { description: "complex and malformed SQL requests Python fallback", run: complex_or_malformed_sql_requests_fallback, diff --git a/crates/sqlbuild-rules-native/src/engine/tests/test_sql_scanning.rs b/crates/sqlbuild-rules-native/src/engine/tests/test_sql_scanning.rs index f2e5c1b7b4..5d4607b0b3 100644 --- a/crates/sqlbuild-rules-native/src/engine/tests/test_sql_scanning.rs +++ b/crates/sqlbuild-rules-native/src/engine/tests/test_sql_scanning.rs @@ -170,7 +170,7 @@ fn given_quoted_commented_and_malformed_fragments_when_scanning_then_every_scann SqlScannerTestCase { description: "dollar quote", fragment: "$$ ) $$", - expected_compiler_paren: Ok(4), + expected_compiler_paren: Ok(10), expected_table_function: "SELECT * FROM __table_fn(\"o\", $$ ) $$ x)(1)", expected_table_function_token: "SELECT $$ ) $$ __table_fn(\"o\", 1)", expected_snowflake_exclude: "SELECT * $$ x) , a FROM t", diff --git a/crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs b/crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs index e67d3451a2..881c4f9658 100644 --- a/crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs +++ b/crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs @@ -315,6 +315,7 @@ pub(crate) fn rules_quote_policy(dialect_name: &str) -> QuotePolicy { single_quote_backslash_escapes: backslash_escapes, double_quote_backslash_escapes: backslash_escapes && !dialect.is_delimited_identifier_start('"'), + dollar_quotes: false, } } diff --git a/crates/sqlbuild-rules-native/src/sql_scan/main/non_code_end.rs b/crates/sqlbuild-rules-native/src/sql_scan/main/non_code_end.rs index cb99e49124..bf57e1b0aa 100644 --- a/crates/sqlbuild-rules-native/src/sql_scan/main/non_code_end.rs +++ b/crates/sqlbuild-rules-native/src/sql_scan/main/non_code_end.rs @@ -15,6 +15,35 @@ pub(crate) fn non_code_end( } match sql.get(index) { Some(byte) if policy.is_quote(*byte) => quote_end(sql, index, policy).map(Some), + Some(b'$') if policy.dollar_quotes => dollar_quote_end(sql, index), _ => Ok(None), } } + +/// Return the end of the `$$` or `$tag$` literal opening at `start`, if one opens there. +fn dollar_quote_end(sql: &[u8], start: usize) -> Result, Unclosed> { + if start + .checked_sub(1) + .is_some_and(|previous| continues_dollar_word(sql[previous])) + { + return Ok(None); + } + let tag_end = sql[start + 1..] + .iter() + .position(|byte| !(byte.is_ascii_alphanumeric() || *byte == b'_')) + .map_or(sql.len(), |offset| start + 1 + offset); + let tag = &sql[start + 1..tag_end]; + if sql.get(tag_end) != Some(&b'$') || tag.first().is_some_and(u8::is_ascii_digit) { + return Ok(None); + } + let delimiter = &sql[start..=tag_end]; + sql[tag_end + 1..] + .windows(delimiter.len()) + .position(|window| window == delimiter) + .map(|offset| Some(tag_end + 1 + offset + delimiter.len())) + .ok_or(Unclosed::Quote) +} + +fn continues_dollar_word(byte: u8) -> bool { + !byte.is_ascii() || byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'$') +} diff --git a/crates/sqlbuild-rules-native/src/sql_scan/models.rs b/crates/sqlbuild-rules-native/src/sql_scan/models.rs index 4f508b195c..bba4289068 100644 --- a/crates/sqlbuild-rules-native/src/sql_scan/models.rs +++ b/crates/sqlbuild-rules-native/src/sql_scan/models.rs @@ -17,14 +17,17 @@ pub(crate) struct QuotePolicy { pub(crate) single_quote_backslash_escapes: bool, /// A backslash escapes the next byte inside double-quoted text. pub(crate) double_quote_backslash_escapes: bool, + /// `$$` and `$tag$` delimit opaque string literals. + pub(crate) dollar_quotes: bool, } impl QuotePolicy { - /// Compiler policy, mirroring Python `sql_analysis`: backticks quote, backslashes are literal. + /// Compiler policy, mirroring Python `sql_analysis`: backticks and dollar quotes are text. pub(crate) const COMPILER: Self = Self { backtick_identifiers: true, single_quote_backslash_escapes: false, double_quote_backslash_escapes: false, + dollar_quotes: true, }; /// SQL lint base policy: backslashes escape; lint enables backticks per dialect. @@ -32,6 +35,7 @@ impl QuotePolicy { backtick_identifiers: false, single_quote_backslash_escapes: true, double_quote_backslash_escapes: true, + dollar_quotes: false, }; /// Return this policy with backtick-delimited identifiers enabled or disabled. diff --git a/crates/sqlbuild-rules-native/src/sql_scan/tests/test_sql_scan.rs b/crates/sqlbuild-rules-native/src/sql_scan/tests/test_sql_scan.rs index 99da540962..8079314db4 100644 --- a/crates/sqlbuild-rules-native/src/sql_scan/tests/test_sql_scan.rs +++ b/crates/sqlbuild-rules-native/src/sql_scan/tests/test_sql_scan.rs @@ -8,12 +8,14 @@ const QUOTES_ONLY: QuotePolicy = QuotePolicy { backtick_identifiers: false, single_quote_backslash_escapes: false, double_quote_backslash_escapes: false, + dollar_quotes: false, }; const BACKSLASH_ONLY: QuotePolicy = QuotePolicy { backtick_identifiers: false, single_quote_backslash_escapes: true, double_quote_backslash_escapes: false, + dollar_quotes: false, }; #[test] @@ -62,9 +64,15 @@ fn given_quote_policies_when_matching_parentheses_then_policy_decides_quote_boun expected_close: Ok(12), }, MatchingParenPolicyTestCase { - description: "dollar quotes are scanned as code", + description: "compiler policy skips dollar-quoted text", sql: "($$ ) $$ x) y", policy: QuotePolicy::COMPILER, + expected_close: Ok(10), + }, + MatchingParenPolicyTestCase { + description: "dollar quotes are code when the policy disables them", + sql: "($$ ) $$ x) y", + policy: QuotePolicy::SQL_LINT, expected_close: Ok(4), }, MatchingParenPolicyTestCase { @@ -136,6 +144,36 @@ fn given_comment_or_quote_starts_when_finding_non_code_end_then_returns_end_offs policy: QuotePolicy::SQL_LINT, expected_end: Err(Unclosed::Quote), }, + NonCodeEndTestCase { + description: "dollar quote hides apostrophes, comments and single dollars", + sql: "$$it's -- $5 $$ b", + policy: QuotePolicy::COMPILER, + expected_end: Ok(Some(15)), + }, + NonCodeEndTestCase { + description: "tagged dollar quote ends only at its own tag", + sql: "$tag$ $$ ) $tag$ b", + policy: QuotePolicy::COMPILER, + expected_end: Ok(Some(16)), + }, + NonCodeEndTestCase { + description: "unterminated dollar quote reports the quote", + sql: "$$it's", + policy: QuotePolicy::COMPILER, + expected_end: Err(Unclosed::Quote), + }, + NonCodeEndTestCase { + description: "positional parameter is code", + sql: "$1 $", + policy: QuotePolicy::COMPILER, + expected_end: Ok(None), + }, + NonCodeEndTestCase { + description: "digit-led tag is not a dollar quote", + sql: "$1$ x $1$", + policy: QuotePolicy::COMPILER, + expected_end: Ok(None), + }, NonCodeEndTestCase { description: "plain code has no non-code end", sql: "SELECT 1", diff --git a/src/sqlbuild/compiler/compile/_helpers/refs/references.py b/src/sqlbuild/compiler/compile/_helpers/refs/references.py index 0e000b994b..255f6b19c9 100644 --- a/src/sqlbuild/compiler/compile/_helpers/refs/references.py +++ b/src/sqlbuild/compiler/compile/_helpers/refs/references.py @@ -34,7 +34,7 @@ _REFERENCE_PREFIX_BY_KIND: dict[SqlReferenceKind, str] = { ref_kind: prefix[:-1] for prefix, ref_kind in _REFERENCE_PREFIXES } -_REFERENCE_SCAN_PATTERN: re.Pattern[str] = re.compile(r"__|--|/\*|'|\"|`") +_REFERENCE_SCAN_PATTERN: re.Pattern[str] = re.compile(r"__|--|/\*|'|\"|`|\$") def extract_sql_references(sql: str) -> tuple[CompileSqlReference, ...]: diff --git a/src/sqlbuild/compiler/compile/_helpers/render/cursor_intrinsics.py b/src/sqlbuild/compiler/compile/_helpers/render/cursor_intrinsics.py index 230b2af9a1..f98fff0085 100644 --- a/src/sqlbuild/compiler/compile/_helpers/render/cursor_intrinsics.py +++ b/src/sqlbuild/compiler/compile/_helpers/render/cursor_intrinsics.py @@ -26,7 +26,9 @@ _IDENTIFIER_JOIN_CHARACTER: str = "_" _LINE_COMMENT_TOKEN: str = "--" _BLOCK_COMMENT_TOKEN: str = "/*" -_INTRINSIC_SCAN_PATTERN: re.Pattern[str] = re.compile(r"__cursor_start|__cursor_end|--|/\*|'|\"|`") +_INTRINSIC_SCAN_PATTERN: re.Pattern[str] = re.compile( + r"__cursor_start|__cursor_end|--|/\*|'|\"|`|\$" +) def get_validated_model_cursor_intrinsics( diff --git a/src/sqlbuild/compiler/compile/_helpers/render/declarations.py b/src/sqlbuild/compiler/compile/_helpers/render/declarations.py index d847acff0c..b9f4ee936a 100644 --- a/src/sqlbuild/compiler/compile/_helpers/render/declarations.py +++ b/src/sqlbuild/compiler/compile/_helpers/render/declarations.py @@ -65,7 +65,7 @@ r"(?P=quote)\s*\)" ) _DECLARATION_REFERENCE_START_PATTERN: re.Pattern[str] = re.compile(r"@(?Penum|const)\b") -_DECLARATION_SCAN_SPECIAL: re.Pattern[str] = re.compile(r"['\"`@/-]") +_DECLARATION_SCAN_SPECIAL: re.Pattern[str] = re.compile(r"['\"`$@/-]") _CONTEXT: str = "Enum and constant expansion" _ACCEPTED_VALUES_AUDIT: str = "accepted_values" _ENUM_REFERENCE_KIND: str = "enum" diff --git a/src/sqlbuild/compiler/compile/_helpers/render/macros.py b/src/sqlbuild/compiler/compile/_helpers/render/macros.py index f428aa948d..000bb0802b 100644 --- a/src/sqlbuild/compiler/compile/_helpers/render/macros.py +++ b/src/sqlbuild/compiler/compile/_helpers/render/macros.py @@ -68,7 +68,7 @@ _LINE_COMMENT_TOKEN: str = "--" _BLOCK_COMMENT_TOKEN: str = "/*" _STAR_IMPORT_NAME: str = "*" -_MACRO_SCAN_PATTERN: re.Pattern[str] = re.compile(r"@|--|/\*|'|\"|`") +_MACRO_SCAN_PATTERN: re.Pattern[str] = re.compile(r"@|--|/\*|'|\"|`|\$") _SYNTHETIC_PACKAGE_PREFIX: str = "_sqlbuild_project_macros_" _OBJECT_ENTRY_LENGTH: int = 2 _MACRO_MODULE_LOAD_LOCK: threading.RLock = threading.RLock() diff --git a/src/sqlbuild/compiler/compile/_helpers/render/sql_vars.py b/src/sqlbuild/compiler/compile/_helpers/render/sql_vars.py index 2d4dd10672..80f496ab11 100644 --- a/src/sqlbuild/compiler/compile/_helpers/render/sql_vars.py +++ b/src/sqlbuild/compiler/compile/_helpers/render/sql_vars.py @@ -46,7 +46,7 @@ from sqlbuild.compiler.sql_analysis.main._skip_quoted_text import skip_quoted_text from sqlbuild.sql_values.types import CollectionRendering -_SQL_INTERPOLATION_SPECIAL: re.Pattern[str] = re.compile(r"['\"`\-/@]") +_SQL_INTERPOLATION_SPECIAL: re.Pattern[str] = re.compile(r"['\"`$\-/@]") _CONTEXT: str = "SQL interpolation" _NATIVE_UNCHANGED: int = 0 diff --git a/src/sqlbuild/compiler/compile/_helpers/sql_tests/core.py b/src/sqlbuild/compiler/compile/_helpers/sql_tests/core.py index 3b49726ee3..687955d01b 100644 --- a/src/sqlbuild/compiler/compile/_helpers/sql_tests/core.py +++ b/src/sqlbuild/compiler/compile/_helpers/sql_tests/core.py @@ -72,6 +72,7 @@ _SQL_TEST_WITH_REQUIREMENT: str = "mock CTEs and one __expected__ CTE" _DIRECT_DEPENDENCY_PATH_LENGTH: int = 2 _SQL_IDENTIFIER_QUOTE_TOKENS: frozenset[str] = frozenset({'"', "`"}) +_SQL_STRING_QUOTE_TOKENS: frozenset[str] = frozenset({SQL_SINGLE_QUOTE_TOKEN, "$"}) @dataclass(frozen=True) @@ -1000,7 +1001,7 @@ def _known_cte_identifier_references(*, sql: str, names_by_key: dict[str, str]) if sql.startswith("/*", index): index = skip_block_comment(sql=sql, start=index, context=_CONTEXT) continue - if sql[index] == SQL_SINGLE_QUOTE_TOKEN: + if sql[index] in _SQL_STRING_QUOTE_TOKENS: index = skip_quoted_text(sql=sql, start=index, context=_CONTEXT) continue if sql[index] in _SQL_IDENTIFIER_QUOTE_TOKENS: diff --git a/src/sqlbuild/compiler/compile/constants.py b/src/sqlbuild/compiler/compile/constants.py index 552d76a929..38c91398ea 100644 --- a/src/sqlbuild/compiler/compile/constants.py +++ b/src/sqlbuild/compiler/compile/constants.py @@ -42,7 +42,7 @@ SQL_ARGUMENT_SEPARATOR_TOKEN: str = "," SQL_STATEMENT_TERMINATOR_TOKEN: str = ";" SQL_SINGLE_QUOTE_TOKEN: str = "'" -SQL_QUOTE_TOKENS: frozenset[str] = frozenset({"'", '"', "`"}) +SQL_QUOTE_TOKENS: frozenset[str] = frozenset({"'", '"', "`", "$"}) SQL_QUALIFIER_SEPARATOR_TOKEN: str = "." SQL_REFERENCE_NAME_QUOTE_TOKENS: frozenset[str] = frozenset({"'", '"'}) SQL_IDENTIFIER_EXTRA_TOKEN: str = "_" diff --git a/src/sqlbuild/compiler/planner/_helpers/resolve/refs.py b/src/sqlbuild/compiler/planner/_helpers/resolve/refs.py index 0d00938244..ddd53bdd28 100644 --- a/src/sqlbuild/compiler/planner/_helpers/resolve/refs.py +++ b/src/sqlbuild/compiler/planner/_helpers/resolve/refs.py @@ -47,7 +47,7 @@ _TABLE_FUNCTION_PATTERN: re.Pattern[str] = quoted_reference_call_pattern( SqlReferenceKind.TABLE_FUNCTION ) -_SQL_QUOTE_TOKENS: frozenset[str] = frozenset({"'", '"', "`"}) +_SQL_QUOTE_TOKENS: frozenset[str] = frozenset({"'", '"', "`", "$"}) _CLAUSE_KEYWORDS: frozenset[str] = frozenset( { "WHERE", diff --git a/src/sqlbuild/compiler/references/_helpers/resolution.py b/src/sqlbuild/compiler/references/_helpers/resolution.py index 53c50412ed..5e0a1fae62 100644 --- a/src/sqlbuild/compiler/references/_helpers/resolution.py +++ b/src/sqlbuild/compiler/references/_helpers/resolution.py @@ -25,7 +25,7 @@ _UNRESOLVED_TABLE_FUNCTION_PATTERN: re.Pattern[str] = re.compile( reference_call_prefix_pattern_text(SqlReferenceKind.TABLE_FUNCTION) ) -_SQL_QUOTE_TOKENS: frozenset[str] = frozenset({"'", '"', "`"}) +_SQL_QUOTE_TOKENS: frozenset[str] = frozenset({"'", '"', "`", "$"}) def assert_no_unresolved_sql_markers(*, sql: str, context: str) -> None: diff --git a/src/sqlbuild/compiler/sql_analysis/_helpers/scanning.py b/src/sqlbuild/compiler/sql_analysis/_helpers/scanning.py index bfd1320426..9dae55936e 100644 --- a/src/sqlbuild/compiler/sql_analysis/_helpers/scanning.py +++ b/src/sqlbuild/compiler/sql_analysis/_helpers/scanning.py @@ -8,19 +8,30 @@ from sqlbuild.compiler.compile.exceptions import CompileInputError from sqlbuild.compiler.sql_analysis.constants import ( SQL_CLOSE_PARENTHESIS, + SQL_DOLLAR_QUOTE_CHARACTER, SQL_ESCAPABLE_QUOTE_CHARACTERS, SQL_IDENTIFIER_PREFIX, SQL_OPEN_PARENTHESIS, - SQL_QUOTE_CHARACTERS, + SQL_TEXT_START_CHARACTERS, ) -_PAREN_SCAN_SPECIAL: re.Pattern[str] = re.compile(r"[-/'\"`()]") +_PAREN_SCAN_SPECIAL: re.Pattern[str] = re.compile(r"[-/'\"`$()]") +_DOLLAR_QUOTE_DELIMITER: re.Pattern[str] = re.compile(r"\$(?:[A-Za-z_][A-Za-z0-9_]*)?\$") def skip_quoted_text_impl(*, sql: str, start: int, context: str = "SQL") -> int: - """Skip past quoted SQL text starting at the supplied position.""" + """Skip quoted text at the position, consuming a non-opening ``$`` as one code character.""" + + end: int | None = quoted_text_end_impl(sql=sql, start=start, context=context) + return start + 1 if end is None else end + + +def quoted_text_end_impl(*, sql: str, start: int, context: str = "SQL") -> int | None: + """Return the end of quoted or dollar-quoted text starting at the position, if any.""" quote_character: str = sql[start] + if quote_character == SQL_DOLLAR_QUOTE_CHARACTER: + return _dollar_quoted_text_end(sql=sql, start=start, context=context) index: int = start + 1 while True: index = sql.find(quote_character, index) @@ -36,6 +47,24 @@ def skip_quoted_text_impl(*, sql: str, start: int, context: str = "SQL") -> int: return index + 1 +def _dollar_quoted_text_end(*, sql: str, start: int, context: str) -> int | None: + delimiter: re.Match[str] | None = _DOLLAR_QUOTE_DELIMITER.match(sql, start) + if delimiter is None or (start > 0 and _continues_dollar_word(sql[start - 1])): + return None + closing_index: int = sql.find(delimiter.group(), delimiter.end()) + if closing_index == -1: + raise CompileInputError(f"{context} contains an unclosed quoted string") + return closing_index + len(delimiter.group()) + + +def _continues_dollar_word(character: str) -> bool: + return ( + not character.isascii() + or character.isalnum() + or character in {SQL_IDENTIFIER_PREFIX, SQL_DOLLAR_QUOTE_CHARACTER} + ) + + def skip_line_comment_impl(*, sql: str, start: int) -> int: """Skip past an SQL line comment.""" @@ -62,14 +91,9 @@ def find_matching_paren_impl(*, sql: str, open_paren_index: int, context: str = if special is None: break index = special.start() - if sql.startswith("--", index): - index = skip_line_comment_impl(sql=sql, start=index) - continue - if sql.startswith("/*", index): - index = skip_block_comment_impl(sql=sql, start=index, context=context) - continue - if sql[index] in SQL_QUOTE_CHARACTERS: - index = skip_quoted_text_impl(sql=sql, start=index, context=context) + non_code_end: int | None = _non_code_end(sql=sql, index=index, context=context) + if non_code_end is not None: + index = non_code_end continue if sql[index] == SQL_OPEN_PARENTHESIS: depth += 1 @@ -87,16 +111,11 @@ def iter_code_positions_impl(*, sql: str, context: str = "SQL") -> Iterator[tupl depth: int = 0 index: int = 0 while index < len(sql): - if sql.startswith("--", index): - index = skip_line_comment_impl(sql=sql, start=index) - continue - if sql.startswith("/*", index): - index = skip_block_comment_impl(sql=sql, start=index, context=context) + non_code_end: int | None = _non_code_end(sql=sql, index=index, context=context) + if non_code_end is not None: + index = non_code_end continue character: str = sql[index] - if character in SQL_QUOTE_CHARACTERS: - index = skip_quoted_text_impl(sql=sql, start=index, context=context) - continue if character == SQL_OPEN_PARENTHESIS: depth += 1 elif character == SQL_CLOSE_PARENTHESIS: @@ -106,6 +125,16 @@ def iter_code_positions_impl(*, sql: str, context: str = "SQL") -> Iterator[tupl index += 1 +def _non_code_end(*, sql: str, index: int, context: str) -> int | None: + if sql.startswith("--", index): + return skip_line_comment_impl(sql=sql, start=index) + if sql.startswith("/*", index): + return skip_block_comment_impl(sql=sql, start=index, context=context) + if sql[index] in SQL_TEXT_START_CHARACTERS: + return quoted_text_end_impl(sql=sql, start=index, context=context) + return None + + def is_identifier_start_impl(character: str) -> bool: """Return whether a character can begin an SQL identifier.""" diff --git a/src/sqlbuild/compiler/sql_analysis/constants.py b/src/sqlbuild/compiler/sql_analysis/constants.py index 6724ecbc59..8bba4fab97 100644 --- a/src/sqlbuild/compiler/sql_analysis/constants.py +++ b/src/sqlbuild/compiler/sql_analysis/constants.py @@ -4,6 +4,7 @@ SQL_ESCAPABLE_QUOTE_CHARACTERS: frozenset[str] = frozenset({"'", '"'}) SQL_QUOTE_CHARACTERS: frozenset[str] = frozenset({*SQL_ESCAPABLE_QUOTE_CHARACTERS, "`"}) +SQL_TEXT_START_CHARACTERS: frozenset[str] = frozenset({*SQL_QUOTE_CHARACTERS, "$"}) SQL_OPEN_PARENTHESIS: str = "(" SQL_CLOSE_PARENTHESIS: str = ")" SQL_OPEN_BRACKET: str = "[" @@ -11,6 +12,7 @@ SQL_STRING_QUOTE_CHARACTER: str = "'" SQL_ESCAPE_CHARACTER: str = "\\" SQL_DOLLAR_QUOTE_DELIMITER: str = "$$" +SQL_DOLLAR_QUOTE_CHARACTER: str = "$" SQL_IDENTIFIER_PREFIX: str = "_" SQL_NULL_KEYWORD: str = "NULL" SNOWFLAKE_DIALECT_NAME: str = "snowflake" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index f10b29960f..522599eb7f 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -71,6 +71,15 @@ class RulesIntegrationTestCase: expected_code: str +@dataclass(frozen=True) +class DollarQuotedLiteralBuildTestCase: + """One model projection with dollar-quoted literals and its built rows.""" + + description: str + projection: str + expected_rows: tuple[tuple[object, ...], ...] + + @dataclass(frozen=True) class ImplicitAliasRuleIntegrationTestCase: """One model query and its expected unused-alias finding locations.""" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_dollar_quoted_literals.py b/tests/integration/src/sqlbuild/cli/commands/main/test_dollar_quoted_literals.py new file mode 100644 index 0000000000..3eae8372df --- /dev/null +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_dollar_quoted_literals.py @@ -0,0 +1,66 @@ +"""Integration coverage for dollar-quoted string literals through the real CLI.""" + +from __future__ import annotations + +from pathlib import Path + +import duckdb +import pytest + +from tests.integration.src.sqlbuild.cli.commands.main._test_types import ( + DollarQuotedLiteralBuildTestCase, +) +from tests.integration.src.sqlbuild.cli.commands.main.helpers import run_build + + +@pytest.mark.parametrize( + "test_case", + [ + DollarQuotedLiteralBuildTestCase( + description="dollar-quoted literals stay opaque while real references build", + projection=( + '$$Customer\'s order -- __ref("missing_orders") costs $5$$ AS order_label,\n' + ' $note$ $$ nested __seed("missing_seed") @missing_macro() $note$ AS order_note,\n' + " o.order_id" + ), + expected_rows=( + ( + 'Customer\'s order -- __ref("missing_orders") costs $5', + ' $$ nested __seed("missing_seed") @missing_macro() ', + 1, + ), + ), + ) + ], + ids=lambda case: case.description, +) +def test_given_dollar_quoted_literal_when_building_then_literal_is_preserved_and_refs_resolve( + test_case: DollarQuotedLiteralBuildTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n\n[connection]\ndatabase = "orders.duckdb"\n', + encoding="utf-8", + ) + models: Path = tmp_path / "models" + models.mkdir() + (models / "orders.sql").write_text( + 'MODEL (description "Orders", materialized table);\nSELECT 1 AS order_id\n', + encoding="utf-8", + ) + (models / "order_labels.sql").write_text( + 'MODEL (description "Order labels", materialized table);\n' + f"SELECT\n {test_case.projection}\n" + 'FROM __ref("orders") AS o\n', + encoding="utf-8", + ) + + exit_code, output = run_build(project_dir=tmp_path, flags=(), capsys=capsys) + + assert exit_code == 0, output + with duckdb.connect(str(tmp_path / "orders.duckdb"), read_only=True) as connection: + rows: list[tuple[object, ...]] = connection.execute( + "SELECT order_label, order_note, order_id FROM main.order_labels" + ).fetchall() + assert tuple(rows) == test_case.expected_rows diff --git a/tests/unit/src/sqlbuild/compiler/compile/_helpers/test_sql_references.py b/tests/unit/src/sqlbuild/compiler/compile/_helpers/test_sql_references.py index 9084472122..4d13a8b42b 100644 --- a/tests/unit/src/sqlbuild/compiler/compile/_helpers/test_sql_references.py +++ b/tests/unit/src/sqlbuild/compiler/compile/_helpers/test_sql_references.py @@ -27,7 +27,24 @@ (SqlReferenceKind.SOURCE, "orders", None), (SqlReferenceKind.DBT_REF, "customers", "shop"), ), - ) + ), + SqlReferenceExtractionTestCase( + description="dollar-quoted text hides embedded references", + sql=( + 'SELECT $$Customer\'s order -- __ref("ignored") $5$$ AS order_label, ' + "$note$ $$ __seed('ignored') $note$ AS order_note " + 'FROM __ref("orders")' + ), + expected_references=((SqlReferenceKind.REF, "orders", None),), + ), + SqlReferenceExtractionTestCase( + description="general scanner hides references in dollar-quoted text", + sql=( + 'SELECT $$Customer\'s order __ref("ignored")$$ AS order_label ' + 'FROM __table_fn("expand_orders")(1)' + ), + expected_references=((SqlReferenceKind.TABLE_FUNCTION, "expand_orders", None),), + ), ], ids=lambda case: case.description, ) @@ -68,6 +85,11 @@ def test_given_simple_references_when_extracting_then_returns_authored_order( sql='SELECT * FROM __ref("orders") WHERE note = \'unterminated', expected_error="unclosed quoted string", ), + SqlReferenceExtractionErrorTestCase( + description="unclosed dollar-quoted string preserves diagnostic", + sql='SELECT * FROM __ref("orders") WHERE note = $$customer\'s', + expected_error="unclosed quoted string", + ), ], ids=lambda case: case.description, ) diff --git a/tests/unit/src/sqlbuild/compiler/sql_analysis/_helpers/test_scanning.py b/tests/unit/src/sqlbuild/compiler/sql_analysis/_helpers/test_scanning.py index 3245e9b7aa..b57f732b56 100644 --- a/tests/unit/src/sqlbuild/compiler/sql_analysis/_helpers/test_scanning.py +++ b/tests/unit/src/sqlbuild/compiler/sql_analysis/_helpers/test_scanning.py @@ -42,6 +42,21 @@ quoted_sql="'a''''b'", expected_end=len("'a''''b'"), ), + SkipQuotedTextSuccessTestCase( + description="dollar quote hides apostrophes, comments and single dollars", + quoted_sql="$$customer's order -- $5 $$", + expected_end=len("$$customer's order -- $5 $$"), + ), + SkipQuotedTextSuccessTestCase( + description="tagged dollar quote ends only at its own tag", + quoted_sql="$order$ $$ it's $order$", + expected_end=len("$order$ $$ it's $order$"), + ), + SkipQuotedTextSuccessTestCase( + description="positional parameter dollar is one code character", + quoted_sql="$1", + expected_end=1, + ), ), ids=lambda case: case.description, ) @@ -64,6 +79,12 @@ def test_given_quoted_text_when_skipping_then_returns_end_position( context="SQL reference", expected_error="SQL reference contains an unclosed quoted string", ), + SkipQuotedTextErrorTestCase( + description="unclosed dollar quote", + sql="$order$ customer's $$", + context="SQL reference", + expected_error="SQL reference contains an unclosed quoted string", + ), ), ids=lambda case: case.description, ) @@ -87,6 +108,16 @@ def test_given_unclosed_quote_when_skipping_then_raises_contextual_error( sql="a'(b'`c`--d\n/*e*/f", expected_positions=((0, 0), (17, 0)), ), + IterCodePositionsTestCase( + description="dollar-quoted text is skipped", + sql="a $$'(--$$b", + expected_positions=((0, 0), (1, 0), (10, 0)), + ), + IterCodePositionsTestCase( + description="dollars inside identifiers and parameters remain code", + sql="a$$b $1", + expected_positions=tuple((index, 0) for index in range(7)), + ), IterCodePositionsTestCase( description="unbalanced close parenthesis goes below zero depth", sql=")a", From 652024518a0a1c59366abbdc042ab9d5eca36497 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 27 Sep 2026 03:20:17 +0100 Subject: [PATCH 4/8] fix: report sqb query warehouse errors as CLI errors A warehouse error from sqb query escaped the CLI error projection and printed a Python traceback. Query execution failures now raise a coded CLI error, error[C108], carrying the warehouse message on stderr with exit code 2 and no stdout output. Refs CHI-343 --- src/sqlbuild/cli/commands/exceptions.py | 7 ++ .../cli/commands/main/inspection/_query.py | 7 +- .../cli/commands/main/query/_test_types.py | 8 ++ .../cli/commands/main/query/test_query.py | 82 +++++++++++++++++++ .../main/inspection/query/_test_types.py | 12 +++ .../main/inspection/query/test_query.py | 46 +++++++++++ website/src/content/docs/docs/cli/query.mdx | 7 ++ 7 files changed, 168 insertions(+), 1 deletion(-) create mode 100644 tests/unit/src/sqlbuild/cli/commands/main/inspection/query/_test_types.py create mode 100644 tests/unit/src/sqlbuild/cli/commands/main/inspection/query/test_query.py diff --git a/src/sqlbuild/cli/commands/exceptions.py b/src/sqlbuild/cli/commands/exceptions.py index 29f3d0277f..0294f1403e 100644 --- a/src/sqlbuild/cli/commands/exceptions.py +++ b/src/sqlbuild/cli/commands/exceptions.py @@ -15,6 +15,13 @@ def __init__(self, message: str, *, code: str | None = None, help: str | None = self.help = help +class QueryExecutionError(CliUserError): + """The warehouse rejected or failed an ad hoc query.""" + + code: str = "C108" + exit_code: int = 2 + + class QueryDiffOutcomeError(CliUserError): """Expected query-diff failure with an explicit outcome and exit code.""" diff --git a/src/sqlbuild/cli/commands/main/inspection/_query.py b/src/sqlbuild/cli/commands/main/inspection/_query.py index 0d6c0f8a33..807a9eda40 100644 --- a/src/sqlbuild/cli/commands/main/inspection/_query.py +++ b/src/sqlbuild/cli/commands/main/inspection/_query.py @@ -6,13 +6,14 @@ from pathlib import Path from sqlbuild.adapter.contract.classes.base_adapter import BaseAdapter +from sqlbuild.adapter.contract.exceptions import AdapterUserError from sqlbuild.adapter.contract.models import QueryResult from sqlbuild.cli.commands._helpers.query.output import render_query_result from sqlbuild.cli.commands._helpers.runtime.adapters import resolve_adapter from sqlbuild.cli.commands._helpers.runtime.connection import ( resolve_project_connection_config, ) -from sqlbuild.cli.commands.exceptions import CliUserError +from sqlbuild.cli.commands.exceptions import CliUserError, QueryExecutionError from sqlbuild.compiler.discovery.main.discover import discover_project_inputs from sqlbuild.compiler.discovery.models import DiscoveredProjectInputs from sqlbuild.spec.contracts.main.resolve_effective_adapter_name import ( @@ -58,6 +59,10 @@ def run_query( connection: object = adapter.connect(connection_config) try: result: QueryResult = adapter.query(connection=connection, sql=query_sql, limit=limit) + except AdapterUserError: + raise + except Exception as error: + raise QueryExecutionError(str(getattr(error, "message", error))) from error finally: adapter.close(connection) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/query/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/query/_test_types.py index 316f92543c..42d64cf8f6 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/query/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/query/_test_types.py @@ -24,3 +24,11 @@ class QueryFileErrorCliTestCase: expected_stderr_fragment: str repo_files: dict[str, str] = field(default_factory=dict) binary_files: dict[str, bytes] = field(default_factory=dict) + + +@dataclass(frozen=True) +class QueryExecutionErrorCliTestCase: + description: str + command: tuple[str, ...] + expected_stderr_fragment: str + expected_returncode: int = 2 diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/query/test_query.py b/tests/e2e/src/sqlbuild/cli/commands/main/query/test_query.py index 61c0888703..b87431f535 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/query/test_query.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/query/test_query.py @@ -7,6 +7,7 @@ from tests.e2e.src.sqlbuild.cli.commands.main.query._test_types import ( QueryCliTestCase, + QueryExecutionErrorCliTestCase, QueryFileCliTestCase, QueryFileErrorCliTestCase, ) @@ -164,3 +165,84 @@ def test_given_invalid_query_file_input_when_running_query_then_reports_actionab assert result.returncode == 1 assert test_case.expected_stderr_fragment in result.stderr + + +@pytest.mark.parametrize( + "test_case", + ( + QueryExecutionErrorCliTestCase( + description="reports a missing relation as a CLI error", + command=("query", "SELECT * FROM missing_orders"), + expected_stderr_fragment=( + "error[C108]: Catalog Error: Table with name missing_orders does not exist!" + ), + ), + QueryExecutionErrorCliTestCase( + description="reports a syntax error as a CLI error", + command=("query", "SELECT FROM WHERE"), + expected_stderr_fragment="error[C108]: Parser Error: syntax error", + ), + ), + ids=lambda case: case.description, +) +def test_given_warehouse_rejects_query_when_running_query_then_reports_cli_error( + tmp_path: Path, + test_case: QueryExecutionErrorCliTestCase, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="query_error_project", + repo_files={ + "sqlbuild_project.toml": ( + 'name = "query_error_project"\nadapter = "duckdb"\n\n' + '[connection]\ndatabase = "query.duckdb"\n' + ), + }, + ) + + result: subprocess.CompletedProcess[str] = run_sqb( + command=test_case.command, + project_dir=project_dir, + ) + + assert result.returncode == test_case.expected_returncode + assert test_case.expected_stderr_fragment in result.stderr + assert "Traceback" not in result.stdout + result.stderr + + +@pytest.mark.parametrize( + "test_case", + ( + QueryExecutionErrorCliTestCase( + description="keeps machine-readable stdout empty when the query fails", + command=("query", "SELECT * FROM missing_orders", "--format", "json"), + expected_stderr_fragment=( + "error[C108]: Catalog Error: Table with name missing_orders does not exist!" + ), + ), + ), + ids=lambda case: case.description, +) +def test_given_json_query_fails_when_running_query_then_stdout_stays_empty( + tmp_path: Path, + test_case: QueryExecutionErrorCliTestCase, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="query_json_error_project", + repo_files={ + "sqlbuild_project.toml": ( + 'name = "query_json_error_project"\nadapter = "duckdb"\n\n' + '[connection]\ndatabase = "query.duckdb"\n' + ), + }, + ) + + result: subprocess.CompletedProcess[str] = run_sqb( + command=test_case.command, + project_dir=project_dir, + ) + + assert result.returncode == test_case.expected_returncode + assert test_case.expected_stderr_fragment in result.stderr + assert result.stdout == "" diff --git a/tests/unit/src/sqlbuild/cli/commands/main/inspection/query/_test_types.py b/tests/unit/src/sqlbuild/cli/commands/main/inspection/query/_test_types.py new file mode 100644 index 0000000000..3cb87d14c1 --- /dev/null +++ b/tests/unit/src/sqlbuild/cli/commands/main/inspection/query/_test_types.py @@ -0,0 +1,12 @@ +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass(frozen=True) +class QueryExecutionErrorTestCase: + description: str + sql: str + expected_message_fragment: str + expected_code: str = "C108" + expected_exit_code: int = 2 diff --git a/tests/unit/src/sqlbuild/cli/commands/main/inspection/query/test_query.py b/tests/unit/src/sqlbuild/cli/commands/main/inspection/query/test_query.py new file mode 100644 index 0000000000..16e03d9725 --- /dev/null +++ b/tests/unit/src/sqlbuild/cli/commands/main/inspection/query/test_query.py @@ -0,0 +1,46 @@ +from __future__ import annotations + +from pathlib import Path + +import duckdb +import pytest + +from sqlbuild.cli.commands.exceptions import QueryExecutionError +from sqlbuild.cli.commands.main.inspection._query import run_query +from tests.unit.src.sqlbuild.cli.commands.main.inspection.query._test_types import ( + QueryExecutionErrorTestCase, +) + + +@pytest.mark.parametrize( + "test_case", + [ + QueryExecutionErrorTestCase( + description="missing relation", + sql="SELECT * FROM missing_orders", + expected_message_fragment="Table with name missing_orders does not exist", + ), + QueryExecutionErrorTestCase( + description="syntax error", + sql="SELECT FROM WHERE", + expected_message_fragment="syntax error", + ), + ], + ids=lambda case: case.description, +) +def test_given_warehouse_rejects_query_when_running_query_then_raises_cli_error( + test_case: QueryExecutionErrorTestCase, + tmp_path: Path, +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n\n[connection]\ndatabase = "orders.duckdb"\n', + encoding="utf-8", + ) + + with pytest.raises(QueryExecutionError) as raised: + run_query(project_dir=tmp_path, sql=test_case.sql) + + assert raised.value.code == test_case.expected_code + assert raised.value.exit_code == test_case.expected_exit_code + assert test_case.expected_message_fragment in raised.value.message + assert isinstance(raised.value.__cause__, duckdb.Error) diff --git a/website/src/content/docs/docs/cli/query.mdx b/website/src/content/docs/docs/cli/query.mdx index 4f5fea8cba..77334b72d9 100644 --- a/website/src/content/docs/docs/cli/query.mdx +++ b/website/src/content/docs/docs/cli/query.mdx @@ -82,6 +82,13 @@ order_id,customer_id,status 2,200,completed ``` +## Errors + +When the warehouse rejects the SQL, `sqb query` prints the warehouse message as +`error[C108]: ...` on stderr and exits with code `2`. Nothing is written to stdout, so +redirected `json` or `csv` output is never partial. Invalid command input, such as a missing +query file, exits with code `1`. + ## Examples ```bash From a2787ccc94f744745e83923c8adbdcec555ae57e Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 27 Sep 2026 03:29:00 +0100 Subject: [PATCH 5/8] fix: skip type-proof rules when SQL analysis is disabled SQBRCONTRACT105 needs inferred type proofs, which only exist when SQL analysis runs, so compile --no-sql-analysis (or sql_analysis = false) reported it for essentially every typed contract column. Type-proof rules are now not evaluated when the run or project disables SQL analysis, and compile prints one note naming the skipped rules on stderr before the final summary. With analysis enabled, behaviour is unchanged. Refs CHI-353 --- .../cli/commands/_helpers/compile/pipeline.py | 2 + .../cli/commands/_helpers/rules/command.py | 6 + .../cli/commands/main/project/_compile.py | 7 + src/sqlbuild/cli/compile/models.py | 1 + .../compiler/pipeline/main/compile.py | 6 + .../rule_engine/_helpers/engine/native.py | 6 +- .../rule_engine/_helpers/run/rules.py | 9 ++ src/sqlbuild/rule_engine/constants.py | 1 + .../rule_engine/main/render_skipped_rules.py | 9 ++ src/sqlbuild/rule_engine/main/run_rules.py | 2 + src/sqlbuild/rule_engine/models.py | 1 + .../cli/commands/main/compile/_test_types.py | 10 ++ .../compile/test_compile_type_proof_rules.py | 141 ++++++++++++++++++ .../main/render_skipped_rules/_test_types.py | 10 ++ .../test_render_skipped_rules.py | 32 ++++ .../rule_engine/main/run_rules/_test_types.py | 12 ++ .../main/run_rules/test_run_rules.py | 87 +++++++++++ website/src/content/docs/docs/cli/compile.mdx | 4 +- 18 files changed, 343 insertions(+), 3 deletions(-) create mode 100644 src/sqlbuild/rule_engine/main/render_skipped_rules.py create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/compile/test_compile_type_proof_rules.py create mode 100644 tests/unit/src/sqlbuild/rule_engine/main/render_skipped_rules/_test_types.py create mode 100644 tests/unit/src/sqlbuild/rule_engine/main/render_skipped_rules/test_render_skipped_rules.py create mode 100644 tests/unit/src/sqlbuild/rule_engine/main/run_rules/_test_types.py create mode 100644 tests/unit/src/sqlbuild/rule_engine/main/run_rules/test_run_rules.py diff --git a/src/sqlbuild/cli/commands/_helpers/compile/pipeline.py b/src/sqlbuild/cli/commands/_helpers/compile/pipeline.py index 18556cfe0e..cb4bf124ed 100644 --- a/src/sqlbuild/cli/commands/_helpers/compile/pipeline.py +++ b/src/sqlbuild/cli/commands/_helpers/compile/pipeline.py @@ -210,6 +210,7 @@ def _analyze_compile_project( selected_keys=selected_keys if select or exclude else None, prepared_sql=early_lint.preparation, expansion_reuse=early_lint.expansion_reuse, + no_sql_analysis=no_sql_validation, ) _ = complete_compile_phase( status=status, @@ -246,6 +247,7 @@ def _analyze_compile_project( custom_rules_ms=rules_result.custom_ms, rule_cache_hits=rules_result.cache_hits, rule_cache_misses=rules_result.cache_misses, + skipped_type_proof_rules=rules_result.skipped_type_proof_rules, ) diff --git a/src/sqlbuild/cli/commands/_helpers/rules/command.py b/src/sqlbuild/cli/commands/_helpers/rules/command.py index d04fd2d6f3..cf926e2834 100644 --- a/src/sqlbuild/cli/commands/_helpers/rules/command.py +++ b/src/sqlbuild/cli/commands/_helpers/rules/command.py @@ -25,6 +25,7 @@ from sqlbuild.rule_engine.main.load_config import load_rules_config from sqlbuild.rule_engine.main.render_result import format_result from sqlbuild.rule_engine.main.render_rule import format_rule +from sqlbuild.rule_engine.main.render_skipped_rules import format_skipped_type_proof_rules from sqlbuild.rule_engine.main.run_rules import run_rules from sqlbuild.rule_engine.main.skills import install_skills from sqlbuild.rule_engine.models import Rule, RulesConfig, RulesResult, RulesRunResult @@ -151,6 +152,11 @@ def _run_rule_selection( dialect=adapter.sql_analysis_dialect() or "generic", selected_keys=selected_keys if request.select or request.exclude else None, ) + skipped_rules_note: str | None = format_skipped_type_proof_rules( + codes=result.skipped_type_proof_rules + ) + if skipped_rules_note is not None: + print(f"note: {skipped_rules_note}", file=sys.stderr) print( format_result( result=RulesResult( diff --git a/src/sqlbuild/cli/commands/main/project/_compile.py b/src/sqlbuild/cli/commands/main/project/_compile.py index 049201aa3f..7818c3dd77 100644 --- a/src/sqlbuild/cli/commands/main/project/_compile.py +++ b/src/sqlbuild/cli/commands/main/project/_compile.py @@ -2,6 +2,7 @@ from __future__ import annotations +import sys import time from dataclasses import replace from pathlib import Path @@ -31,6 +32,7 @@ from sqlbuild.compiler.profiling.models import CompileTimingCollector from sqlbuild.presentation.classes.transient_status_reporter import TransientStatusReporter from sqlbuild.presentation.main.supports_color import supports_color +from sqlbuild.rule_engine.main.render_skipped_rules import format_skipped_type_proof_rules def run_compile(request: CompileCommandRequest) -> int: @@ -143,6 +145,11 @@ def _run_compile_with_status( if status is not None: status.close() + skipped_rules_note: str | None = format_skipped_type_proof_rules( + codes=analysis.skipped_type_proof_rules + ) + if skipped_rules_note is not None: + print(f"note: {skipped_rules_note}", file=sys.stderr) if json_output: print( diff --git a/src/sqlbuild/cli/compile/models.py b/src/sqlbuild/cli/compile/models.py index 98bbc05424..7a177ea7db 100644 --- a/src/sqlbuild/cli/compile/models.py +++ b/src/sqlbuild/cli/compile/models.py @@ -62,6 +62,7 @@ class CompileAnalysis: custom_rules_ms: int = 0 rule_cache_hits: int = 0 rule_cache_misses: int = 0 + skipped_type_proof_rules: tuple[str, ...] = () @dataclass(frozen=True) diff --git a/src/sqlbuild/compiler/pipeline/main/compile.py b/src/sqlbuild/compiler/pipeline/main/compile.py index 14feaead0a..91f2e04283 100644 --- a/src/sqlbuild/compiler/pipeline/main/compile.py +++ b/src/sqlbuild/compiler/pipeline/main/compile.py @@ -64,6 +64,7 @@ from sqlbuild.rule_engine.exceptions import RulesError from sqlbuild.rule_engine.main.load_config import load_rules_config from sqlbuild.rule_engine.main.render_result import format_result +from sqlbuild.rule_engine.main.render_skipped_rules import format_skipped_type_proof_rules from sqlbuild.rule_engine.main.run_rules import run_rules from sqlbuild.rule_engine.models import RulesCacheConfig, RulesConfig, RulesResult, RulesRunResult from sqlbuild.runtime.contracts.main.open_connection import open_connection_with_hooks @@ -114,11 +115,16 @@ def run_compile_pipeline( discovered_inputs=discovered_inputs, project=project, ), + no_sql_analysis=resolved_options.no_sql_validation, ) if on_progress is not None: + skipped_rules_note: str | None = format_skipped_type_proof_rules( + codes=rules_result.skipped_type_proof_rules + ) on_progress( f"Evaluated rules. (built-in {rules_result.built_in_ms / 1000:.2f}s, " f"custom {rules_result.custom_ms / 1000:.2f}s)" + + ("" if skipped_rules_note is None else f"; {skipped_rules_note}") ) if rules_result.findings: rendered: str = format_result( diff --git a/src/sqlbuild/rule_engine/_helpers/engine/native.py b/src/sqlbuild/rule_engine/_helpers/engine/native.py index 51ec3a3b6a..30dccae1a4 100644 --- a/src/sqlbuild/rule_engine/_helpers/engine/native.py +++ b/src/sqlbuild/rule_engine/_helpers/engine/native.py @@ -48,6 +48,7 @@ RULE_CONTEXT_PROJECT_FACT, RULE_CONTEXT_TESTS_FACT, RULES_NATIVE_API_VERSION, + TYPE_PROOF_RULE_CODES, ) from sqlbuild.rule_engine.exceptions import RulesError from sqlbuild.rule_engine.models import ( @@ -61,7 +62,6 @@ _CUSTOM_HOST_REQUIRED: str = "selected custom rules require a custom host" _NATIVE_CACHE_MISSES_PATTERN: re.Pattern[str] = re.compile(r"native_cache_misses=(\d+)") -_EXPLICIT_OUTPUT_TYPES_RULE: str = "SQBRCONTRACT105" def evaluate_native( @@ -89,7 +89,9 @@ def evaluate_native( "models": _model_payloads( project=project, dialect=dialect, - include_type_proof=_rule_selected(config=config, code=_EXPLICIT_OUTPUT_TYPES_RULE), + include_type_proof=any( + _rule_selected(config=config, code=code) for code in TYPE_PROOF_RULE_CODES + ), ), "sql_tests": _sql_test_payloads(project), "sql_scenarios": _sql_scenario_payloads(project), diff --git a/src/sqlbuild/rule_engine/_helpers/run/rules.py b/src/sqlbuild/rule_engine/_helpers/run/rules.py index 3979bca495..e9a3004feb 100644 --- a/src/sqlbuild/rule_engine/_helpers/run/rules.py +++ b/src/sqlbuild/rule_engine/_helpers/run/rules.py @@ -38,6 +38,7 @@ finalize_native_findings, native_catalogue, ) +from sqlbuild.rule_engine.constants import TYPE_PROOF_RULE_CODES from sqlbuild.rule_engine.exceptions import RulesError from sqlbuild.rule_engine.main.load_config import load_rules_config from sqlbuild.rule_engine.models import ( @@ -73,6 +74,7 @@ def evaluate_rules( selected_keys: frozenset[CompiledObjectKey] | None = None, prepared_sql: PreparedSqlLint | None = None, expansion_reuse: SqlExpansionReuse | None = None, + no_sql_analysis: bool = False, ) -> RulesRunResult: """Evaluate independent rule phases concurrently, then finalize their combined findings.""" effective_config: RulesConfig = resolve_rule_ignore_selectors( @@ -89,6 +91,12 @@ def evaluate_rules( selected: tuple[Rule, ...] = select_rules( catalogue=catalogue, config=effective_config, project_dir=resolved_project_dir ) + skipped_type_proof_rules: tuple[str, ...] = () + if no_sql_analysis or not graph.project.settings.sql_analysis: + skipped_type_proof_rules = tuple( + rule.code for rule in selected if rule.code in TYPE_PROOF_RULE_CODES + ) + selected = tuple(rule for rule in selected if rule.code not in TYPE_PROOF_RULE_CODES) native_rules: tuple[Rule, ...] = tuple(rule for rule in selected if not rule.custom) custom_rules: tuple[Rule, ...] = tuple(rule for rule in selected if rule.custom) if custom_rules: @@ -149,6 +157,7 @@ def evaluate_rules( custom_ms=result.custom_ms, cache_hits=result.cache_hits + sql_result.cache_hits, cache_misses=result.cache_misses + sql_result.cache_misses, + skipped_type_proof_rules=skipped_type_proof_rules, ) diff --git a/src/sqlbuild/rule_engine/constants.py b/src/sqlbuild/rule_engine/constants.py index ed7084a29b..3561e0ae4a 100644 --- a/src/sqlbuild/rule_engine/constants.py +++ b/src/sqlbuild/rule_engine/constants.py @@ -22,6 +22,7 @@ CONTRACT_TIMESTAMP_NAME_CODE: str = "SQBRCONTRACT103" CONTRACT_DATE_NAME_CODE: str = "SQBRCONTRACT104" EMPTY_INPUT_TEST_CODE: str = "SQBRTEST203" +TYPE_PROOF_RULE_CODES: frozenset[str] = frozenset({"SQBRCONTRACT105"}) BOOLEAN_TYPE: str = "BOOLEAN" DATE_TYPE: str = "DATE" diff --git a/src/sqlbuild/rule_engine/main/render_skipped_rules.py b/src/sqlbuild/rule_engine/main/render_skipped_rules.py new file mode 100644 index 0000000000..f883250905 --- /dev/null +++ b/src/sqlbuild/rule_engine/main/render_skipped_rules.py @@ -0,0 +1,9 @@ +"""Skipped type-proof rules note entrypoint.""" + + +def format_skipped_type_proof_rules(*, codes: tuple[str, ...]) -> str | None: + """Describe type-proof rules skipped because SQL analysis is disabled, if any.""" + + if not codes: + return None + return f"skipped type-proof rules ({', '.join(codes)}) because SQL analysis is disabled" diff --git a/src/sqlbuild/rule_engine/main/run_rules.py b/src/sqlbuild/rule_engine/main/run_rules.py index 46b7136910..c5ecda7344 100644 --- a/src/sqlbuild/rule_engine/main/run_rules.py +++ b/src/sqlbuild/rule_engine/main/run_rules.py @@ -24,6 +24,7 @@ def run_rules( selected_keys: frozenset[CompiledObjectKey] | None = None, prepared_sql: PreparedSqlLint | None = None, expansion_reuse: SqlExpansionReuse | None = None, + no_sql_analysis: bool = False, ) -> RulesRunResult: """Run selected native built-ins before selected custom Python rules.""" @@ -36,4 +37,5 @@ def run_rules( selected_keys=selected_keys, prepared_sql=prepared_sql, expansion_reuse=expansion_reuse, + no_sql_analysis=no_sql_analysis, ) diff --git a/src/sqlbuild/rule_engine/models.py b/src/sqlbuild/rule_engine/models.py index a05beb6bfe..f3651eef92 100644 --- a/src/sqlbuild/rule_engine/models.py +++ b/src/sqlbuild/rule_engine/models.py @@ -341,6 +341,7 @@ class RulesRunResult: custom_ms: int cache_hits: int = 0 cache_misses: int = 0 + skipped_type_proof_rules: tuple[str, ...] = () @dataclass(frozen=True) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/_test_types.py index 4fc2e65bff..b2220b0da7 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/_test_types.py @@ -227,3 +227,13 @@ class EmptyInputTestRuleCacheTestCase: replacement_mock: str expected_first_codes: tuple[str, ...] expected_second_codes: tuple[str, ...] + + +@dataclass(frozen=True) +class TypeProofRuleCompileTestCase: + description: str + project_toml: str + extra_args: tuple[str, ...] + expected_returncode: int + expected_rule_findings: int + expected_note_count: int diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_compile_type_proof_rules.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_compile_type_proof_rules.py new file mode 100644 index 0000000000..6cc5f4990d --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/test_compile_type_proof_rules.py @@ -0,0 +1,141 @@ +"""E2E tests for type-proof rules when SQL analysis is disabled.""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path +from typing import Any + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.compile._test_types import ( + TypeProofRuleCompileTestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import prepare_inline_project, run_sqb + +_SKIPPED_NOTE: str = ( + "note: skipped type-proof rules (SQBRCONTRACT105) because SQL analysis is disabled" +) +_PROJECT_TOML: str = ( + 'name = "orders"\nadapter = "duckdb"\n\n[rules]\nselect = ["SQBRCONTRACT105"]\n' +) +_ORDERS_MODEL: str = ( + "MODEL (\n" + ' description "Orders",\n' + " contract enforced,\n" + " columns (\n" + " order_id (type INTEGER, nullable false),\n" + " status (type VARCHAR),\n" + " ),\n" + ");\n\n" + "WITH final AS (\n" + " SELECT 1 AS order_id, 'placed' AS status\n" + " UNION ALL\n" + " SELECT 2 AS order_id, 'shipped' AS status\n" + ")\n" + "SELECT order_id, status FROM final\n" +) + + +@pytest.mark.parametrize( + "test_case", + ( + TypeProofRuleCompileTestCase( + description="analysis enabled evaluates type-proof rules", + project_toml=_PROJECT_TOML, + extra_args=(), + expected_returncode=1, + expected_rule_findings=4, + expected_note_count=0, + ), + TypeProofRuleCompileTestCase( + description="no-sql-analysis flag skips type-proof rules", + project_toml=_PROJECT_TOML, + extra_args=("--no-sql-analysis",), + expected_returncode=0, + expected_rule_findings=0, + expected_note_count=1, + ), + TypeProofRuleCompileTestCase( + description="project setting skips type-proof rules", + project_toml=_PROJECT_TOML + "\n[settings]\nsql_analysis = false\n", + extra_args=(), + expected_returncode=0, + expected_rule_findings=0, + expected_note_count=1, + ), + ), + ids=lambda case: case.description, +) +def test_given_typed_contract_when_compiling_then_type_proof_rules_follow_sql_analysis( + test_case: TypeProofRuleCompileTestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="orders", + repo_files={ + "sqlbuild_project.toml": test_case.project_toml, + "models/orders.sql": _ORDERS_MODEL, + }, + ) + + text_result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "compile", "--no-cache", *test_case.extra_args), + project_dir=project_dir, + ) + json_result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "compile", "--json", "--no-cache", *test_case.extra_args), + project_dir=project_dir, + ) + + text_output: str = text_result.stdout + text_result.stderr + assert text_result.returncode == test_case.expected_returncode, text_output + assert text_result.stderr.count(_SKIPPED_NOTE) == test_case.expected_note_count + assert _SKIPPED_NOTE not in text_result.stdout + assert text_result.stdout.count("error[SQBRCONTRACT105]") == test_case.expected_rule_findings + assert text_result.stdout.rstrip().splitlines()[-1].startswith(" Wrote:"), text_output + payload: dict[str, Any] = json.loads(json_result.stdout) + rule_codes: list[object] = [diagnostic["code"] for diagnostic in payload["diagnostics"]] + assert json_result.returncode == test_case.expected_returncode + assert rule_codes.count("SQBRCONTRACT105") == test_case.expected_rule_findings + assert json_result.stderr.count(_SKIPPED_NOTE) == test_case.expected_note_count + + +@pytest.mark.parametrize( + "test_case", + ( + TypeProofRuleCompileTestCase( + description="build without SQL analysis skips type-proof rules", + project_toml=_PROJECT_TOML, + extra_args=("--no-sql-analysis",), + expected_returncode=0, + expected_rule_findings=0, + expected_note_count=1, + ), + ), + ids=lambda case: case.description, +) +def test_given_typed_contract_when_building_without_sql_analysis_then_type_proof_rules_skip( + test_case: TypeProofRuleCompileTestCase, + tmp_path: Path, +) -> None: + project_dir: Path = prepare_inline_project( + tmp_path=tmp_path, + project_name="orders", + repo_files={ + "sqlbuild_project.toml": test_case.project_toml, + "models/orders.sql": _ORDERS_MODEL, + }, + ) + + result: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", "build", *test_case.extra_args), + project_dir=project_dir, + ) + + output: str = result.stdout + result.stderr + assert result.returncode == test_case.expected_returncode, output + assert output.count("SQBRCONTRACT105") == test_case.expected_note_count + assert output.count(_SKIPPED_NOTE.removeprefix("note: ")) == test_case.expected_note_count diff --git a/tests/unit/src/sqlbuild/rule_engine/main/render_skipped_rules/_test_types.py b/tests/unit/src/sqlbuild/rule_engine/main/render_skipped_rules/_test_types.py new file mode 100644 index 0000000000..cb6b90fdfe --- /dev/null +++ b/tests/unit/src/sqlbuild/rule_engine/main/render_skipped_rules/_test_types.py @@ -0,0 +1,10 @@ +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass(frozen=True) +class SkippedTypeProofRulesNoteTestCase: + description: str + codes: tuple[str, ...] + expected_note: str | None diff --git a/tests/unit/src/sqlbuild/rule_engine/main/render_skipped_rules/test_render_skipped_rules.py b/tests/unit/src/sqlbuild/rule_engine/main/render_skipped_rules/test_render_skipped_rules.py new file mode 100644 index 0000000000..812a893850 --- /dev/null +++ b/tests/unit/src/sqlbuild/rule_engine/main/render_skipped_rules/test_render_skipped_rules.py @@ -0,0 +1,32 @@ +from __future__ import annotations + +import pytest + +from sqlbuild.rule_engine.main.render_skipped_rules import format_skipped_type_proof_rules +from tests.unit.src.sqlbuild.rule_engine.main.render_skipped_rules._test_types import ( + SkippedTypeProofRulesNoteTestCase, +) + + +@pytest.mark.parametrize( + "test_case", + ( + SkippedTypeProofRulesNoteTestCase( + description="skipped rules produce one note", + codes=("SQBRCONTRACT105",), + expected_note=( + "skipped type-proof rules (SQBRCONTRACT105) because SQL analysis is disabled" + ), + ), + SkippedTypeProofRulesNoteTestCase( + description="no skipped rules produce no note", + codes=(), + expected_note=None, + ), + ), + ids=lambda case: case.description, +) +def test_given_skipped_type_proof_rules_when_formatting_note_then_names_skipped_codes( + test_case: SkippedTypeProofRulesNoteTestCase, +) -> None: + assert format_skipped_type_proof_rules(codes=test_case.codes) == test_case.expected_note diff --git a/tests/unit/src/sqlbuild/rule_engine/main/run_rules/_test_types.py b/tests/unit/src/sqlbuild/rule_engine/main/run_rules/_test_types.py new file mode 100644 index 0000000000..b9d3397c7f --- /dev/null +++ b/tests/unit/src/sqlbuild/rule_engine/main/run_rules/_test_types.py @@ -0,0 +1,12 @@ +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass(frozen=True) +class TypeProofRuleSkipTestCase: + description: str + no_sql_analysis: bool + settings_toml: str + expected_skipped_rules: tuple[str, ...] + expected_finding_codes: tuple[str, ...] diff --git a/tests/unit/src/sqlbuild/rule_engine/main/run_rules/test_run_rules.py b/tests/unit/src/sqlbuild/rule_engine/main/run_rules/test_run_rules.py new file mode 100644 index 0000000000..8f1ac856b1 --- /dev/null +++ b/tests/unit/src/sqlbuild/rule_engine/main/run_rules/test_run_rules.py @@ -0,0 +1,87 @@ +from __future__ import annotations + +from pathlib import Path + +import pytest + +from sqlbuild.adapter.contract.classes.base_adapter import BaseAdapter +from sqlbuild.cli.commands._helpers.runtime.adapters import resolve_adapter +from sqlbuild.compiler.discovery.main.discover import discover_project_inputs +from sqlbuild.compiler.discovery.models import DiscoveredProjectInputs +from sqlbuild.compiler.pipeline.main.graph import build_project_graph +from sqlbuild.compiler.pipeline.models import ProjectGraph +from sqlbuild.rule_engine.main.load_config import load_rules_config +from sqlbuild.rule_engine.main.run_rules import run_rules +from sqlbuild.rule_engine.models import RulesRunResult +from tests.unit.src.sqlbuild.rule_engine.main.run_rules._test_types import ( + TypeProofRuleSkipTestCase, +) + + +@pytest.mark.parametrize( + "test_case", + ( + TypeProofRuleSkipTestCase( + description="analysis enabled evaluates type-proof rules", + no_sql_analysis=False, + settings_toml="", + expected_skipped_rules=(), + expected_finding_codes=("SQBRCONTRACT105",) * 4, + ), + TypeProofRuleSkipTestCase( + description="run without SQL analysis skips type-proof rules", + no_sql_analysis=True, + settings_toml="", + expected_skipped_rules=("SQBRCONTRACT105",), + expected_finding_codes=(), + ), + TypeProofRuleSkipTestCase( + description="project without SQL analysis skips type-proof rules", + no_sql_analysis=False, + settings_toml="\n[settings]\nsql_analysis = false\n", + expected_skipped_rules=("SQBRCONTRACT105",), + expected_finding_codes=(), + ), + ), + ids=lambda case: case.description, +) +def test_given_sql_analysis_state_when_running_rules_then_type_proof_rules_follow_it( + test_case: TypeProofRuleSkipTestCase, + tmp_path: Path, +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n\n[rules]\nselect = ["SQBRCONTRACT105"]\n' + + test_case.settings_toml, + encoding="utf-8", + ) + models: Path = tmp_path / "models" + models.mkdir() + (models / "orders.sql").write_text( + "MODEL (\n" + " contract enforced,\n" + " columns (order_id (type INTEGER), status (type VARCHAR)),\n" + ");\n" + "SELECT order_id, status FROM (SELECT 1 AS order_id, 'placed' AS status) AS raw_orders\n" + "UNION ALL\n" + "SELECT 2 AS order_id, 'shipped' AS status\n", + encoding="utf-8", + ) + discovered: DiscoveredProjectInputs = discover_project_inputs(project_dir=tmp_path) + adapter: BaseAdapter = resolve_adapter(adapter_name="duckdb", project_dir=tmp_path) + graph: ProjectGraph = build_project_graph( + discovered_inputs=discovered, + adapter=adapter, + no_sql_validation=test_case.no_sql_analysis, + ) + + result: RulesRunResult = run_rules( + graph=graph, + discovered_inputs=discovered, + config=load_rules_config(project_dir=tmp_path), + project_dir=tmp_path, + dialect="duckdb", + no_sql_analysis=test_case.no_sql_analysis, + ) + + assert result.skipped_type_proof_rules == test_case.expected_skipped_rules + assert tuple(finding.code for finding in result.findings) == test_case.expected_finding_codes diff --git a/website/src/content/docs/docs/cli/compile.mdx b/website/src/content/docs/docs/cli/compile.mdx index e6a26bc945..d5d553bbfc 100644 --- a/website/src/content/docs/docs/cli/compile.mdx +++ b/website/src/content/docs/docs/cli/compile.mdx @@ -36,7 +36,9 @@ sqb --project-dir compile [flags] 3. **SQL validation** - validates SQL syntax (when SQL analysis is enabled) 4. **Column lineage** - analyzes column-level dependencies across models (fast mode by default) 5. **Contract validation** - checks declared column contracts against inferred query output -6. **Rules** - evaluates selected native built-ins, then selected custom Python Rules +6. **Rules** - evaluates selected native built-ins, then selected custom Python Rules. Rules that + need inferred type proofs, such as `SQBRCONTRACT105`, are skipped when SQL analysis is disabled, + and compile prints one note naming them 7. **Artifact write** - writes compiled SQL to `target/compiled/` when Rules pass ## Focused compilation From 7dd38082a718369946e6628af8f9310fffa97393 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 27 Sep 2026 03:38:29 +0100 Subject: [PATCH 6/8] fix: keep join modifiers and whole-row alias uses out of unused-alias findings SQBRSQL023 treated an unaliased relation's join or relation modifier, such as POSITIONAL in `FROM orders POSITIONAL JOIN customers`, as an implicit alias and offered to delete it. It also counted an alias as used only when followed by a dot, so whole-row uses such as `to_json(o)`, `SELECT o` or `WHERE o IS NOT NULL` were reported and their deletion broke binding. Modifier and clause keywords are no longer implicit-alias candidates, and any bare identifier reference to the alias counts as a use for both implicit and AS aliases. Refs CHI-376 --- .../src/sql_lint/_helpers/additional.rs | 19 ++- .../src/sql_lint/constants.rs | 44 ++++++ .../sql_lint/tests/test_unused_table_alias.rs | 148 ++++++++++++++++++ .../sqlbuild/cli/commands/main/test_rules.py | 14 ++ 4 files changed, 222 insertions(+), 3 deletions(-) diff --git a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs index 360f5fcfcc..140407773f 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs @@ -10,6 +10,7 @@ use crate::sql_lint::_helpers::engine::{ direct_indices, is_comment, is_layout, is_query_from, query_end, significant_after, significant_before, token_depths, }; +use crate::sql_lint::constants::RELATION_MODIFIER_KEYWORDS; use crate::sql_lint::models::{AdditionalFactOptions, AdditionalQueryFacts}; const COUNT_ONE_LITERAL: &str = "1"; @@ -1250,10 +1251,15 @@ fn unused_alias_spans(query: &QuerySlice<'_>) -> Vec { } let alias = &tokens[alias_index].text; let used = (query_start..query_end).any(|index| { - depths[index] >= depth + index != alias_index + && depths[index] >= depth + && is_identifier(&tokens[index]) && tokens[index].text.eq_ignore_ascii_case(alias) + && significant_before(tokens, index).is_none_or(|previous| { + !matches!(tokens[previous].token_type, TokenType::Dot | TokenType::As) + }) && significant_after(tokens, index) - .is_some_and(|next| tokens[next].token_type == TokenType::Dot) + .is_none_or(|next| tokens[next].token_type != TokenType::LParen) }); if !used { spans.push(Span { @@ -1301,7 +1307,14 @@ fn relation_alias_positions(tokens: &[Token], relation_clause: &[usize]) -> Vec< } fn is_implicit_relation_alias(tokens: &[Token], segment: &[usize], position: usize) -> bool { - if position == 0 || !is_identifier(&tokens[segment[position]]) { + let candidate = &tokens[segment[position]]; + if position == 0 + || !is_identifier(candidate) + || candidate.token_type != TokenType::QuotedIdentifier + && RELATION_MODIFIER_KEYWORDS + .iter() + .any(|keyword| candidate.text.eq_ignore_ascii_case(keyword)) + { return false; } let previous = &tokens[segment[position - 1]]; diff --git a/crates/sqlbuild-rules-native/src/sql_lint/constants.rs b/crates/sqlbuild-rules-native/src/sql_lint/constants.rs index 873ef0ab42..cb094978d6 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/constants.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/constants.rs @@ -6,3 +6,47 @@ pub(crate) const VALUES_RELATION_PREFIX_TOKEN_COUNT: usize = 2; pub(crate) const POSITION_ARGUMENT_SEPARATOR: &str = ","; pub(crate) const CTE_MACRO_PLACEHOLDER_LITERAL: &str = "1"; pub(crate) const POSTFIX_CAST_PREFIX_TOKEN_COUNT: usize = 2; +pub(crate) const RELATION_MODIFIER_KEYWORDS: [&str; 42] = [ + "ANTI", + "APPLY", + "ASOF", + "AT", + "BEFORE", + "CHANGES", + "CONNECT", + "CROSS", + "EXCEPT", + "FETCH", + "FINAL", + "FOR", + "FULL", + "GROUP", + "HAVING", + "INNER", + "INTERSECT", + "JOIN", + "LATERAL", + "LEFT", + "LIMIT", + "MATCH_RECOGNIZE", + "MINUS", + "NATURAL", + "OFFSET", + "ON", + "ORDER", + "OUTER", + "PASTE", + "PIVOT", + "POSITIONAL", + "QUALIFY", + "RETURNING", + "RIGHT", + "SAMPLE", + "SEMI", + "START", + "TABLESAMPLE", + "UNION", + "UNPIVOT", + "USING", + "WHERE", +]; diff --git a/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs b/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs index 573119f283..6ceccfc68f 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs @@ -139,3 +139,151 @@ fn given_implicit_and_explicit_table_aliases_when_linting_then_unused_aliases_ma } Ok(()) } + +#[test] +fn given_join_modifiers_and_whole_row_uses_when_linting_duckdb_then_aliases_are_kept() +-> Result<(), String> { + let test_cases = [ + test_types::AdditionalLintRuleTestCase { + description: "unaliased relation before positional join has no alias", + sql: "SELECT * FROM orders POSITIONAL JOIN customers", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "unaliased relation before asof join has no alias", + sql: "SELECT * FROM orders ASOF JOIN prices ON orders.item_id = prices.item_id AND orders.ordered_at >= prices.valid_from", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "unaliased relation before natural join has no alias", + sql: "SELECT * FROM orders NATURAL JOIN customers", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "unaliased relation before anti join has no alias", + sql: "SELECT * FROM orders ANTI JOIN refunds USING (order_id)", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "unaliased relation before semi join has no alias", + sql: "SELECT * FROM orders SEMI JOIN refunds USING (order_id)", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "unaliased relation before cross join has no alias", + sql: "SELECT * FROM orders CROSS JOIN customers", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "unaliased relation before tablesample has no alias", + sql: "SELECT * FROM orders TABLESAMPLE BERNOULLI (10)", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "unaliased relation before unpivot has no alias", + sql: "SELECT * FROM orders UNPIVOT (amount FOR month IN (jan, feb))", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "implicit alias used as whole-row function argument", + sql: "SELECT row_to_json(o) AS payload FROM orders o", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "explicit alias used as whole-row function argument", + sql: "SELECT row_to_json(o) AS payload FROM orders AS o", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "implicit alias used as bare projection", + sql: "SELECT o FROM orders o", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "explicit alias used as bare projection", + sql: "SELECT o FROM orders AS o", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "implicit alias used in whole-row comparison", + sql: "SELECT 1 AS present FROM orders o WHERE o IS NOT NULL", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "explicit alias used in whole-row comparison", + sql: "SELECT 1 AS present FROM orders AS o WHERE o IS NOT NULL", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, + test_types::AdditionalLintRuleTestCase { + description: "unused implicit alias beside an unrelated qualified column is reported", + sql: "SELECT c.o FROM customers c CROSS JOIN orders o", + rule: "SQBRSQL023", + expected_anchor: Some("o"), + expected_replacement: Some(""), + }, + test_types::AdditionalLintRuleTestCase { + description: "unused explicit alias shadowed by an output alias is reported", + sql: "SELECT order_id AS o FROM orders AS o", + rule: "SQBRSQL023", + expected_anchor: Some("AS o"), + expected_replacement: Some(""), + }, + ]; + + for test_case in test_cases { + let diagnostics = + helpers::diagnostics_for_dialect(test_case.sql, "duckdb", &[test_case.rule])?; + assert_eq!( + diagnostics.len(), + usize::from(test_case.expected_anchor.is_some()), + "{}", + test_case.description + ); + let _ = test_case.expected_anchor.map(|expected_anchor| { + let diagnostic = &diagnostics[0]; + let start = diagnostic["start"].as_u64().unwrap_or_default() as usize; + let end = diagnostic["end"].as_u64().unwrap_or_default() as usize; + assert_eq!( + &test_case.sql[start..end], + expected_anchor, + "{}", + test_case.description + ); + assert_eq!( + diagnostic["fix"]["replacement"].as_str(), + test_case.expected_replacement, + "{}", + test_case.description + ); + }); + } + Ok(()) +} diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py index a8aed10d13..98e07c6822 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py @@ -1057,6 +1057,20 @@ def test_given_table_function_argument_when_running_alias_rule_then_left_alias_i query_sql='SELECT order_id\nFROM __ref("orders") AS o, __ref("customers") c\n', expected_locations=((3, 22), (3, 47)), ), + ImplicitAliasRuleIntegrationTestCase( + description="whole-row alias uses are clean", + query_sql=( + "SELECT to_json(o) AS payload, c\n" + 'FROM __ref("orders") o\n' + 'INNER JOIN __ref("customers") AS c ON o.customer_id = c.customer_id\n' + ), + expected_locations=(), + ), + ImplicitAliasRuleIntegrationTestCase( + description="join modifiers after unaliased relations are clean", + query_sql='SELECT *\nFROM __ref("orders") POSITIONAL JOIN __ref("customers")\n', + expected_locations=(), + ), ImplicitAliasRuleIntegrationTestCase( description="used implicit aliases are clean", query_sql=( From facc2e35c1702dcfe6ad7053acd2ee5e6d3c595d Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 27 Sep 2026 03:39:25 +0100 Subject: [PATCH 7/8] fix: keep WINDOW clauses out of unused-alias findings --- crates/sqlbuild-rules-native/src/sql_lint/constants.rs | 3 ++- .../src/sql_lint/tests/test_unused_table_alias.rs | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/crates/sqlbuild-rules-native/src/sql_lint/constants.rs b/crates/sqlbuild-rules-native/src/sql_lint/constants.rs index cb094978d6..7d1409979e 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/constants.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/constants.rs @@ -6,7 +6,7 @@ pub(crate) const VALUES_RELATION_PREFIX_TOKEN_COUNT: usize = 2; pub(crate) const POSITION_ARGUMENT_SEPARATOR: &str = ","; pub(crate) const CTE_MACRO_PLACEHOLDER_LITERAL: &str = "1"; pub(crate) const POSTFIX_CAST_PREFIX_TOKEN_COUNT: usize = 2; -pub(crate) const RELATION_MODIFIER_KEYWORDS: [&str; 42] = [ +pub(crate) const RELATION_MODIFIER_KEYWORDS: [&str; 43] = [ "ANTI", "APPLY", "ASOF", @@ -49,4 +49,5 @@ pub(crate) const RELATION_MODIFIER_KEYWORDS: [&str; 42] = [ "UNPIVOT", "USING", "WHERE", + "WINDOW", ]; diff --git a/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs b/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs index 6ceccfc68f..d94c22f3c9 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs @@ -186,6 +186,13 @@ fn given_join_modifiers_and_whole_row_uses_when_linting_duckdb_then_aliases_are_ expected_anchor: None, expected_replacement: None, }, + test_types::AdditionalLintRuleTestCase { + description: "unaliased relation before window clause has no alias", + sql: "SELECT SUM(amount) OVER w FROM orders WINDOW w AS (ORDER BY id)", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, test_types::AdditionalLintRuleTestCase { description: "unaliased relation before tablesample has no alias", sql: "SELECT * FROM orders TABLESAMPLE BERNOULLI (10)", From 23fba65b647c42621b3fe3189f4da4794bc0a1b4 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 27 Sep 2026 03:41:49 +0100 Subject: [PATCH 8/8] fix: report aliases that repeat their relation name when unused --- .../src/sql_lint/_helpers/additional.rs | 2 ++ .../sql_lint/tests/test_unused_table_alias.rs | 21 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs index 140407773f..3d6b769bcb 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/additional.rs @@ -1250,8 +1250,10 @@ fn unused_alias_spans(query: &QuerySlice<'_>) -> Vec { continue; } let alias = &tokens[alias_index].text; + let relation_name = significant_before(tokens, span_start); let used = (query_start..query_end).any(|index| { index != alias_index + && Some(index) != relation_name && depths[index] >= depth && is_identifier(&tokens[index]) && tokens[index].text.eq_ignore_ascii_case(alias) diff --git a/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs b/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs index d94c22f3c9..940b79c4f1 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/tests/test_unused_table_alias.rs @@ -186,6 +186,27 @@ fn given_join_modifiers_and_whole_row_uses_when_linting_duckdb_then_aliases_are_ expected_anchor: None, expected_replacement: None, }, + test_types::AdditionalLintRuleTestCase { + description: "implicit alias repeating the relation name is unused", + sql: "SELECT id FROM orders orders", + rule: "SQBRSQL023", + expected_anchor: Some("orders"), + expected_replacement: Some(""), + }, + test_types::AdditionalLintRuleTestCase { + description: "explicit alias repeating the relation name is unused", + sql: "SELECT id FROM orders AS orders", + rule: "SQBRSQL023", + expected_anchor: Some("AS orders"), + expected_replacement: Some(""), + }, + test_types::AdditionalLintRuleTestCase { + description: "alias repeating the relation name is used when referenced", + sql: "SELECT orders.id FROM orders orders", + rule: "SQBRSQL023", + expected_anchor: None, + expected_replacement: None, + }, test_types::AdditionalLintRuleTestCase { description: "unaliased relation before window clause has no alias", sql: "SELECT SUM(amount) OVER w FROM orders WINDOW w AS (ORDER BY id)",