From 6cca6800052916c615c39b4e3ba304ab2fea6fe7 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 6 Nov 2026 05:13:22 +0000 Subject: [PATCH 1/2] fix: charge json_fidelity only for VARIANT columns a statement may read A base table's VARIANT, OBJECT or ARRAY column was guarded, and charged json_fidelity, whenever the table was read, so strict mode refused SELECT ticket_id FROM tickets because of an unread ARRAY column. The guard and the charge now apply only when the statement names the column, uses a star other than COUNT(*)'s, or joins NATURAL; the guard's failure is loud, so a missed read raises rather than returns a wrong value. --- .../sqlquack-rules/src/catalogue/constants.rs | 2 +- .../conversion/_helpers/queries/sources.rs | 14 ++-- .../conversion/_helpers/support/locations.rs | 5 ++ .../src/conversion/_helpers/support/tree.rs | 65 +++++++++++++++++++ crates/sqlquack/tests/cli/test_convert.rs | 63 +++++++++++++++++- crates/sqlquack/tests/cli/test_types.rs | 6 ++ 6 files changed, 147 insertions(+), 8 deletions(-) diff --git a/crates/sqlquack-rules/src/catalogue/constants.rs b/crates/sqlquack-rules/src/catalogue/constants.rs index c6bab09bc..9f49d0568 100644 --- a/crates/sqlquack-rules/src/catalogue/constants.rs +++ b/crates/sqlquack-rules/src/catalogue/constants.rs @@ -85,7 +85,7 @@ pub const RULES: &[RuleSpec] = &[ RuleSpec { id: FROM_RELATION, constructs: &["node:table"], - conditions: "The relation is a CTE or has typed columns in the schema.", + conditions: "The relation is a CTE or has typed columns in the schema. A base table's VARIANT, OBJECT or ARRAY column the statement may read, by name, through a star other than COUNT(*)'s, or by a NATURAL join, is guarded as PARSE_JSON's result is and needs json_fidelity; a column it never reads needs nothing.", failures: &[], }, RuleSpec { diff --git a/crates/sqlquack-rules/src/conversion/_helpers/queries/sources.rs b/crates/sqlquack-rules/src/conversion/_helpers/queries/sources.rs index d80e69818..a6f6a4ba2 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/queries/sources.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/queries/sources.rs @@ -24,8 +24,8 @@ use crate::conversion::_helpers::support::collecting::{ use crate::conversion::_helpers::support::scope::{Context, Scope}; use crate::conversion::_helpers::support::site::{Placement, Site}; use crate::conversion::_helpers::support::tree::{ - Payload, field, function_construct, identifier_name, integer_literal, list, node_construct, - node_kind, payload_object, require_understood, + Payload, field, function_construct, identifier_name, integer_literal, list, may_read_column, + node_construct, node_kind, payload_object, require_understood, }; use crate::conversion::constants::{ ASOF_JOIN, ASOF_KIND, CORRELATED_DERIVED, FROM_RELATION, FROM_SUBQUERY, GENERATOR_MAX_ROWS, @@ -559,10 +559,12 @@ fn table_source( .unwrap_or_else(|| name.clone()); let is_cte = context.ctes.iter().any(|(cte, _)| cte == &name); let mut guarded: Vec = Vec::new(); - for column in columns - .iter() - .filter(|column| !is_cte && column.data_type == SnowflakeType::SemiStructured) - { + let statement = context.locator.tree(); + for column in columns.iter().filter(|column| { + !is_cte + && column.data_type == SnowflakeType::SemiStructured + && may_read_column(statement, &column.name) + }) { let quoted = quoted_identifier(&column.name, log); guarded.push(format!("{} AS {quoted}", guarded_column("ed, context)?)); } diff --git a/crates/sqlquack-rules/src/conversion/_helpers/support/locations.rs b/crates/sqlquack-rules/src/conversion/_helpers/support/locations.rs index ebb12352d..887a61051 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/support/locations.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/support/locations.rs @@ -52,6 +52,11 @@ impl<'a> Locator<'a> { } } + /// The statement tree the conversion reads. + pub(crate) fn tree(&self) -> &'a Value { + self.tree + } + /// `sql`, converted from `node`, with its unidentified raises identified and located at `node`. pub(crate) fn identified(&self, sql: &str, node: &Value) -> String { let (sql, ids) = identify_raises(sql); diff --git a/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs b/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs index b08730857..da80daa22 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/support/tree.rs @@ -47,6 +47,12 @@ pub(crate) type Payload = Map; pub(crate) const LITERAL_NUMBER: &str = "number"; /// The parser's literal type for strings. pub(crate) const LITERAL_STRING: &str = "string"; +/// The join kind prefix of NATURAL joins, which compare every column the two sides share. +const NATURAL_JOIN_KIND: &str = "Natural"; +/// The fields of an identifier node: its name and whether it was quoted. +const IDENTIFIER_FIELDS: [&str; 2] = ["name", "quoted"]; +/// The field holding a call's arguments. +const ARGUMENTS_FIELD: &str = "args"; /// The kind and payload of a one-key node such as `{"add": {...}}`. pub(crate) fn node_kind(node: &Value) -> Option<(&str, &Value)> { @@ -237,3 +243,62 @@ pub(crate) fn strip_positions(value: &Value) -> Value { other => other.clone(), } } + +/// Whether `tree` may read column `name`: by name, by a star other than COUNT(*)'s, or by NATURAL. +pub(crate) fn may_read_column(tree: &Value, name: &str) -> bool { + match tree { + Value::Array(items) => items.iter().any(|item| may_read_column(item, name)), + Value::Object(fields) => { + if fields.len() == IDENTIFIER_FIELDS.len() + && IDENTIFIER_FIELDS + .iter() + .all(|field| fields.contains_key(*field)) + { + return identifier_name(tree).is_some_and(|resolved| resolved == name); + } + match node_kind(tree) { + Some(("star", _)) => return true, + Some(("aggregate_function", payload)) if counts_rows(payload) => { + return payload.as_object().is_some_and(|call| { + call.iter().any(|(field, value)| { + field != ARGUMENTS_FIELD && may_read_column(value, name) + }) + }); + } + _ => {} + } + if fields + .get("kind") + .and_then(Value::as_str) + .is_some_and(|kind| kind.starts_with(NATURAL_JOIN_KIND)) + && fields.contains_key("using") + { + return true; + } + fields.values().any(|value| may_read_column(value, name)) + } + _ => false, + } +} + +/// Whether an aggregate call is COUNT(*) with a bare star, which counts rows and reads no column. +fn counts_rows(payload: &Value) -> bool { + let named_count = payload + .get("name") + .and_then(Value::as_str) + .is_some_and(|called| called.eq_ignore_ascii_case("count")); + let bare_star = match payload + .get("args") + .and_then(Value::as_array) + .map(Vec::as_slice) + { + Some([argument]) => match node_kind(argument) { + Some(("star", star)) => star + .as_object() + .is_some_and(|star| star.values().all(Value::is_null)), + _ => false, + }, + _ => false, + }; + named_count && bare_star +} diff --git a/crates/sqlquack/tests/cli/test_convert.rs b/crates/sqlquack/tests/cli/test_convert.rs index 304766dd3..199c98a94 100644 --- a/crates/sqlquack/tests/cli/test_convert.rs +++ b/crates/sqlquack/tests/cli/test_convert.rs @@ -9,7 +9,7 @@ use crate::test_types::{ ConvertedRunTestCase, CountedCallsTestCase, CrossingTestCase, DedicatedOverrideTestCase, DeepChainTestCase, DivergenceHelpTestCase, FailureKindTestCase, OutputFormatTestCase, OverrideFailureTestCase, OverrideTestCase, PolicyFreeTestCase, PythonRefusalTestCase, - PythonRunTestCase, RunErrorTestCase, SchemaErrorTestCase, ValueTestCase, + PythonRunTestCase, ReadColumnTestCase, RunErrorTestCase, SchemaErrorTestCase, ValueTestCase, }; use sha2::{Digest, Sha256}; @@ -1759,3 +1759,64 @@ fn given_a_malformed_schema_when_converting_then_the_error_names_what_is_wrong_i ); } } + +#[test] +fn given_a_table_with_an_unread_variant_column_when_converting_strictly_then_json_fidelity_is_not_needed() + { + let directory = tempfile::tempdir().expect("temporary directory"); + let schema = directory.path().join("schema.json"); + std::fs::write( + &schema, + r#"{"relations": {"TICKETS": [["TICKET_ID", "NUMBER(38,0)"], ["SUBJECT", "VARCHAR"], ["TAGS", "ARRAY"]], "ORDERS": [["ORDER_ID", "NUMBER(38,0)"]]}}"#, + ) + .expect("schema written"); + let schema = schema.to_string_lossy().into_owned(); + let test_cases = [ + ReadColumnTestCase { + description: "a scalar column of a table with an unread ARRAY column", + sql: "SELECT ticket_id FROM tickets", + expected_json_fidelity: false, + }, + ReadColumnTestCase { + description: "a UNION ALL whose branch reads only scalar columns", + sql: "SELECT order_id FROM orders UNION ALL SELECT ticket_id FROM tickets", + expected_json_fidelity: false, + }, + ReadColumnTestCase { + description: "the ARRAY column named in the select list", + sql: "SELECT ticket_id, tags FROM tickets", + expected_json_fidelity: true, + }, + ReadColumnTestCase { + description: "the ARRAY column read only in WHERE", + sql: "SELECT subject FROM tickets WHERE ARRAY_SIZE(tags) > 0", + expected_json_fidelity: true, + }, + ReadColumnTestCase { + description: "a star, which reads every column", + sql: "SELECT * FROM tickets", + expected_json_fidelity: true, + }, + ReadColumnTestCase { + description: "a NATURAL join, which compares columns without naming them", + sql: "SELECT t.ticket_id FROM tickets t NATURAL JOIN orders o", + expected_json_fidelity: true, + }, + ]; + for test_case in test_cases { + let (stdout, stderr) = + convert_streams(test_case.sql, &["--builtin-policy", "--schema", &schema]); + assert_eq!( + ( + stderr.contains("json_fidelity"), + stdout.trim_start().starts_with(['S', '(']) + ), + ( + test_case.expected_json_fidelity, + !test_case.expected_json_fidelity + ), + "{}: {stderr}", + test_case.description + ); + } +} diff --git a/crates/sqlquack/tests/cli/test_types.rs b/crates/sqlquack/tests/cli/test_types.rs index 191b9a7bf..d91e8d454 100644 --- a/crates/sqlquack/tests/cli/test_types.rs +++ b/crates/sqlquack/tests/cli/test_types.rs @@ -293,3 +293,9 @@ pub(crate) struct SchemaErrorTestCase { pub(crate) schema: &'static str, pub(crate) expected_error: &'static str, } + +pub(crate) struct ReadColumnTestCase { + pub(crate) description: &'static str, + pub(crate) sql: &'static str, + pub(crate) expected_json_fidelity: bool, +} From 4fcbd1011804e1e00b813e2cf6d62d41eb252185 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 6 Nov 2026 05:15:00 +0000 Subject: [PATCH 2/2] fix: keep the JSON guard on every base-table VARIANT column Only the json_fidelity charge depends on whether the statement may read the column; the guard stays, so a read the scan missed raises loudly instead of returning an unguarded value. --- .../_helpers/expressions/semi_structured.rs | 5 ++--- .../src/conversion/_helpers/queries/sources.rs | 17 ++++++++++------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/crates/sqlquack-rules/src/conversion/_helpers/expressions/semi_structured.rs b/crates/sqlquack-rules/src/conversion/_helpers/expressions/semi_structured.rs index 39014e1b0..423cdc2fa 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/expressions/semi_structured.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/expressions/semi_structured.rs @@ -84,9 +84,8 @@ pub(crate) fn parse_json( } /// A VARIANT column of a base table, guarded as PARSE_JSON's result is. -pub(crate) fn guarded_column(sql: &str, context: &Context<'_>) -> Result { - diverge(context, JSON_FIDELITY, &node_construct("column"))?; - Ok(json_guarded(sql, false, context.log)) +pub(crate) fn guarded_column(sql: &str, log: &ProvenanceLog) -> String { + json_guarded(sql, false, log) } /// `v:a.b[0]`: a literal path into a VARIANT. diff --git a/crates/sqlquack-rules/src/conversion/_helpers/queries/sources.rs b/crates/sqlquack-rules/src/conversion/_helpers/queries/sources.rs index a6f6a4ba2..5393a04d3 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/queries/sources.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/queries/sources.rs @@ -9,6 +9,7 @@ use sqlquack_core::refusals::main::outside_conditions::outside_conditions; use sqlquack_core::refusals::main::unresolved::unresolved; use sqlquack_core::refusals::models::{Refusal, RefusalCategory}; use sqlquack_duckdb::blocks::main::quoted_identifier::quoted_identifier; +use sqlquack_duckdb::semi_structured::constants::JSON_FIDELITY; use sqlquack_snowflake::typing::models::SnowflakeType; use crate::conversion::_helpers::expressions::project_functions::{exact_arguments, held_type}; @@ -21,6 +22,7 @@ use crate::conversion::_helpers::queries::query::{convert_query, require_distinc use crate::conversion::_helpers::support::collecting::{ collecting, events, opaque, record, recorded, replace_since, suspended, unresolved_since, }; +use crate::conversion::_helpers::support::divergence::diverge; use crate::conversion::_helpers::support::scope::{Context, Scope}; use crate::conversion::_helpers::support::site::{Placement, Site}; use crate::conversion::_helpers::support::tree::{ @@ -559,14 +561,15 @@ fn table_source( .unwrap_or_else(|| name.clone()); let is_cte = context.ctes.iter().any(|(cte, _)| cte == &name); let mut guarded: Vec = Vec::new(); - let statement = context.locator.tree(); - for column in columns.iter().filter(|column| { - !is_cte - && column.data_type == SnowflakeType::SemiStructured - && may_read_column(statement, &column.name) - }) { + for column in columns + .iter() + .filter(|column| !is_cte && column.data_type == SnowflakeType::SemiStructured) + { + if may_read_column(context.locator.tree(), &column.name) { + diverge(context, JSON_FIDELITY, &node_construct("column"))?; + } let quoted = quoted_identifier(&column.name, log); - guarded.push(format!("{} AS {quoted}", guarded_column("ed, context)?)); + guarded.push(format!("{} AS {quoted}", guarded_column("ed, log))); } let relation = match guarded.is_empty() { true => quoted_identifier(&name, log),