From d261efa37c8a4e6ccb913d220ed3c55b5651b797 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 18 Sep 2026 10:16:35 +0100 Subject: [PATCH] fix: evaluate authored numeric decisions structurally --- .../src/engine/tests/test_evaluation.rs | 79 +++++++++++++++++ .../src/engine/tests/test_types.rs | 7 ++ .../src/rules/_helpers/authored_literals.rs | 88 ------------------- .../src/rules/_helpers/evaluation.rs | 79 ++++++++++++----- .../src/rules/_helpers/mod.rs | 2 +- .../src/rules/_helpers/numeric_decisions.rs | 77 ++++++++++++++++ .../compile/_helpers/assembly/project.py | 1 + src/sqlbuild/compiler/compile/models.py | 1 + .../rule_engine/_helpers/engine/native.py | 2 +- 9 files changed, 223 insertions(+), 113 deletions(-) delete mode 100644 crates/sqlbuild-rules-native/src/rules/_helpers/authored_literals.rs create mode 100644 crates/sqlbuild-rules-native/src/rules/_helpers/numeric_decisions.rs 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 6a34a2b1bd..ed97813298 100644 --- a/crates/sqlbuild-rules-native/src/engine/tests/test_evaluation.rs +++ b/crates/sqlbuild-rules-native/src/engine/tests/test_evaluation.rs @@ -619,6 +619,85 @@ fn given_contract_name_type_options_when_evaluating_then_accepts_configured_inte Ok(()) } +#[test] +fn given_numeric_comparisons_when_evaluating_named_decisions_then_only_authored_decisions_fault() +-> Result<(), String> { + let project_dir = TempDir::new().map_err(|error| error.to_string())?; + let test_cases = [ + test_types::NumericDecisionTestCase { + description: "authored threshold is a named decision finding", + query_sql: "SELECT amount FROM orders WHERE amount > 5", + authored_sql: "SELECT amount FROM orders WHERE amount > 5", + expected_fault_count: 1, + }, + test_types::NumericDecisionTestCase { + description: "expanded named constant is not an authored literal finding", + query_sql: "SELECT amount FROM orders WHERE amount > 5", + authored_sql: "SELECT amount FROM orders WHERE amount > @const(\"_minimum_amount\")", + expected_fault_count: 0, + }, + test_types::NumericDecisionTestCase { + description: "numeric comparison shown only in a comment is not an authored finding", + query_sql: "SELECT amount FROM orders WHERE amount > 5", + authored_sql: concat!( + "-- The expanded predicate is amount > 5.\n", + "SELECT amount FROM orders WHERE amount > @const(\"_minimum_amount\")" + ), + expected_fault_count: 0, + }, + test_types::NumericDecisionTestCase { + description: "fixed output bucket suffix makes equality structural", + query_sql: concat!( + "SELECT MAX(CASE WHEN offset_seconds = 30 THEN amount END) AS amount_30 ", + "FROM orders" + ), + authored_sql: concat!( + "SELECT MAX(CASE WHEN offset_seconds = 30 THEN amount END) AS amount_30 ", + "FROM orders" + ), + expected_fault_count: 0, + }, + test_types::NumericDecisionTestCase { + description: "output suffix does not exempt a threshold comparison", + query_sql: ("SELECT CASE WHEN amount > 30 THEN 'large' END AS amount_30 FROM orders"), + authored_sql: ("SELECT CASE WHEN amount > 30 THEN 'large' END AS amount_30 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"), + authored_sql: ("SELECT CASE WHEN amount > 5 THEN status END = 'ready' AS selected FROM orders"), + expected_fault_count: 1, + }, + ]; + + for test_case in test_cases { + let mut request: Value = serde_json::from_str(&helpers::request( + &project_dir, + &json!({ + "select": ["SQBRDECLARATION102"], + "cache": {"enabled": false} + }), + )) + .map_err(|error| error.to_string())?; + request["models"][0]["query_sql"] = json!(test_case.query_sql); + request["models"][0]["authored_sql"] = json!(test_case.authored_sql); + + let result: Value = serde_json::from_str(&evaluate_json(&request.to_string())?) + .map_err(|error| error.to_string())?; + let faults = result["faults"] + .as_array() + .ok_or_else(|| "faults must be an array".to_string())?; + assert_eq!( + faults.len(), + test_case.expected_fault_count, + "{}", + test_case.description + ); + } + Ok(()) +} + #[test] fn given_snowflake_parser_extensions_when_normalizing_then_preserves_source_positions() { let test_cases = [test_types::NormalizationTestCase { diff --git a/crates/sqlbuild-rules-native/src/engine/tests/test_types.rs b/crates/sqlbuild-rules-native/src/engine/tests/test_types.rs index 481290dac8..547dd93b27 100644 --- a/crates/sqlbuild-rules-native/src/engine/tests/test_types.rs +++ b/crates/sqlbuild-rules-native/src/engine/tests/test_types.rs @@ -35,6 +35,13 @@ pub(crate) struct ContractNameTypeOptionsTestCase { pub(crate) expected_fault_codes: &'static [&'static str], } +pub(crate) struct NumericDecisionTestCase { + pub(crate) description: &'static str, + pub(crate) query_sql: &'static str, + pub(crate) authored_sql: &'static str, + pub(crate) expected_fault_count: usize, +} + pub(crate) struct TypedContractColumnTestCase { pub(crate) description: &'static str, pub(crate) contract: &'static str, diff --git a/crates/sqlbuild-rules-native/src/rules/_helpers/authored_literals.rs b/crates/sqlbuild-rules-native/src/rules/_helpers/authored_literals.rs deleted file mode 100644 index 59539d1b8e..0000000000 --- a/crates/sqlbuild-rules-native/src/rules/_helpers/authored_literals.rs +++ /dev/null @@ -1,88 +0,0 @@ -use sqlparser::dialect::Dialect; -use sqlparser::tokenizer::{Token, Tokenizer}; -use std::collections::BTreeSet; - -#[derive(Clone, Copy)] -struct CastScope { - depth: usize, - in_type: bool, -} - -pub(super) fn numeric_literal_tokens( - source: &str, - dialect: &dyn Dialect, -) -> Result, String> { - let tokens = Tokenizer::new(dialect, source) - .tokenize() - .map_err(|error| format!("could not tokenize authored numeric literals: {error}"))?; - let tokens = tokens - .iter() - .filter(|token| !matches!(token, Token::Whitespace(_))) - .collect::>(); - let has_model_header = matches!( - tokens.first(), - Some(Token::Word(word)) - if word.quote_style.is_none() && word.value.eq_ignore_ascii_case("model") - ) && matches!(tokens.get(1), Some(Token::LParen)); - let model_header_end = has_model_header - .then(|| { - tokens - .iter() - .position(|token| matches!(token, Token::SemiColon)) - }) - .flatten(); - let query_start = if has_model_header { - model_header_end.map_or(0, |end| end + 1) - } else { - 0 - }; - let mut literals: BTreeSet = BTreeSet::new(); - let mut cast_scopes: Vec = Vec::new(); - let mut depth = 0_usize; - for (index, token) in tokens[query_start..].iter().enumerate() { - match token { - Token::LParen => { - depth += 1; - if index > 0 - && matches!( - tokens[query_start + index - 1], - Token::Word(word) - if word.quote_style.is_none() - && matches!( - word.value.to_ascii_uppercase().as_str(), - "CAST" | "TRY_CAST" - ) - ) - { - cast_scopes.push(CastScope { - depth, - in_type: false, - }); - } - } - Token::RParen => { - if cast_scopes.last().is_some_and(|scope| scope.depth == depth) { - cast_scopes.pop(); - } - depth = depth.saturating_sub(1); - } - Token::Word(word) - if word.quote_style.is_none() - && word.value.eq_ignore_ascii_case("as") - && cast_scopes.last().is_some_and(|scope| scope.depth == depth) => - { - if let Some(scope) = cast_scopes.last_mut() { - scope.in_type = true; - } - } - Token::Number(number, _) if !cast_scopes.last().is_some_and(|scope| scope.in_type) => { - literals.insert(number.to_string()); - if index > 0 && matches!(tokens[query_start + index - 1], Token::Minus) { - literals.insert(format!("-{number}")); - } - } - _ => {} - } - } - Ok(literals) -} diff --git a/crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs b/crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs index 7fa0866da2..fbeb184d18 100644 --- a/crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs +++ b/crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.rs @@ -3,9 +3,10 @@ use crate::constants::{ SOURCE_REFERENCE_KIND, VIEW_MATERIALIZATION, }; use crate::models::{Declaration, EvaluateRequest, Fault, Model, RuleMetadata, RulesConfig}; -use crate::rules::_helpers::authored_literals::numeric_literal_tokens; use crate::rules::_helpers::domain_layout::folder_layer_details; -use crate::rules::_helpers::{contract_name_types, explicit_output_types, typed_contract_columns}; +use crate::rules::_helpers::{ + contract_name_types, explicit_output_types, numeric_decisions, typed_contract_columns, +}; use crate::rules::models::{ FaultCollector, ModelEvaluationRequest, ProjectEvaluationRequest, ResolvedThresholdOverride, }; @@ -35,6 +36,9 @@ struct Position { #[derive(Default)] struct ComparisonFact { position: Option, + sql: String, + equality: bool, + output_name: Option, columns: BTreeSet, string_literals: Vec, numeric_literals: Vec, @@ -64,7 +68,7 @@ struct ParsedModel<'a> { query: Query, model: &'a Model, classification: ModelClassification, - authored_numeric_literals: BTreeSet, + authored_compact_sql: String, } struct ModelClassification { @@ -137,14 +141,11 @@ fn evaluate_model_inner(request: ModelEvaluationRequest<'_>) -> Result Vec { } for item in &select.projection { match item { - SelectItem::UnnamedExpr(expression) - | SelectItem::ExprWithAlias { - expr: expression, .. + SelectItem::UnnamedExpr(expression) => { + case_comparison_facts( + expression, + None, + &source_context, + &mut result.comparisons, + ); + } + SelectItem::ExprWithAlias { + expr: expression, + alias, } => { - case_comparison_facts(expression, &source_context, &mut result.comparisons); + case_comparison_facts( + expression, + Some(&alias.value), + &source_context, + &mut result.comparisons, + ); } SelectItem::QualifiedWildcard(_, _) | SelectItem::Wildcard(_) => {} } @@ -1738,7 +1763,7 @@ fn join_constraint(operator: &JoinOperator) -> Option<&JoinConstraint> { } fn numeric_value(expression: &Expr) -> Option { - match expression { + match unwrap_nested(expression) { Expr::Value(value) => match &value.value { Value::Number(value, _) => Some(value.to_string()), _ => None, @@ -1784,6 +1809,7 @@ fn comparison_facts(root: &Expr, source_context: &SourceContext, output: &mut Ve fn case_comparison_facts( root: &Expr, + output_name: Option<&str>, source_context: &SourceContext, output: &mut Vec, ) { @@ -1815,7 +1841,11 @@ fn case_comparison_facts( source_context, output, }; + let start = visitor.output.len(); let _ = root.visit(&mut visitor); + for fact in &mut visitor.output[start..] { + fact.output_name = output_name.map(str::to_owned); + } } fn comparison_fact(root: &Expr, source_context: &SourceContext) -> ComparisonFact { @@ -1835,9 +1865,6 @@ fn comparison_fact(root: &Expr, source_context: &SourceContext) -> ComparisonFac } } Expr::Value(value) => match &value.value { - Value::Number(number, _) => { - self.fact.numeric_literals.push(number.to_string()); - } Value::SingleQuotedString(_) | Value::DoubleQuotedString(_) | Value::TripleSingleQuotedString(_) @@ -1849,11 +1876,6 @@ fn comparison_fact(root: &Expr, source_context: &SourceContext) -> ComparisonFac } _ => {} }, - Expr::UnaryOp { op, expr } if op.to_string() == NEGATION_OPERATOR => { - if let Some(number) = numeric_value(expr) { - self.fact.numeric_literals.push(format!("-{number}")); - } - } _ => {} } ControlFlow::Continue(()) @@ -1862,6 +1884,14 @@ fn comparison_fact(root: &Expr, source_context: &SourceContext) -> ComparisonFac let mut visitor = Values { fact: ComparisonFact { position: Some(position(root)), + sql: root.to_string(), + equality: matches!( + root, + Expr::BinaryOp { + op: BinaryOperator::Eq, + .. + } + ), source_context: source_context.clone(), ..ComparisonFact::default() }, @@ -1869,6 +1899,9 @@ fn comparison_fact(root: &Expr, source_context: &SourceContext) -> ComparisonFac 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)); } diff --git a/crates/sqlbuild-rules-native/src/rules/_helpers/mod.rs b/crates/sqlbuild-rules-native/src/rules/_helpers/mod.rs index e67dc1a4bb..0c45193930 100644 --- a/crates/sqlbuild-rules-native/src/rules/_helpers/mod.rs +++ b/crates/sqlbuild-rules-native/src/rules/_helpers/mod.rs @@ -1,8 +1,8 @@ -pub(crate) mod authored_literals; pub(crate) mod catalogue; pub(crate) mod contract_name_types; pub(crate) mod domain_layout; pub(crate) mod evaluation; pub(crate) mod explicit_output_types; +pub(crate) mod numeric_decisions; pub(crate) mod sql_test_rules; pub(crate) mod typed_contract_columns; diff --git a/crates/sqlbuild-rules-native/src/rules/_helpers/numeric_decisions.rs b/crates/sqlbuild-rules-native/src/rules/_helpers/numeric_decisions.rs new file mode 100644 index 0000000000..fddcbcb86a --- /dev/null +++ b/crates/sqlbuild-rules-native/src/rules/_helpers/numeric_decisions.rs @@ -0,0 +1,77 @@ +const CANONICAL_NUMERIC_DECISIONS: [&str; 3] = ["-1", "0", "1"]; + +pub(crate) struct AuthoredDecision<'a> { + pub(crate) authored_compact_sql: &'a str, + pub(crate) comparison_sql: &'a str, + pub(crate) equality: bool, + pub(crate) output_name: Option<&'a str>, + pub(crate) literal: &'a str, +} + +pub(crate) fn compact_sql(sql: &str) -> String { + let bytes = sql.as_bytes(); + let mut compact = Vec::with_capacity(bytes.len()); + let mut index = 0; + let mut quote: Option = None; + let mut line_comment = false; + let mut block_comment = false; + while index < bytes.len() { + let byte = bytes[index]; + let next = bytes.get(index + 1).copied(); + if line_comment { + line_comment = byte != b'\n'; + index += 1; + continue; + } + if block_comment { + if byte == b'*' && next == Some(b'/') { + block_comment = false; + index += 2; + } else { + index += 1; + } + continue; + } + if let Some(active_quote) = quote { + compact.push(byte.to_ascii_lowercase()); + if byte == active_quote { + if next == Some(active_quote) { + compact.push(active_quote); + index += 2; + continue; + } + quote = None; + } + index += 1; + continue; + } + if byte == b'-' && next == Some(b'-') { + line_comment = true; + index += 2; + continue; + } + if byte == b'/' && next == Some(b'*') { + block_comment = true; + index += 2; + continue; + } + if matches!(byte, b'\'' | b'"') { + quote = Some(byte); + } + if !byte.is_ascii_whitespace() { + compact.push(byte.to_ascii_lowercase()); + } + index += 1; + } + String::from_utf8(compact).unwrap_or_default() +} + +pub(crate) fn is_authored_decision(decision: AuthoredDecision<'_>) -> bool { + !CANONICAL_NUMERIC_DECISIONS.contains(&decision.literal) + && decision + .authored_compact_sql + .contains(&compact_sql(decision.comparison_sql)) + && !decision.output_name.is_some_and(|name| { + decision.equality && name.ends_with(&format!("_{}", decision.literal)) + }) +} diff --git a/src/sqlbuild/compiler/compile/_helpers/assembly/project.py b/src/sqlbuild/compiler/compile/_helpers/assembly/project.py index 78663c6551..a94204b1e1 100644 --- a/src/sqlbuild/compiler/compile/_helpers/assembly/project.py +++ b/src/sqlbuild/compiler/compile/_helpers/assembly/project.py @@ -416,6 +416,7 @@ def _assemble_compiled_model( fast_lineage_columns=fast_lineage_columns, fast_lineage_has_star=fast_lineage_has_star, authored_sql=model_input.model_file.contents, + authored_query_sql=model_input.model_file.query_sql, output_column_locations=model_input.model_file.output_column_locations, extract_implicit_alias_columns=model_input.model_file.extract_implicit_alias_columns, macro_deps=model_input.macro_deps or find_macro_call_names(model_input.macro_source_sql), diff --git a/src/sqlbuild/compiler/compile/models.py b/src/sqlbuild/compiler/compile/models.py index eb63487385..f0fabd96cc 100644 --- a/src/sqlbuild/compiler/compile/models.py +++ b/src/sqlbuild/compiler/compile/models.py @@ -731,6 +731,7 @@ class CompiledModel: fast_lineage_columns: tuple[CompiledLineageColumnFact, ...] | None = None fast_lineage_has_star: bool = False authored_sql: str = "" + authored_query_sql: str = "" output_column_locations: dict[str, SourceLocation] = field(default_factory=dict) extract_implicit_alias_columns: bool = True macro_deps: tuple[str, ...] = field(default_factory=tuple) diff --git a/src/sqlbuild/rule_engine/_helpers/engine/native.py b/src/sqlbuild/rule_engine/_helpers/engine/native.py index 7777860265..bf8a3f8e18 100644 --- a/src/sqlbuild/rule_engine/_helpers/engine/native.py +++ b/src/sqlbuild/rule_engine/_helpers/engine/native.py @@ -386,7 +386,7 @@ def _model_payload( "name": model.name, "relative_path": model.relative_path.as_posix(), "query_sql": model.query_sql, - "authored_sql": model.authored_sql, + "authored_sql": model.authored_query_sql or model.authored_sql, "config": model.config.values, "references": [ {