From 264165c51aa178bbce05b256c4d2b4ede7c66935 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 6 Nov 2026 22:17:27 +0000 Subject: [PATCH 1/2] fix: convert the clock under the built-in policy and guard only semi-structured columns a query reads The built-in default now converts CURRENT_DATE() and CURRENT_TIMESTAMP() with a warning that states the caveat: both engines take the value when the query runs, in the session's time zone. Strict mode, --loud-divergences-only and a session outside UTC still refuse them. A base table's VARIANT, ARRAY and OBJECT columns are guarded, relying on json_fidelity, only where the statement can read them: it names the column, or reads columns without naming them through a star other than COUNT(*), a positional column, PIVOT, UNPIVOT or a NATURAL join. Reading another column of such a table no longer needs json_fidelity. --- .../src/diagnostics/constants.rs | 2 +- .../conversion/_helpers/queries/sources.rs | 62 ++++++- .../conversion/_helpers/support/locations.rs | 5 + .../sqlquack-rules/src/session/constants.rs | 14 ++ .../main/builtin_conversion_options.rs | 33 ++++ crates/sqlquack-rules/src/session/main/mod.rs | 1 + .../main/session_conversion_options.rs | 18 +- .../src/cli/_helpers/convert_command.rs | 26 ++- crates/sqlquack/src/cli/_helpers/evidence.rs | 25 ++- crates/sqlquack/src/cli/models.rs | 2 +- crates/sqlquack/tests/cli.rs | 2 + .../cli/fixtures/semi_structured_tickets.json | 10 + crates/sqlquack/tests/cli/helpers.rs | 7 +- .../sqlquack/tests/cli/test_builtin_policy.rs | 175 ++++++++++++++++++ crates/sqlquack/tests/cli/test_convert.rs | 87 +-------- website/public/playground/index.html | 2 +- .../docs/docs/diagnostics/reference.mdx | 2 +- website/src/content/docs/docs/policy.mdx | 7 +- website/src/content/docs/docs/quickstart.mdx | 3 +- 19 files changed, 377 insertions(+), 106 deletions(-) create mode 100644 crates/sqlquack-rules/src/session/main/builtin_conversion_options.rs create mode 100644 crates/sqlquack/tests/cli/fixtures/semi_structured_tickets.json create mode 100644 crates/sqlquack/tests/cli/test_builtin_policy.rs diff --git a/crates/sqlquack-core/src/diagnostics/constants.rs b/crates/sqlquack-core/src/diagnostics/constants.rs index 87894fa98..8b37bb245 100644 --- a/crates/sqlquack-core/src/diagnostics/constants.rs +++ b/crates/sqlquack-core/src/diagnostics/constants.rs @@ -155,7 +155,7 @@ pub const CODES: &[CodeEntry] = &[ help: Some( "the help names the divergence and the fixes that work in the current mode: allow \ it in the policy file, drop --strict or --loud-divergences-only, or confirm its \ - assumption; the built-in policy refuses every one", + assumption; the built-in policy refuses every one but the clock outside strict mode", ), explanation: "The construct at the span converts only by relying on the known \ divergence the label names, where DuckDB may raise or differ from Snowflake as the \ diff --git a/crates/sqlquack-rules/src/conversion/_helpers/queries/sources.rs b/crates/sqlquack-rules/src/conversion/_helpers/queries/sources.rs index 159e51984..cf34d04c2 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/queries/sources.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/queries/sources.rs @@ -25,8 +25,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, is_inert, list, + 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, @@ -35,6 +35,9 @@ use crate::conversion::constants::{ TABLE_WRAPPER, UNALIASED_SUBQUERY_PREFIX, USING_ALIAS_PREFIX, }; use crate::conversion::constants::{FULL_JOIN_KEYWORD, INNER_JOIN_KEYWORD}; + +/// How the parsed tree spells a NATURAL join's kind, lower-cased. +const NATURAL_JOIN_PREFIX: &str = "natural"; use sqlquack_semantics::lineage::main::stored_columns::stored_columns; use sqlquack_semantics::names::constants::RELATION_KEY_SEPARATOR; use sqlquack_semantics::names::main::relation_key::relation_key; @@ -598,6 +601,7 @@ fn table_source( for column in columns .iter() .filter(|column| !is_cte && column.data_type == SnowflakeType::SemiStructured) + .filter(|column| may_read(context.locator.tree(), &column.name)) { let quoted = quoted_identifier(&column.name, log); guarded.push(format!("{} AS {quoted}", guarded_column("ed, context)?)); @@ -886,3 +890,57 @@ fn reads_enclosing(query: &Value, outer: &Scope<'_>, context: &Context<'_>) -> b Err(refusal) => refusal.reason == CORRELATED_DERIVED, } } + +/// Whether `tree` names column `name`, or reads columns unnamed: `*`, `$n`, PIVOT or NATURAL. +fn may_read(tree: &Value, name: &str) -> bool { + let plain = !name.is_empty() + && name + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || byte == b'_' || byte == b'$'); + !plain || reads_unnamed_or(tree, name) +} + +/// The walk behind `may_read`. +fn reads_unnamed_or(tree: &Value, name: &str) -> bool { + match tree { + Value::String(text) => { + text.eq_ignore_ascii_case(name) + || text.to_ascii_lowercase().starts_with(NATURAL_JOIN_PREFIX) + } + Value::Array(items) => items.iter().any(|item| reads_unnamed_or(item, name)), + Value::Object(fields) => fields.iter().any(|(key, value)| match key.as_str() { + "star" | "parameter" | "pivot" | "unpivot" | "columns" => true, + "count" | "aggregate_function" if counts_rows(value) => false, + _ => reads_unnamed_or(value, name), + }), + _ => false, + } +} + +/// Whether a COUNT payload is COUNT(*) alone, which reads no column. +fn counts_rows(payload: &Value) -> bool { + let Some(fields) = payload.as_object() else { + return false; + }; + let star = + |value: &Value| matches!(node_kind(value), Some(("star", star)) if is_inert_star(star)); + let named_count = fields + .get("name") + .and_then(Value::as_str) + .is_none_or(|called| called.eq_ignore_ascii_case("COUNT")); + named_count + && fields.iter().all(|(key, value)| match key.as_str() { + "this" => star(value), + "args" => value + .as_array() + .is_some_and(|items| items.len() == 1 && star(&items[0])), + "name" => true, + _ => is_inert(value), + }) +} + +/// A bare `*`, without a qualifier, ILIKE, EXCLUDE, REPLACE or RENAME. +fn is_inert_star(star: &Value) -> bool { + star.as_object() + .is_none_or(|fields| fields.values().all(is_inert)) +} diff --git a/crates/sqlquack-rules/src/conversion/_helpers/support/locations.rs b/crates/sqlquack-rules/src/conversion/_helpers/support/locations.rs index f10b289cb..51bbaf414 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 being converted, positions stripped. + 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/session/constants.rs b/crates/sqlquack-rules/src/session/constants.rs index 96be33069..b8bbe7b7a 100644 --- a/crates/sqlquack-rules/src/session/constants.rs +++ b/crates/sqlquack-rules/src/session/constants.rs @@ -2,3 +2,17 @@ /// The profile name reported when the built-in default stands in for a policy. pub const BUILTIN_PROFILE: &str = "the built-in default"; + +/// The clock functions the built-in default converts with a warning outside strict mode. +pub const BUILTIN_CLOCK_CONSTRUCTS: [&str; 2] = ["fn:CURRENT_DATE", "fn:CURRENT_TIMESTAMP"]; + +/// Why the built-in default converts the clock functions, printed with the warning. +pub const BUILTIN_CLOCK_REASON: &str = "CURRENT_DATE() and CURRENT_TIMESTAMP() take their value \ + when the query runs, in DuckDB as in Snowflake, so the value differs between runs; the \ + converted SQL reads DuckDB's clock in the DuckDB session's time zone, which must match \ + Snowflake's TIMEZONE (UTC) for the date and time to agree. --strict refuses them"; + +/// The session time zone the clock functions rest on under the built-in default. +pub const BUILTIN_CLOCK_ZONE: &str = "CURRENT_DATE() and CURRENT_TIMESTAMP() give the date and \ + time in the session's time zone, so they agree only where Snowflake's TIMEZONE and DuckDB's \ + TimeZone are both UTC"; diff --git a/crates/sqlquack-rules/src/session/main/builtin_conversion_options.rs b/crates/sqlquack-rules/src/session/main/builtin_conversion_options.rs new file mode 100644 index 000000000..1e5ff262b --- /dev/null +++ b/crates/sqlquack-rules/src/session/main/builtin_conversion_options.rs @@ -0,0 +1,33 @@ +//! `builtin_conversion_options`: the built-in default's options, which allow only the clock. + +use crate::conversion::models::{AllowedDivergence, ConversionOptions}; +use crate::session::constants::{ + BUILTIN_CLOCK_CONSTRUCTS, BUILTIN_CLOCK_REASON, BUILTIN_CLOCK_ZONE, +}; +use sqlquack_policy::policy::constants::{CURRENT_TIME, SESSION_TIMEZONE_UTC}; + +/// The built-in default's options: the clock converts with a warning unless `refuse_clock`. +pub fn builtin_conversion_options(refuse_clock: bool) -> ConversionOptions { + let mut options = ConversionOptions::default(); + if refuse_clock { + return options; + } + let constructs: Vec = BUILTIN_CLOCK_CONSTRUCTS + .iter() + .map(|construct| (*construct).to_owned()) + .collect(); + for (divergence, reason, assumption) in [ + (CURRENT_TIME, BUILTIN_CLOCK_REASON, false), + (SESSION_TIMEZONE_UTC, BUILTIN_CLOCK_ZONE, true), + ] { + options.divergences.insert( + divergence.to_owned(), + AllowedDivergence { + constructs: constructs.clone(), + reason: reason.to_owned(), + assumption, + }, + ); + } + options +} diff --git a/crates/sqlquack-rules/src/session/main/mod.rs b/crates/sqlquack-rules/src/session/main/mod.rs index eee42cc48..8c0b7f4c6 100644 --- a/crates/sqlquack-rules/src/session/main/mod.rs +++ b/crates/sqlquack-rules/src/session/main/mod.rs @@ -1,3 +1,4 @@ +pub mod builtin_conversion_options; pub mod confirm_utc_session; pub mod conversion_options; pub mod parse_session; diff --git a/crates/sqlquack-rules/src/session/main/session_conversion_options.rs b/crates/sqlquack-rules/src/session/main/session_conversion_options.rs index 6dd209056..507aacf93 100644 --- a/crates/sqlquack-rules/src/session/main/session_conversion_options.rs +++ b/crates/sqlquack-rules/src/session/main/session_conversion_options.rs @@ -12,6 +12,7 @@ use sqlquack_policy::policy::models::{Policy, SessionValue}; use crate::session::_helpers::session_profiles::{matching_profile, value_text}; use crate::session::constants::BUILTIN_PROFILE; +use crate::session::main::builtin_conversion_options::builtin_conversion_options; use crate::session::main::confirm_utc_session::confirm_utc_session; use crate::session::main::conversion_options::conversion_options; @@ -28,10 +29,19 @@ pub fn session_conversion_options( .filter(|name| !builtin_accepts(name)) .collect(); return match unknown.is_empty() { - true => Ok(( - with_namespace(ConversionOptions::default(), parameters), - BUILTIN_PROFILE.to_owned(), - )), + true => { + let mut options = builtin_conversion_options(strict); + if parameters + .get(TIMEZONE_PARAMETER) + .is_some_and(|zone| zone != SESSION_TIME_ZONE) + { + options.divergences.remove(SESSION_TIMEZONE_UTC); + } + Ok(( + with_namespace(options, parameters), + BUILTIN_PROFILE.to_owned(), + )) + } false => Err(format!( "the built-in default records no session profiles, so it takes only TIMEZONE, \ DATABASE, SCHEMA and the result-neutral parameters, not {}; pass --policy with a policy file whose \ diff --git a/crates/sqlquack/src/cli/_helpers/convert_command.rs b/crates/sqlquack/src/cli/_helpers/convert_command.rs index e45c23f31..a89e6e28b 100644 --- a/crates/sqlquack/src/cli/_helpers/convert_command.rs +++ b/crates/sqlquack/src/cli/_helpers/convert_command.rs @@ -243,6 +243,10 @@ pub(crate) fn chosen_options( ) -> Result { let options = match policy { Some(policy) => caller_choices(options, policy, &arguments.conversion), + None if arguments.conversion.loud_divergences_only => ConversionOptions { + divergences: Default::default(), + ..options + }, None => options, }; with_overrides( @@ -266,10 +270,15 @@ pub(crate) fn analysis_report( let assumptions: Vec = provenance .divergences .keys() - .filter(|id| { - policy - .and_then(|policy| policy.divergences.get(assumption_id(id))) - .is_some_and(|divergence| divergence.assumption) + .filter(|id| match policy { + Some(policy) => policy + .divergences + .get(assumption_id(id)) + .is_some_and(|divergence| divergence.assumption), + None => options + .divergences + .get(id.as_str()) + .is_some_and(|divergence| divergence.assumption), }) .cloned() .collect(); @@ -284,6 +293,15 @@ pub(crate) fn analysis_report( ) }) .into_iter() + .chain( + provenance + .divergences + .keys() + .filter(|_| policy.is_none()) + .filter_map(|id| options.divergences.get(id.as_str())) + .filter(|divergence| !divergence.assumption) + .map(|divergence| divergence.reason.clone()), + ) .collect(); Ok(ConvertReport { converted, diff --git a/crates/sqlquack/src/cli/_helpers/evidence.rs b/crates/sqlquack/src/cli/_helpers/evidence.rs index fb5f3e4b5..4ed7f8df5 100644 --- a/crates/sqlquack/src/cli/_helpers/evidence.rs +++ b/crates/sqlquack/src/cli/_helpers/evidence.rs @@ -17,7 +17,10 @@ use crate::analysis::main::assumption_id::assumption_id; use crate::analysis::main::confirm_assumption::confirm_assumption; use crate::analysis::main::refuse_value_divergences::refuse_value_divergences; use crate::cli::models::{EvidencePaths, PolicyArguments}; -use sqlquack_policy::policy::constants::{DECLARED_OFFSET, PROJECT_UDF, SESSION_TIMEZONE_UTC}; +use sqlquack_policy::policy::constants::{ + CURRENT_TIME, DECLARED_OFFSET, PROJECT_UDF, SESSION_TIMEZONE_UTC, +}; +use sqlquack_rules::session::main::builtin_conversion_options::builtin_conversion_options; use sqlquack_rules::session::main::confirm_utc_session::confirm_utc_session; use sqlquack_rules::session::main::conversion_options::conversion_options; @@ -44,7 +47,7 @@ pub(crate) fn chosen_policy(path: Option<&Path>, builtin: bool) -> Result