fix: evaluate authored numeric decisions structurally

Author: kvlonge

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 {

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,

crates/sqlbuild-rules-native/src/rules/_helpers/authored_literals.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<Position>,
+    sql: String,
+    equality: bool,
+    output_name: Option<String>,
     columns: BTreeSet<String>,
     string_literals: Vec<String>,
     numeric_literals: Vec<String>,
@@ -64,7 +68,7 @@ struct ParsedModel<'a> {
     query: Query,
     model: &'a Model,
     classification: ModelClassification,
-    authored_numeric_literals: BTreeSet<String>,
+    authored_compact_sql: String,
 }
 
 struct ModelClassification {
@@ -137,14 +141,11 @@ fn evaluate_model_inner(request: ModelEvaluationRequest<'_>) -> Result<Vec<Fault
     };
     let query = *query;
     let classification = classify_model(&query, model, &request.dialect)?;
-    let authored_dialect = rules_dialect(&request.dialect);
-    let authored_numeric_literals =
-        numeric_literal_tokens(&model.authored_sql, authored_dialect.as_ref())?;
     let parsed = ParsedModel {
         query,
         model,
         classification,
-        authored_numeric_literals,
+        authored_compact_sql: numeric_decisions::compact_sql(&model.authored_sql),
     };
     if let Some(rule) = metadata("SQBRMODEL101") {
         import_ctes(&parsed, rule, &faults);
@@ -1361,11 +1362,22 @@ fn evaluate_literal_rules(
         }
         if let Some(rule) = selected.get("SQBRDECLARATION102") {
             let magic = comparison.numeric_literals.iter().any(|literal| {
-                !matches!(literal.as_str(), "-1" | "0" | "1")
-                    && parsed.authored_numeric_literals.contains(literal)
+                numeric_decisions::is_authored_decision(numeric_decisions::AuthoredDecision {
+                    authored_compact_sql: &parsed.authored_compact_sql,
+                    comparison_sql: &comparison.sql,
+                    equality: comparison.equality,
+                    output_name: comparison.output_name.as_deref(),
+                    literal,
+                })
             });
             if magic {
-                faults.push(fault(parsed.model, rule, comparison.position.as_ref()));
+                faults.push(custom_fault!(
+                    parsed.model,
+                    rule,
+                    comparison.position.as_ref(),
+                    format!("non-canonical numeric comparison: {}", comparison.sql),
+                    None,
+                ));
             }
         }
     }
@@ -1633,11 +1645,24 @@ fn select_facts(query: &Query) -> Vec<SelectFacts> {
             }
             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<String> {
-    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<ComparisonFact>,
 ) {
@@ -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,13 +1884,24 @@ 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()
         },
     };
     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));
             }

crates/sqlbuild-rules-native/src/rules/_helpers/evaluation.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;