From 144dc45af48a6dffbfa7ae5c7b3e85b13b7144fd Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 6 Nov 2026 13:21:19 +0000 Subject: [PATCH] fix: refuse COUNT over a qualified or modified star COUNT(t.*), COUNT(* EXCLUDE ...), COUNT(* ILIKE ...), COUNT(* RENAME ...), COUNT(DISTINCT t.*) and their windowed forms converted to count(*), which counts every row. Snowflake counts other rows for them, so they refuse until recordings show what to convert them to. --- .../sqlquack-rules/src/catalogue/constants.rs | 2 +- .../conversion/_helpers/expressions/calls.rs | 17 +++++-- .../_helpers/expressions/spellings.rs | 6 ++- crates/sqlquack/tests/cli/test_convert.rs | 50 +++++++++++++++++++ 4 files changed, 70 insertions(+), 5 deletions(-) diff --git a/crates/sqlquack-rules/src/catalogue/constants.rs b/crates/sqlquack-rules/src/catalogue/constants.rs index 956582135..a36fe4791 100644 --- a/crates/sqlquack-rules/src/catalogue/constants.rs +++ b/crates/sqlquack-rules/src/catalogue/constants.rs @@ -1235,7 +1235,7 @@ pub const RULES: &[RuleSpec] = &[ RuleSpec { id: COUNT, constructs: &["fn:COUNT"], - conditions: "COUNT(*), COUNT(x) or COUNT(DISTINCT x); typed NUMBER(18,0). COUNT of a VARIANT skips JSON null as well as NULL; COUNT(DISTINCT) of a VARIANT is refused.", + conditions: "COUNT(*), COUNT(x) or COUNT(DISTINCT x); typed NUMBER(18,0). COUNT of a VARIANT skips JSON null as well as NULL; COUNT(DISTINCT) of a VARIANT is refused. COUNT over a qualified star (t.*) or a star with EXCLUDE, ILIKE, RENAME or REPLACE is refused, as it counts other rows than COUNT(*).", failures: &[], }, RuleSpec { diff --git a/crates/sqlquack-rules/src/conversion/_helpers/expressions/calls.rs b/crates/sqlquack-rules/src/conversion/_helpers/expressions/calls.rs index ea2be5126..1fb5fa2c5 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/expressions/calls.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/expressions/calls.rs @@ -63,7 +63,7 @@ use crate::conversion::_helpers::support::divergence::diverge; use crate::conversion::_helpers::support::scope::{Context, Emitted}; use crate::conversion::_helpers::support::site::{Placement, Site}; use crate::conversion::_helpers::support::tree::{ - Payload, field, flag, function_construct, list, node_construct, payload_object, + Payload, field, flag, function_construct, list, node_construct, node_kind, payload_object, require_understood, }; use crate::conversion::_helpers::support::typed_leaves::compared_by_text; @@ -71,8 +71,8 @@ use crate::conversion::constants::MAX_KIND; use crate::conversion::constants::{ AVERAGE, AVG_NAME, COUNT, GENERATOR_RULE, GENERATOR_SEQUENCE_COLUMN, LEAST_GREATEST, LOCAL_ZONE_SUFFIX, MIN_MAX, MIN_MAX_FLOAT, NOT_SNOWFLAKE_FUNCTIONS, NULLIF, - PART_FUNCTIONS_PAIR, REGEXP_LIKE_NAME, RLIKE_PAIR, SORTED_ARRAY_FUNCTION, SUM, SUM_FLOAT, - TIMESTAMPDIFF_PAIR, TO_DATE_NAME, TRIM, TRY_PREFIX, VARIANT_MAX, + PART_FUNCTIONS_PAIR, REGEXP_LIKE_NAME, RLIKE_PAIR, SORTED_ARRAY_FUNCTION, STAR_KIND, SUM, + SUM_FLOAT, TIMESTAMPDIFF_PAIR, TO_DATE_NAME, TRIM, TRY_PREFIX, VARIANT_MAX, }; use sqlquack_policy::policy::constants::NUMERIC_RANGE; @@ -432,6 +432,17 @@ impl AggregateCall<'_> { fn count(&self) -> Result { let log = self.site.context.log; let _rule = log.rule(COUNT); + let starred = self + .payload + .get("this") + .and_then(node_kind) + .is_some_and(|(kind, _)| kind == STAR_KIND); + if starred || (flag(self.payload, "star") && flag(self.payload, "distinct")) { + return Err(outside_conditions( + node_construct("count"), + "COUNT over a qualified star or a star with EXCLUDE, ILIKE, RENAME, REPLACE or DISTINCT, which counts other rows than COUNT(*)".to_owned(), + )); + } let inner = match flag(self.payload, "star") { true => "*".to_owned(), false => { diff --git a/crates/sqlquack-rules/src/conversion/_helpers/expressions/spellings.rs b/crates/sqlquack-rules/src/conversion/_helpers/expressions/spellings.rs index 03af935c5..78e1399f3 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/expressions/spellings.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/expressions/spellings.rs @@ -100,7 +100,11 @@ fn shaped(spelling: &SpellingRule, arguments: &[Value], payload: &Payload) -> Op json!({ "this": end, "expression": start, "unit": unit_var(unit)? }) } (CallShape::Count, [argument]) => { - let star = argument.get("star").is_some(); + let star = argument.get("star").is_some_and(|star| { + ["table", "ilike", "except", "replace", "rename"] + .iter() + .all(|field| star.get(*field).is_none_or(Value::is_null)) + }); json!({ "this": if star { Value::Null } else { argument.clone() }, "star": star, diff --git a/crates/sqlquack/tests/cli/test_convert.rs b/crates/sqlquack/tests/cli/test_convert.rs index 6f54b5aa9..405461f15 100644 --- a/crates/sqlquack/tests/cli/test_convert.rs +++ b/crates/sqlquack/tests/cli/test_convert.rs @@ -14,6 +14,7 @@ use crate::test_types::{ use sha2::{Digest, Sha256}; const OTHER_BODY_SHA256: &str = "0000000000000000000000000000000000000000000000000000000000000000"; +const COUNT_STAR_REFUSAL: &str = "COUNT over a qualified star or a star with"; const COUNTING_HANDLER: &str = "import itertools\nTICKETS = itertools.count(1)\ndef handle(*arguments):\n return next(TICKETS)\n"; const QUOTED_ALIAS: &str = "WITH totals AS (SELECT 1 AS \"orderTotal\") SELECT ordertotal FROM totals"; @@ -1151,6 +1152,55 @@ fn given_recorded_shapes_when_converting_then_they_convert_or_refuse_as_recorded } } +#[test] +fn given_count_over_a_qualified_or_modified_star_when_converting_then_it_refuses() { + let test_cases = [ + PythonRefusalTestCase { + description: "a qualified star counts rows where some column is not NULL, not every row", + sql: "SELECT COUNT(t.*) AS n FROM (SELECT 1 AS a, 'x' AS b UNION ALL SELECT NULL, NULL) AS t", + expected_mention: COUNT_STAR_REFUSAL, + }, + PythonRefusalTestCase { + description: "EXCLUDE narrows the columns a star counts over", + sql: "SELECT COUNT(* EXCLUDE b) AS n FROM (SELECT 1 AS a, 'x' AS b UNION ALL SELECT NULL, NULL) AS t", + expected_mention: COUNT_STAR_REFUSAL, + }, + PythonRefusalTestCase { + description: "ILIKE narrows the columns a star counts over", + sql: "SELECT COUNT(* ILIKE 'a%') AS n FROM (SELECT 1 AS a, 'x' AS b UNION ALL SELECT NULL, NULL) AS t", + expected_mention: COUNT_STAR_REFUSAL, + }, + PythonRefusalTestCase { + description: "RENAME on a counted star", + sql: "SELECT COUNT(* RENAME a AS z) AS n FROM (SELECT 1 AS a, 'x' AS b UNION ALL SELECT NULL, NULL) AS t", + expected_mention: COUNT_STAR_REFUSAL, + }, + PythonRefusalTestCase { + description: "a qualified star with EXCLUDE", + sql: "SELECT COUNT(t.* EXCLUDE b) AS n FROM (SELECT 1 AS a, 'x' AS b UNION ALL SELECT NULL, NULL) AS t", + expected_mention: COUNT_STAR_REFUSAL, + }, + PythonRefusalTestCase { + description: "DISTINCT over a qualified star", + sql: "SELECT COUNT(DISTINCT t.*) AS n FROM (SELECT 1 AS a, 'x' AS b UNION ALL SELECT NULL, NULL) AS t", + expected_mention: COUNT_STAR_REFUSAL, + }, + PythonRefusalTestCase { + description: "a qualified star over a window", + sql: "SELECT COUNT(t.*) OVER () AS n FROM (SELECT 1 AS a, 'x' AS b UNION ALL SELECT NULL, NULL) AS t", + expected_mention: COUNT_STAR_REFUSAL, + }, + ]; + for test_case in test_cases { + let refusals = python_report(test_case.sql, &[])["refusals"].to_string(); + assert!( + refusals.contains(test_case.expected_mention), + "{}: {refusals}", + test_case.description + ); + } +} + #[test] fn given_a_subquery_operand_when_converting_and_running_then_it_is_bound_outside_lambdas() { let test_cases = [