From d2d6312b768afcf5ef8c864500559fba31d53883 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 6 Nov 2026 04:21:01 +0000 Subject: [PATCH 1/2] fix: count only rows whose columns are all non-NULL for COUNT(t.*) COUNT(input.*) and COUNT(* EXCLUDE ...) were read as COUNT(*), counting every row. Snowflake counts the rows whose selected columns are all non-NULL, as the recorded receipts show. DISTINCT, a VARIANT column, and a star that selects no column are refused. --- .../sqlquack-rules/src/catalogue/constants.rs | 2 +- .../conversion/_helpers/expressions/calls.rs | 42 +++++++++++++++++-- .../_helpers/expressions/spellings.rs | 14 ++++++- crates/sqlquack/tests/cli/test_convert.rs | 28 +++++++++++++ 4 files changed, 80 insertions(+), 6 deletions(-) diff --git a/crates/sqlquack-rules/src/catalogue/constants.rs b/crates/sqlquack-rules/src/catalogue/constants.rs index c6bab09bc..cd7dec18f 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(t.*) or COUNT(* EXCLUDE ...) counts the rows whose selected columns are all non-NULL, as recorded; with DISTINCT, over a VARIANT column, or selecting no column it is refused.", 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..cc6abedba 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/expressions/calls.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/expressions/calls.rs @@ -59,11 +59,12 @@ use crate::conversion::_helpers::expressions::text_functions::{ }; use crate::conversion::_helpers::expressions::text_search::charindex_call; use crate::conversion::_helpers::expressions::variants::variant_function; +use crate::conversion::_helpers::queries::query::star as query_star; 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; @@ -432,9 +433,17 @@ impl AggregateCall<'_> { fn count(&self) -> Result { let log = self.site.context.log; let _rule = log.rule(COUNT); - let inner = match flag(self.payload, "star") { - true => "*".to_owned(), - false => { + let star = self + .payload + .get("this") + .and_then(|node| match node_kind(node) { + Some(("star", star)) => Some(star), + _ => None, + }); + let inner = match (flag(self.payload, "star"), star) { + (true, _) => "*".to_owned(), + (false, Some(star)) => self.counted_star(star)?, + (false, None) => { let argument = self.argument("count")?; match argument.data_type { SnowflakeType::SemiStructured if flag(self.payload, "distinct") => { @@ -456,6 +465,31 @@ impl AggregateCall<'_> { Ok(Emitted::new(sql, data_type)) } + /// `t.*` or a filtered star: Snowflake counts the rows whose selected columns are all non-NULL. + fn counted_star(&self, star: &Value) -> Result { + let refuse = |reason: &str| outside_conditions(node_construct("count"), reason.to_owned()); + if flag(self.payload, "distinct") { + return Err(refuse("COUNT(DISTINCT) of a star")); + } + let columns = query_star(star, self.site)?; + if columns + .iter() + .any(|(_, column)| column.data_type == SnowflakeType::SemiStructured) + { + return Err(refuse( + "COUNT of a star over a VARIANT column, whose JSON null no recording shows", + )); + } + let present: Vec = columns + .iter() + .map(|(_, column)| format!("{} IS NOT NULL", column.sql)) + .collect(); + match present.is_empty() { + true => Err(refuse("COUNT of a star that selects no column")), + false => Ok(format!("CASE WHEN {} THEN 1 END", present.join(" AND "))), + } + } + fn sum(&self) -> Result { let log = self.site.context.log; let refuse = |reason: String| outside_conditions(node_construct("sum"), reason); diff --git a/crates/sqlquack-rules/src/conversion/_helpers/expressions/spellings.rs b/crates/sqlquack-rules/src/conversion/_helpers/expressions/spellings.rs index 03af935c5..b566b6381 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/expressions/spellings.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/expressions/spellings.rs @@ -100,7 +100,7 @@ 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(bare_star); json!({ "this": if star { Value::Null } else { argument.clone() }, "star": star, @@ -165,3 +165,15 @@ pub(crate) fn rlike_as_call(payload: &Value) -> Option { }}) }) } + +/// Whether a star's payload names no relation and filters no column: `*` as written alone. +fn bare_star(star: &Value) -> bool { + star.as_object().is_none_or(|fields| { + fields.values().all(|value| match value { + Value::Null | Value::Bool(false) => true, + Value::Array(items) => items.is_empty(), + Value::Object(entries) => entries.is_empty(), + _ => false, + }) + }) +} diff --git a/crates/sqlquack/tests/cli/test_convert.rs b/crates/sqlquack/tests/cli/test_convert.rs index 304766dd3..84a0f0e4a 100644 --- a/crates/sqlquack/tests/cli/test_convert.rs +++ b/crates/sqlquack/tests/cli/test_convert.rs @@ -519,6 +519,34 @@ fn given_a_bracket_expression_with_a_lazy_spelling_when_converting_then_the_clas } } +#[test] +fn given_count_of_a_qualified_or_filtered_star_when_running_then_rows_with_a_null_are_skipped() { + let test_cases = [ + ValueTestCase { + description: "input.* skips a row with a NULL in any column, unlike *", + sql: "SELECT COUNT(input.*)::VARCHAR || ',' || COUNT(*)::VARCHAR AS r FROM (SELECT * \ + FROM (VALUES (1::NUMBER(38,0), 'x'::VARCHAR), (1, 'x'), (1, NULL), (NULL, 'y'), \ + (2, 'x')) AS v(a, b)) AS input", + expected_value: "3,5", + }, + ValueTestCase { + description: "* EXCLUDE b checks only the remaining column", + sql: "SELECT COUNT(* EXCLUDE b)::VARCHAR AS r FROM (SELECT * FROM (VALUES \ + (1::NUMBER(38,0), 'x'::VARCHAR), (1, 'x'), (1, NULL), (NULL, 'y'), (2, 'x')) \ + AS v(a, b)) AS input", + expected_value: "4", + }, + ]; + for test_case in test_cases { + assert_eq!( + converted_first_value(test_case.sql), + test_case.expected_value, + "{}", + test_case.description + ); + } +} + #[test] fn given_a_time_with_nanoseconds_when_formatting_then_digits_beyond_the_format_are_truncated() { let test_cases = [ From 3f2d5f2ca78ea8b25f13f2a983c8fd58842f3816 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 6 Nov 2026 04:58:42 +0000 Subject: [PATCH 2/2] fix: type AVG(DISTINCT) and NULLIF(NULL, x) as Snowflake records them AVG(DISTINCT) of NUMBER(38,0) is NUMBER(38,3), not the NUMBER(38,6) of a plain AVG; other input types refuse until recorded. NULLIF with a NULL first argument is a NULL typed VARCHAR, not the second argument's type. --- .../sqlquack-rules/src/catalogue/constants.rs | 2 +- .../conversion/_helpers/expressions/calls.rs | 24 ++++++++++++++++++ crates/sqlquack/tests/cli/test_convert.rs | 25 +++++++++++++++++++ 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/crates/sqlquack-rules/src/catalogue/constants.rs b/crates/sqlquack-rules/src/catalogue/constants.rs index cd7dec18f..71911240f 100644 --- a/crates/sqlquack-rules/src/catalogue/constants.rs +++ b/crates/sqlquack-rules/src/catalogue/constants.rs @@ -402,7 +402,7 @@ pub const RULES: &[RuleSpec] = &[ RuleSpec { id: NULLIF, constructs: &["fn:NULLIF"], - conditions: "Arguments unify to a comparable type.", + conditions: "Arguments unify to a comparable type, the result typed as they unify. NULLIF(NULL, x) is NULL typed VARCHAR, as recorded.", 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 cc6abedba..1b4af0610 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/expressions/calls.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/expressions/calls.rs @@ -371,6 +371,12 @@ fn nullif(left: &Value, right: &Value, site: Site<'_>) -> Result { ))); }; let distinct = self.distinct(); + let data_type = match (distinct.is_empty(), argument.data_type) { + (true, _) => data_type, + ( + false, + SnowflakeType::Number { + precision: 38, + scale: 0, + }, + ) => SnowflakeType::Number { + precision: 38, + scale: 3, + }, + (false, other) => { + return Err(refuse(format!( + "AVG(DISTINCT) of {other}; its result type is recorded only for NUMBER(38,0)" + ))); + } + }; let counted = cast( &format!("count({distinct}{}){}", argument.sql, self.over), count(log), diff --git a/crates/sqlquack/tests/cli/test_convert.rs b/crates/sqlquack/tests/cli/test_convert.rs index 84a0f0e4a..b88ea4905 100644 --- a/crates/sqlquack/tests/cli/test_convert.rs +++ b/crates/sqlquack/tests/cli/test_convert.rs @@ -519,6 +519,31 @@ fn given_a_bracket_expression_with_a_lazy_spelling_when_converting_then_the_clas } } +#[test] +fn given_aggregates_and_nullif_with_recorded_types_when_running_then_values_follow_the_types() { + let test_cases = [ + ValueTestCase { + description: "NULLIF(NULL, x) is text, so COALESCE with text keeps the text", + sql: "SELECT COALESCE(NULLIF(NULL, 1), 'none') AS r", + expected_value: "none", + }, + ValueTestCase { + description: "AVG(DISTINCT) of NUMBER(38,0) keeps three decimals", + sql: "SELECT AVG(DISTINCT x)::VARCHAR AS r FROM (SELECT * FROM (VALUES \ + (1::NUMBER(38,0)), (2), (2)) AS v(x)) AS t", + expected_value: "1.500", + }, + ]; + for test_case in test_cases { + assert_eq!( + converted_first_value(test_case.sql), + test_case.expected_value, + "{}", + test_case.description + ); + } +} + #[test] fn given_count_of_a_qualified_or_filtered_star_when_running_then_rows_with_a_null_are_skipped() { let test_cases = [