From cad4c1e714fa07d31b9e81c7acde7d486813d9ad Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:32:32 +0100 Subject: [PATCH 01/11] feat: ANY SQL macro arguments --- .../_helpers/sql_macro_parts.rs | 1 + .../src/sql_macros/_helpers/arguments.rs | 8 +- .../src/sql_macros/_helpers/body.rs | 2 +- .../src/sql_macros/_helpers/expansion.rs | 9 +- .../src/sql_macros/_helpers/header.rs | 20 +++- .../sql_macros/_helpers/literal_positions.rs | 3 +- .../src/sql_macros/constants.rs | 4 +- .../src/sql_macros/models.rs | 3 + .../src/sql_macros/tests/test_definitions.rs | 31 +++++ .../src/sql_macros/tests/test_expansion.rs | 35 ++++++ .../sql_macros/test_sql_macro_types.py | 108 ++++++++++++++++++ .../docs/docs/concepts/macros/sql-macros.mdx | 8 ++ 12 files changed, 225 insertions(+), 7 deletions(-) create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py diff --git a/crates/sqlbuild-analysis/src/definition_changes/_helpers/sql_macro_parts.rs b/crates/sqlbuild-analysis/src/definition_changes/_helpers/sql_macro_parts.rs index 9bd4f7685f..7fdce06669 100644 --- a/crates/sqlbuild-analysis/src/definition_changes/_helpers/sql_macro_parts.rs +++ b/crates/sqlbuild-analysis/src/definition_changes/_helpers/sql_macro_parts.rs @@ -7,6 +7,7 @@ use crate::sql_macros::models::{ParameterKind, SqlMacroParameter}; pub(crate) fn parameter_text(parameter: &SqlMacroParameter) -> String { let kind: String = match ¶meter.kind { ParameterKind::Expression { data_type } => format!("expression {}", type_text(data_type)), + ParameterKind::Any => "expression any".to_owned(), ParameterKind::Constant { data_type } => format!("const {}", type_text(data_type)), ParameterKind::Collection { set, data_type } => { format!("collection {set} {}", type_text(data_type)) diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/arguments.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/arguments.rs index c696813797..76769a2286 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/arguments.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/arguments.rs @@ -350,6 +350,11 @@ pub(crate) fn check_shape( Err(expects("a SQL expression")) } (ParameterKind::Expression { .. }, _) => Ok(()), + (ParameterKind::Any, ArgumentShape::InlineCollection | ArgumentShape::Reference { .. }) => { + Err(expects("a scalar SQL expression") + .helped("an ANY argument takes one value per row, such as a column, a literal or an expression; pass lists to LIST arguments and relations to RELATION arguments")) + } + (ParameterKind::Any, _) => Ok(()), (ParameterKind::Constant { .. }, ArgumentShape::ConstantReference { .. }) => Ok(()), (ParameterKind::Constant { .. }, _) => Err(expects("a literal, a scalar constant or an enum value") .helped(format!("pass a literal, such as 2, or a constant, such as @const(\"{name}\"); a CONST argument is known at compile time, so a column or expression cannot be passed"))), @@ -386,9 +391,10 @@ pub(crate) fn check_shape( /// Whether an argument of kind `outer`, forwarded as `@name`, fits a parameter of kind `inner`. fn forwards(outer: &ParameterKind, inner: &ParameterKind) -> bool { match inner { - ParameterKind::Expression { .. } => matches!( + ParameterKind::Expression { .. } | ParameterKind::Any => matches!( outer, ParameterKind::Expression { .. } + | ParameterKind::Any | ParameterKind::Constant { .. } | ParameterKind::Enum { .. } | ParameterKind::ColumnOf { .. } diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/body.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/body.rs index 69de0a50a4..90a597e4ba 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/body.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/body.rs @@ -216,7 +216,7 @@ fn substituted( _ => argument.sql.clone(), } } - ParameterKind::Expression { .. } | ParameterKind::Constant { .. } + ParameterKind::Expression { .. } | ParameterKind::Any | ParameterKind::Constant { .. } if !is_atomic(&argument.sql) => { format!("({})", argument.sql) diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/expansion.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/expansion.rs index 6962997c11..5b59135c67 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/expansion.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/expansion.rs @@ -294,7 +294,10 @@ fn rendered_argument( } host.render_declarations(scope, text)? } - (ParameterKind::Constant { .. }, ArgumentShape::ConstantReference { name }) => { + ( + ParameterKind::Constant { .. } | ParameterKind::Any, + ArgumentShape::ConstantReference { name }, + ) => { if host.constant_shape(scope, name)? != ConstantShape::Scalar { return Err(expects("a scalar constant")); } @@ -516,7 +519,9 @@ fn check_adapter_types( .iter() .map(|(_, data_type)| data_type.as_str()) .collect(), - ParameterKind::Enum { .. } | ParameterKind::ColumnOf { .. } => Vec::new(), + ParameterKind::Any | ParameterKind::Enum { .. } | ParameterKind::ColumnOf { .. } => { + Vec::new() + } }; for data_type in types { if let Some(error) = invalid( diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/header.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/header.rs index f3dc32cafe..46e2d62be8 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/header.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/header.rs @@ -2,7 +2,7 @@ use crate::sql_macros::_helpers::lexing::{Token, TokenKind, closing_token, line_column, tokens}; use crate::sql_macros::constants::{ - ARGUMENTS_KEY, COLUMN_KEYWORD, COMMA, CONST_KEYWORD, DEFAULT_KEYWORD, DESCRIPTION_KEY, + ANY_KEYWORD, ARGUMENTS_KEY, COLUMN_KEYWORD, COMMA, CONST_KEYWORD, DEFAULT_KEYWORD, DESCRIPTION_KEY, ENUM_KEYWORD, HEADER_KEYWORD, INVALID_DEFINITION_CODE, LIST_KEYWORD, OF_KEYWORD, RAW_TYPE_NAMES, RELATION_COLUMN_MIN_TOKENS, RELATION_KEYWORD, RETURNS_KEY, SET_KEYWORD, }; @@ -259,10 +259,28 @@ impl HeaderParser<'_> { )), }; } + if first.is_word(source, ANY_KEYWORD) { + let Some(after) = tokens.get(1) else { + return Ok(ParameterKind::Any); + }; + if after.is_word(source, CONST_KEYWORD) { + return Err(self + .error(after.start, format!("argument {name} ANY CONST: a CONST argument needs a type its values are checked against")) + .helped(format!("declare it with the type of its values, such as {name} VARCHAR CONST"))); + } + return Err(self + .error(after.start, format!("argument {name} ANY takes nothing after it; ANY accepts any scalar expression")) + .helped(format!("declare it {name} ANY, or give it a SQL type"))); + } if first.is_word(source, LIST_KEYWORD) || first.is_word(source, SET_KEYWORD) { let set: bool = first.is_word(source, SET_KEYWORD); let keyword: String = first.text(source).to_ascii_uppercase(); let element: &[Token] = &tokens[1..]; + if element.len() == 1 && element[0].is_word(source, ANY_KEYWORD) { + return Err(self + .error(element[0].start, format!("argument {name} {keyword} ANY: ANY is for scalar arguments only")) + .helped(format!("declare the element type, such as {name} {keyword} VARCHAR"))); + } if let Some((last, element)) = element.split_last() && last.is_word(source, CONST_KEYWORD) { diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/literal_positions.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/literal_positions.rs index c904ea4759..71e8a26965 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/literal_positions.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/literal_positions.rs @@ -39,7 +39,7 @@ pub(crate) fn non_literal_use<'r>( let parameter = definition.parameter(&reference.name)?; if !matches!( parameter.kind, - ParameterKind::Expression { .. } | ParameterKind::ColumnOf { .. } + ParameterKind::Expression { .. } | ParameterKind::Any | ParameterKind::ColumnOf { .. } ) { return None; } @@ -232,6 +232,7 @@ pub(crate) fn literal_error( Some(ParameterKind::Expression { data_type }) => { format!("declare it {name} {data_type} CONST") } + Some(ParameterKind::Any) => format!("declare it with a type and CONST, such as {name} VARCHAR CONST"), _ => format!("pass the value through a CONST argument, such as {name}_value VARCHAR CONST"), }; SqlMacroError::new(LITERAL_REQUIRED_CODE, format!("{name} is used where a literal is required; {help}")) diff --git a/crates/sqlbuild-analysis/src/sql_macros/constants.rs b/crates/sqlbuild-analysis/src/sql_macros/constants.rs index e7905d424f..731dda31cc 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/constants.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/constants.rs @@ -41,7 +41,9 @@ pub(crate) const SET_KEYWORD: &str = "SET"; /// Names a parameter may not take, because `@name` already means something else. pub(crate) const RESERVED_PARAMETER_NAMES: [&str; 5] = ["sql", "py", "const", "enum", "param"]; /// Type names that would make a parameter raw, untyped text. -pub(crate) const RAW_TYPE_NAMES: [&str; 4] = ["RAW", "SQL", "ANY", "EXPRESSION"]; +pub(crate) const RAW_TYPE_NAMES: [&str; 3] = ["RAW", "SQL", "EXPRESSION"]; +/// The type of a parameter that takes any scalar expression and checks no type. +pub(crate) const ANY_KEYWORD: &str = "ANY"; /// Reference functions a SQL macro body may not call. pub(crate) const REFERENCE_FUNCTIONS: [&str; 3] = ["__ref", "__source", "__seed"]; /// The prefix of a SQL macro call. diff --git a/crates/sqlbuild-analysis/src/sql_macros/models.rs b/crates/sqlbuild-analysis/src/sql_macros/models.rs index f5d6c32ba5..5d93f65409 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/models.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/models.rs @@ -29,6 +29,8 @@ impl SqlMacroForm { pub enum ParameterKind { /// Any SQL expression of a compatible type. Expression { data_type: String }, + /// Any scalar SQL expression, whatever its type. + Any, /// A literal or a scalar constant. Constant { data_type: String }, /// An inline list or set, or a list or set constant; `set` for `SET`. @@ -46,6 +48,7 @@ impl ParameterKind { pub fn label(&self) -> String { match self { Self::Expression { data_type } => data_type.clone(), + Self::Any => "ANY".to_owned(), Self::Constant { data_type } => format!("{data_type} CONST"), Self::Collection { set, data_type } => { format!("{} {data_type}", if *set { "SET" } else { "LIST" }) diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/test_definitions.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/test_definitions.rs index c6fa3a4f3b..29c6fa4329 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests/test_definitions.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/test_definitions.rs @@ -35,6 +35,13 @@ fn given_valid_macro_files_when_parsing_then_each_parameter_and_the_body_are_rea expected_form: "CTE-list", expected_body_position: (2, 1), }, + DefinitionTestCase { + description: "ANY parameter, with and without a default", + source: "MACRO (description \"x\", arguments (store VARCHAR, number ANY, separator ANY DEFAULT '::'), returns VARCHAR);\n@store || @separator || CAST(@number AS VARCHAR)", + expected_labels: &["VARCHAR", "ANY", "ANY"], + expected_form: "expression", + expected_body_position: (2, 1), + }, DefinitionTestCase { description: "CONST parameter as LIMIT", source: "MACRO (description \"x\", arguments (src RELATION (id BIGINT), n BIGINT CONST));\nWITH firsts AS (SELECT id FROM @src LIMIT @n)", @@ -180,6 +187,30 @@ fn given_invalid_macro_files_when_parsing_then_each_is_rejected() { expected_code: "P015", expected_message: "argument codes LIST VARCHAR CONST: a LIST argument is always known at compile time, so it takes no CONST", }, + DefinitionErrorTestCase { + description: "ANY with CONST", + source: "MACRO (description \"x\", arguments (n ANY CONST), returns BIGINT);\n@n", + expected_code: "P015", + expected_message: "argument n ANY CONST: a CONST argument needs a type its values are checked against", + }, + DefinitionErrorTestCase { + description: "ANY with more after it", + source: "MACRO (description \"x\", arguments (n ANY[]), returns BIGINT);\n@n", + expected_code: "P015", + expected_message: "argument n ANY takes nothing after it", + }, + DefinitionErrorTestCase { + description: "LIST of ANY", + source: "MACRO (description \"x\", arguments (codes LIST ANY), returns BOOLEAN);\n'a' IN @codes", + expected_code: "P015", + expected_message: "argument codes LIST ANY: ANY is for scalar arguments only", + }, + DefinitionErrorTestCase { + description: "ANY parameter as LIMIT", + source: "MACRO (description \"x\", arguments (src RELATION (id BIGINT), n ANY));\nWITH firsts AS (SELECT id FROM @src LIMIT @n)", + expected_code: "P023", + expected_message: "n is used where a literal is required; declare it with a type and CONST, such as n VARCHAR CONST", + }, DefinitionErrorTestCase { description: "array type with CONST", source: "MACRO (description \"x\", arguments (codes VARCHAR[] CONST), returns BOOLEAN);\nlen(@codes) > 0", diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/test_expansion.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/test_expansion.rs index 167a3f279f..6307803193 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests/test_expansion.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/test_expansion.rs @@ -117,6 +117,10 @@ const SPACED_DECLARATIONS: (&str, &str) = ( "spaced", "MACRO (description \"Spaced\", arguments (status VARCHAR, places INT), returns BOOLEAN);\n\n@status <> @enum (\"order_status\").CLOSED AND @places = @const (\"price_places\")\n", ); +const ORDER_KEY: (&str, &str) = ( + "order_key", + "MACRO (description \"Order key\", arguments (store_code VARCHAR, order_number ANY), returns VARCHAR);\n\n@store_code || '::' || CAST(@order_number AS VARCHAR)\n", +); const ODD: (&str, &str) = ( "odd", "MACRO (description \"Odd\", arguments (v FOO BAR), returns BIGINT);\n\n@v + 1\n", @@ -349,6 +353,13 @@ fn given_calls_of_each_form_when_expanding_then_the_ticket_examples_compile() { sql: "SELECT @sql.has_x(o.a) AS l FROM orders AS o", expected_sql: "SELECT CONTAINS(o.a, 'x') -- why x\n AS l FROM orders AS o", }, + ExpansionTestCase { + description: "ANY arguments of any type: a column, a number, a string, an expression and a scalar constant", + adapter: "duckdb", + macros: &[ORDER_KEY], + sql: "SELECT @sql.order_key(o.store, o.number) AS a, @sql.order_key(o.store, 42) AS b, @sql.order_key(o.store, 'x') AS c, @sql.order_key(o.store, o.number + 1) AS d, @sql.order_key(o.store, @const(\"price_places\")) AS e FROM orders AS o", + expected_sql: "SELECT o.store || '::' || CAST(o.number AS VARCHAR) AS a, o.store || '::' || CAST(42 AS VARCHAR) AS b, o.store || '::' || CAST('x' AS VARCHAR) AS c, o.store || '::' || CAST((o.number + 1) AS VARCHAR) AS d, o.store || '::' || CAST(2 AS VARCHAR) AS e FROM orders AS o", + }, ExpansionTestCase { description: "a body ending in a line comment, called last on its line, adds no line", adapter: "", @@ -522,6 +533,30 @@ fn given_invalid_calls_when_expanding_then_each_error_names_the_problem() { expected_code: "P017", expected_message: "argument factor of macro scaled() expects INT CONST", }, + ExpansionErrorTestCase { + description: "a list passed to an ANY argument", + adapter: "", + macros: &[ORDER_KEY], + sql: "SELECT @sql.order_key(o.store, [1, 2]) AS k FROM orders AS o", + expected_code: "P017", + expected_message: "argument order_number of macro order_key() expects ANY: a scalar SQL expression; got [1, 2]", + }, + ExpansionErrorTestCase { + description: "a list constant passed to an ANY argument", + adapter: "", + macros: &[ORDER_KEY], + sql: "SELECT @sql.order_key(o.store, @const(\"supported_countries\")) AS k FROM orders AS o", + expected_code: "P017", + expected_message: "argument order_number of macro order_key() expects ANY: a scalar constant", + }, + ExpansionErrorTestCase { + description: "a relation passed to an ANY argument", + adapter: "", + macros: &[ORDER_KEY], + sql: "SELECT @sql.order_key(o.store, __ref(\"orders\")) AS k FROM orders AS o", + expected_code: "P017", + expected_message: "argument order_number of macro order_key() expects ANY: a scalar SQL expression", + }, ExpansionErrorTestCase { description: "type the adapter cannot parse", adapter: "duckdb", diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py new file mode 100644 index 0000000000..9a4e2311b4 --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py @@ -0,0 +1,108 @@ +"""E2E coverage for ANY SQL macro arguments.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.compile.sql_macros._test_types import ( + SqlMacroBuildTestCase, + SqlMacroErrorTestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.main.compile.sql_macros.helpers import ( + DATABASE, + orders_model, + output, + run_project, +) +from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import query_duckdb + +_ORDER_KEY: dict[str, str] = { + "macros/sql/order_key.sql": ( + 'MACRO (\n description "Order key from the country and the order",\n' + " arguments (country_code VARCHAR, order_ref ANY),\n" + " returns VARCHAR,\n);\n\n" + "@country_code || '::' || CAST(@order_ref AS VARCHAR)\n" + ) +} + + +@pytest.mark.parametrize( + "test_case", + [ + SqlMacroBuildTestCase( + description="any_argument_takes_a_number_a_string_and_an_expression", + files={ + **_ORDER_KEY, + **orders_model( + "SELECT order_id,\n" + " @sql.order_key(country_code, order_id) AS by_number,\n" + " @sql.order_key(country_code, status) AS by_text,\n" + " @sql.order_key(country_code, amount_cents / 100) AS by_expression\n" + 'FROM __ref("stg_orders")\nWHERE order_id = 2\n' + ), + }, + query="SELECT by_number, by_text, by_expression FROM orders", + expected_rows=[("FR::2", "FR::closed", "FR::5.0")], + ), + ], + ids=lambda case: case.description, +) +def test_given_an_any_argument_when_building_then_callers_pass_values_of_any_type( + test_case: SqlMacroBuildTestCase, tmp_path: Path +) -> None: + project_dir, result = run_project(tmp_path=tmp_path, files=test_case.files, command=("build",)) + + assert result.returncode == 0, output(result) + assert "P017" not in output(result) + assert ( + query_duckdb(db_path=project_dir / DATABASE, sql=test_case.query) == test_case.expected_rows + ) + + +@pytest.mark.parametrize( + "test_case", + [ + SqlMacroErrorTestCase( + description="a_list_passed_to_an_any_argument", + files={ + **_ORDER_KEY, + **orders_model( + 'SELECT @sql.order_key(country_code, [1, 2]) AS k FROM __ref("stg_orders")\n' + ), + }, + expected_code="P017", + expected_fragments=( + "argument order_ref of macro order_key() expects ANY: a scalar SQL expression", + ), + ), + SqlMacroErrorTestCase( + description="any_with_const", + files={ + "macros/sql/order_key.sql": ( + 'MACRO (description "Key", arguments (order_ref ANY CONST), ' + "returns VARCHAR);\n\nCAST(@order_ref AS VARCHAR)\n" + ), + **orders_model('SELECT @sql.order_key(1) AS k FROM __ref("stg_orders")\n'), + }, + expected_code="P015", + expected_fragments=( + "argument order_ref ANY CONST: a CONST argument needs a type its values are " + "checked against", + ), + ), + ], + ids=lambda case: case.description, +) +def test_given_an_any_argument_misused_when_compiling_then_it_fails( + test_case: SqlMacroErrorTestCase, tmp_path: Path +) -> None: + _, result = run_project( + tmp_path=tmp_path, files=test_case.files, command=("compile", "--no-cache") + ) + + assert result.returncode == 1, output(result) + assert f"error[{test_case.expected_code}]" in output(result) + for fragment in test_case.expected_fragments: + assert fragment in output(result) diff --git a/website/src/content/docs/docs/concepts/macros/sql-macros.mdx b/website/src/content/docs/docs/concepts/macros/sql-macros.mdx index f9b16676e7..f6ef59039f 100644 --- a/website/src/content/docs/docs/concepts/macros/sql-macros.mdx +++ b/website/src/content/docs/docs/concepts/macros/sql-macros.mdx @@ -139,6 +139,7 @@ Every parameter has a type, and may have a `DEFAULT`: | Type | Accepts | |---|---| | `BIGINT`, `VARCHAR`, `TIMESTAMP`, ... | Any SQL expression of a compatible type, evaluated per row: a column, arithmetic, a function call, a literal or a constant. | +| `ANY` | Any scalar SQL expression, whatever its type: a column, a literal, an expression or a scalar constant. Its type is not checked. Lists and relations fail with `P017`. | | ` CONST` | Only a value known at compile time: a literal, a scalar constant, an enum value, or a `CONST` parameter of a calling macro. A column or expression fails with `P017`. Scalar types only. | | `LIST `, `SET ` | An inline list or set, or a list or set constant. Always compile-time, so they take no `CONST`. They render like constants. | | `ENUM ` | A member of that enum, such as `@enum("order_status").OPEN`. | @@ -149,6 +150,13 @@ SQL types are written as in [schema declarations](/docs/concepts/models/schemas) adapter's SQL dialect the same way, including the dialect's array types such as `VARCHAR[]`. A runtime array value uses the array type; `LIST VARCHAR` is a list known at compile time. +Use `ANY` when the body works for values of any type, typically because it casts the argument +itself, as in `CAST(@order_ref AS VARCHAR)`, so callers can pass a number or a string. The body is +still checked, and the argument's type counts as unknown there. `ANY` is not `VARIANT`: a +`VARIANT` parameter says the macro takes semi-structured data, while `ANY` says it takes a plain +value of any type. `ANY` takes a `DEFAULT` but not `CONST`, which needs a type to check values +against, and it cannot be a `LIST` or `SET` element type. + Leave `CONST` off unless the body needs a literal. `CONST` is part of the macro's contract: callers can no longer pass a column. From 697ce4aa5e2063e30c54ee75d0ecccd5b0f918df Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 23 Oct 2026 15:34:05 +0100 Subject: [PATCH 02/11] feat: count a macro folder's python and sql kind folders separately in declaration layout rules --- .../src/engine/tests/helpers.rs | 4 + .../src/engine/tests/test_domain_layout.rs | 73 +++++++++++++++++- .../src/rules/_helpers/domain_layout.rs | 77 +++++++++++++++---- .../main/compile/sql_macros/helpers.py | 47 +++++++++++ .../sql_macros/test_sql_macro_layout_rules.py | 77 +++++++++++++++++++ .../docs/docs/concepts/macros/sql-macros.mdx | 5 ++ 6 files changed, 268 insertions(+), 15 deletions(-) create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_layout_rules.py diff --git a/crates/sqlbuild-rules/src/engine/tests/helpers.rs b/crates/sqlbuild-rules/src/engine/tests/helpers.rs index 4bba084546..43e315b7de 100644 --- a/crates/sqlbuild-rules/src/engine/tests/helpers.rs +++ b/crates/sqlbuild-rules/src/engine/tests/helpers.rs @@ -77,6 +77,10 @@ pub(crate) fn scope_with_macros(paths: &[String]) -> Value { .enumerate() .map(|(index, path)| { let relative = path.trim_start_matches("models/commerce/_macros/"); + let relative = relative + .strip_prefix("python/") + .or_else(|| relative.strip_prefix("sql/")) + .unwrap_or(relative); let bucket_path = std::path::Path::new(relative) .parent() .and_then(std::path::Path::to_str) diff --git a/crates/sqlbuild-rules/src/engine/tests/test_domain_layout.rs b/crates/sqlbuild-rules/src/engine/tests/test_domain_layout.rs index 228a398f4f..3a4979e8d3 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_domain_layout.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_domain_layout.rs @@ -12,7 +12,72 @@ fn given_owner_and_declaration_layouts_when_evaluating_then_returns_expected_fau let repeated_paths: Vec = (0..11) .map(|_| "models/commerce/_macros/orders.py".to_owned()) .collect(); + let python_and_sql: Vec = (0..6) + .map(|index| format!("models/commerce/_macros/python/normalise/item_{index}.py")) + .chain( + (0..7).map(|index| format!("models/commerce/_macros/sql/normalise/item_{index}.sql")), + ) + .collect(); + let oversized_sql: Vec = (0..11) + .map(|index| format!("models/commerce/_macros/sql/normalise/item_{index}.sql")) + .collect(); let test_cases = [ + test_types::DomainLayoutTestCase { + description: "python and sql macro kind folders are counted apart", + code: "SQBRDECLARATION304", + models: json!([]), + thresholds: json!({}), + layout: json!({}), + scope_index: helpers::scope_with_macros(&python_and_sql), + expected_codes: &[], + expected_message_fragments: &[], + expected_absent_fragments: &[], + }, + test_types::DomainLayoutTestCase { + description: "an oversized sql macro kind folder names its real folder", + code: "SQBRDECLARATION304", + models: json!([]), + thresholds: json!({}), + layout: json!({}), + scope_index: helpers::scope_with_macros(&oversized_sql), + expected_codes: &["SQBRDECLARATION304"], + expected_message_fragments: &[ + "declaration container \"models/commerce/_macros/sql/normalise\" has 11 files", + ], + expected_absent_fragments: &[], + }, + test_types::DomainLayoutTestCase { + description: "a shared prefix across kind folders is no fault", + code: "SQBRDECLARATION306", + models: json!([]), + thresholds: json!({}), + layout: json!({}), + scope_index: helpers::scope_with_macros(&[ + "models/commerce/_macros/python/normalise_order.py".into(), + "models/commerce/_macros/sql/normalise_person.sql".into(), + ]), + expected_codes: &[], + expected_message_fragments: &[], + expected_absent_fragments: &[], + }, + test_types::DomainLayoutTestCase { + description: "sql macro files sharing a prefix move into a subfolder of sql", + code: "SQBRDECLARATION306", + models: json!([]), + thresholds: json!({}), + layout: json!({}), + scope_index: helpers::scope_with_macros(&[ + "models/commerce/_macros/sql/normalise_order.sql".into(), + "models/commerce/_macros/sql/normalise_person.sql".into(), + ]), + expected_codes: &["SQBRDECLARATION306"], + expected_message_fragments: &[ + "2 sibling SQL macro files", + "Move these files into models/commerce/_macros/sql/normalise/", + "each macro keeps its name", + ], + expected_absent_fragments: &[], + }, test_types::DomainLayoutTestCase { description: "leaf and branch models fault", code: "SQBRPROJECT202", @@ -226,7 +291,13 @@ fn given_owner_and_declaration_layouts_when_evaluating_then_returns_expected_fau .collect(); let messages = faults .iter() - .filter_map(|fault| fault["message"].as_str()) + .map(|fault| { + format!( + "{} {}", + fault["message"].as_str().unwrap_or_default(), + fault["remediation"].as_str().unwrap_or_default() + ) + }) .collect::>() .join("\n"); diff --git a/crates/sqlbuild-rules/src/rules/_helpers/domain_layout.rs b/crates/sqlbuild-rules/src/rules/_helpers/domain_layout.rs index 4b24a49267..a39aa84b97 100644 --- a/crates/sqlbuild-rules/src/rules/_helpers/domain_layout.rs +++ b/crates/sqlbuild-rules/src/rules/_helpers/domain_layout.rs @@ -10,6 +10,10 @@ const DEFAULT_MAX_ROLE_CONTAINER_DEPTH: u32 = 1; const DEFAULT_MAX_CONTAINER_FILES: u32 = 10; const MIN_MODEL_PATH_COMPONENTS: usize = 3; const MIN_BRANCH_OUTCOMES: usize = 2; +/// The kind folders a macro folder holds, each counted as its own container. +const MACRO_KIND_FOLDERS: [&str; 2] = ["python", "sql"]; +/// The kind folder of SQL macro files, whose names are their file stems. +const SQL_MACRO_KIND_FOLDER: &str = "sql"; const GENERIC_BUCKET_NAMES: [&str; 8] = [ "common", "helper", "helpers", "misc", "shared", "util", "utils", "macros", ]; @@ -379,8 +383,8 @@ fn inspect_owner( faults.extend(prefix_faults( rule, &siblings, - inspection.min_prefixes, - "owner directories", + (inspection.min_prefixes, "owner directories"), + shared_prefix_remedy, )); } for (name, child) in &node.children { @@ -412,7 +416,12 @@ fn inspect_prefix_owners( } faults.extend(inspect_prefix_owners(rule, child, minimum, label)); } - faults.extend(prefix_faults(rule, &siblings, minimum, label)); + faults.extend(prefix_faults( + rule, + &siblings, + (minimum, label), + shared_prefix_remedy, + )); faults } @@ -466,6 +475,16 @@ fn container_entry(declaration: &ScopeDeclaration) -> Option<(String, ContainerE let stem = file .rsplit_once('.') .map_or_else(|| file.to_owned(), |(value, _)| value.into()); + let kind_folder: Option<&str> = matches!(declaration.kind, DeclarationKind::Macro) + .then_some(()) + .and(declaration.path.strip_prefix(&format!("{root}/"))) + .and_then(|rest| rest.split_once('/')) + .map(|(folder, _)| folder) + .filter(|folder| MACRO_KIND_FOLDERS.contains(folder)); + let root: String = match kind_folder { + Some(folder) => format!("{root}/{folder}"), + None => root, + }; Some(( root, ContainerEntry { @@ -597,12 +616,39 @@ fn inspect_container( .or_default() .push((entry.stem.clone(), entry.path.clone())); } - for siblings in locations.values() { + let sql_macros: bool = inspection + .root + .rsplit('/') + .next() + .is_some_and(|folder| folder == SQL_MACRO_KIND_FOLDER) + && files + .iter() + .all(|entry| matches!(entry.kind, DeclarationKind::Macro)); + for (location, siblings) in &locations { + let folder: String = if location.is_empty() { + inspection.root.to_owned() + } else { + format!("{}/{location}", inspection.root) + }; + let label: &str = if sql_macros { + "SQL macro files" + } else { + "declaration files" + }; + let remedy = |prefix: &str| { + if sql_macros { + format!( + "Move these files into {folder}/{prefix}/; a SQL macro is named by its file, so each macro keeps its name and its callers stay unchanged." + ) + } else { + shared_prefix_remedy(prefix) + } + }; faults.extend(prefix_faults( rule, siblings, - inspection.min_prefixes, - "declaration files", + (inspection.min_prefixes, label), + remedy, )); } } @@ -623,11 +669,18 @@ fn container_limit(evaluation: &ProjectEvaluationRequest<'_>, kind: &Declaration threshold(evaluation, name, DEFAULT_MAX_CONTAINER_FILES) } +/// The remedy for siblings that share a token prefix. +fn shared_prefix_remedy(prefix: &str) -> String { + format!( + "Consolidate these siblings beneath {prefix}/, or rename them when the shared token prefix is not a real concern." + ) +} + fn prefix_faults( rule: &RuleMetadata, siblings: &[(String, String)], - minimum: u32, - label: &str, + (minimum, label): (u32, &str), + remedy: impl Fn(&str) -> String, ) -> Vec { let mut root = TokenNode::default(); for (name, path) in siblings { @@ -644,12 +697,8 @@ fn prefix_faults( path_fault( rule, &path, - format!( - "{count} sibling {label} encode the implicit owner or bucket {prefix:?}" - ), - format!( - "Consolidate these siblings beneath {prefix}/, or rename them when the shared token prefix is not a real concern." - ), + format!("{count} sibling {label} encode the implicit owner or bucket {prefix:?}"), + remedy(&prefix), ) }) .collect() diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/helpers.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/helpers.py index 19e9278d05..01e92ccaca 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/helpers.py @@ -205,3 +205,50 @@ def check_macro(arguments: str, body: str) -> dict[str, str]: f" returns BOOLEAN,\n);\n\n{body}\n" ) } + + +def python_normalise_macros(names: list[str]) -> dict[str, str]: + """One Python macro file per name in the `normalise` bucket of `macros/python/`.""" + + return { + f"macros/python/normalise/{name}.py": ( + f"def {name}(expression: str) -> str:\n" + f' """Clean a value with {name}."""\n' + ' return f"TRIM({expression})"\n' + ) + for name in names + } + + +def sql_text_macros(folder: str, names: list[str]) -> dict[str, str]: + """One lower-casing SQL macro file per name in `macros/sql//`.""" + + return { + f"macros/sql/{folder}/{name}.sql": ( + f'MACRO (description "Tidy {name}", arguments (value VARCHAR), returns VARCHAR);\n\n' + "LOWER(@value)\n" + ) + for name in names + } + + +def text_macro(name: str, returns: str, body: str) -> dict[str, str]: + """The SQL macro `name` taking one VARCHAR value and declaring `returns`.""" + + return { + f"macros/sql/{name}.sql": ( + f'MACRO (\n description "Text from a value",\n arguments (value VARCHAR),\n' + f" returns {returns},\n);\n\n{body}\n" + ) + } + + +def contract_model(name: str, columns: str, sql: str) -> dict[str, str]: + """The mart model `name` with an enforced contract over `columns`.""" + + return { + f"models/marts/{name}.sql": ( + f'MODEL (\n description "Typed {name}",\n contract enforced,\n' + f" columns ({columns}),\n);\n\n{sql}" + ) + } diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_layout_rules.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_layout_rules.py new file mode 100644 index 0000000000..0a978ee07d --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_layout_rules.py @@ -0,0 +1,77 @@ +"""E2E coverage for declaration layout Rules over a macro folder's python/ and sql/ kind folders.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.compile.sql_macros._test_types import ( + SqlMacroCommandTestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.main.compile.sql_macros.helpers import ( + output, + python_normalise_macros, + run_project, + sql_text_macros, + with_rules, +) + +_LAYOUT_RULES: dict[str, str] = with_rules("SQBRDECLARATION304", "SQBRDECLARATION306") + + +@pytest.mark.parametrize( + "test_case", + [ + SqlMacroCommandTestCase( + description="six_python_and_seven_sql_files_fit_their_own_folders", + files={ + **_LAYOUT_RULES, + **python_normalise_macros(["trim", "upper", "pad", "strip", "fold", "squash"]), + **sql_text_macros( + "normalise", ["lower", "title", "slug", "ascii", "spaces", "digits", "letters"] + ), + }, + command=("compile", "--no-cache"), + expected_returncode=0, + expected_fragments=("0 errors",), + ), + SqlMacroCommandTestCase( + description="eleven_sql_files_overflow_their_real_folder", + files={ + **_LAYOUT_RULES, + **sql_text_macros("normalise", [f"tidy_{word}" for word in "abcdefghijk"]), + }, + command=("compile", "--no-cache"), + expected_returncode=1, + expected_fragments=( + "error[SQBRDECLARATION304]", + 'declaration container "macros/sql/normalise" has 11 files', + ), + ), + SqlMacroCommandTestCase( + description="sql_files_sharing_a_prefix_move_into_a_subfolder_of_sql", + files={ + **_LAYOUT_RULES, + **sql_text_macros("text", ["name_first", "name_last"]), + }, + command=("compile", "--no-cache"), + expected_returncode=1, + expected_fragments=( + "error[SQBRDECLARATION306]", + "2 sibling SQL macro files", + "Move these files into macros/sql/text/name/", + "each macro keeps its name", + ), + ), + ], + ids=lambda case: case.description, +) +def test_given_macro_kind_folders_when_checking_layout_rules_then_each_folder_counts_alone( + test_case: SqlMacroCommandTestCase, tmp_path: Path +) -> None: + _, result = run_project(tmp_path=tmp_path, files=test_case.files, command=test_case.command) + + assert result.returncode == test_case.expected_returncode, output(result) + for fragment in test_case.expected_fragments: + assert fragment in output(result) diff --git a/website/src/content/docs/docs/concepts/macros/sql-macros.mdx b/website/src/content/docs/docs/concepts/macros/sql-macros.mdx index f6ef59039f..3db9724684 100644 --- a/website/src/content/docs/docs/concepts/macros/sql-macros.mdx +++ b/website/src/content/docs/docs/concepts/macros/sql-macros.mdx @@ -37,6 +37,11 @@ my_project/ A file directly under a `macros/` folder fails with `D013`, which names the subfolder to move it to. Folder scopes use the same layout, such as `models/marts/_sqlbuild/_macros/sql/`. +Rules that bound declaration folders, such as `SQBRDECLARATION304` (files per folder) and +`SQBRDECLARATION306` (shared filename prefixes), count `python/` and `sql/` as separate folders. To +split a full `sql/` folder, move files into subfolders such as `macros/sql/text/`: a SQL macro is +named by its file, so it keeps its name and its callers stay unchanged. + A SQL macro file starts with a `MACRO` header, then the body: ```sql From e4e116e2aa0a7556699a21ce306c70ddf92a347e Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 23 Oct 2026 15:42:58 +0100 Subject: [PATCH 03/11] feat: use a SQL macro's declared returns type as its call's type in contract checks --- .../_helpers/analysis_cache.rs | 15 ++ .../analysis_session/_helpers/cte_facts.rs | 75 +++++++- .../analysis_session/_helpers/enrichment.rs | 2 + .../_helpers/query_columns.rs | 1 + .../analysis_session/_helpers/session.rs | 72 +++++++- .../main/expression_shapes.rs | 1 + .../main/recover_cte_facts.rs | 1 + .../analysis_session/tests/helpers.rs | 2 + .../src/sql_macros/_helpers/expansion.rs | 26 ++- .../src/sql_macros/_helpers/header.rs | 16 +- .../sql_macros/_helpers/literal_positions.rs | 4 +- .../src/sql_macros/_helpers/placement.rs | 44 ++++- .../src/sql_macros/constants.rs | 2 + .../src/sql_macros/main/mod.rs | 3 + .../main/sql_macro_declared_outputs.rs | 23 +++ .../main/sql_macro_declared_type.rs | 27 +++ .../sql_macros/main/sql_macro_output_names.rs | 42 +++++ .../src/sql_macros/models.rs | 21 +++ .../sqlbuild-analysis/src/sql_macros/tests.rs | 2 + .../src/sql_macros/tests/helpers.rs | 19 +- .../src/sql_macros/tests/test_output_types.rs | 103 +++++++++++ .../src/sql_macros/tests/test_types.rs | 16 ++ .../rules/_helpers/explicit_output_types.rs | 37 +++- .../docs/concepts/macros/sql-macros.md | 23 +++ .../sql_macros/test_sql_macro_types.py | 162 +++++++++++++++++- .../docs/docs/concepts/macros/sql-macros.mdx | 10 ++ 26 files changed, 726 insertions(+), 23 deletions(-) create mode 100644 crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_outputs.rs create mode 100644 crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs create mode 100644 crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_output_names.rs create mode 100644 crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/analysis_cache.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/analysis_cache.rs index 757a894a90..8cb400c6a8 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/analysis_cache.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/analysis_cache.rs @@ -1,5 +1,7 @@ //! Per-model analysis cache keys over everything an analysis reads, and stored outcomes. +use crate::sql_macros::main::sql_macro_declared_outputs::sql_macro_declared_outputs; +use crate::sql_macros::models::DeclaredOutput; use std::collections::{BTreeSet, HashMap}; use sha2::{Digest, Sha256}; @@ -169,6 +171,19 @@ impl KeyHasher { } self.text(&model.pivot_sql); self.families(&model.dynamic_families); + let declared: Vec = sql_macro_declared_outputs(&model.sql_macro_checks); + self.count(declared.len()); + for output in &declared { + match &output.cte { + Some(cte) => { + self.flag(true); + self.text(cte); + } + None => self.flag(false), + } + self.text(&output.name); + self.text(&output.returns); + } } fn families(&mut self, families: &[DynamicFamily]) { diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/cte_facts.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/cte_facts.rs index 3fff43f207..c35adf3b6d 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/cte_facts.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/cte_facts.rs @@ -35,6 +35,8 @@ use crate::assembly::analysis_session::types::{Pairs, Shapes}; use crate::nullability_rules::main::conditional_result_nullability::conditional_result_nullability; use crate::nullability_rules::main::first_arg_nullability::first_arg_nullability; use crate::nullability_rules::main::safe_cast_nullability::safe_cast_nullability; +use crate::sql_macros::main::sql_macro_declared_type::sql_macro_declared_type; +use crate::sql_macros::models::DeclaredOutput; use crate::type_system::main::normalize_type::normalize_type; use crate::type_system::models::{NormalizedType, TypeFamily}; @@ -58,6 +60,8 @@ pub(crate) struct RecoveryProfile<'a> { pub(crate) rules: Option<&'a Pairs>, /// Runs the adapter's own rules, those with the `python` rule id. pub(crate) callback: Option<&'a NullabilityCallback>, + /// The model's select items that are one SQL macro call each, typed by its `returns`. + pub(crate) declared: &'a [DeclaredOutput], } /// One enrichment's recovery request. @@ -129,6 +133,25 @@ pub(crate) struct LegacyAnalysis { pub(crate) lineage: Vec, } +/// The model query's output types its SQL macro call items change, given the relation types. +pub(crate) fn declared_output_types( + cleaned_sql: &str, + types: &ShapeTable, + profile: &RecoveryProfile<'_>, +) -> Fact> { + let Some(parsed) = parse_one(cleaned_sql, profile.dialect)? else { + return Ok(Vec::new()); + }; + let facts: Relations = referenced_types(&parsed, types); + let context = Context { profile }; + let declared: Vec<(String, String)> = context.model_passthrough_types(&parsed, &facts)?; + let plain: Vec<(String, String)> = context.passthrough_types(&parsed, &facts)?; + Ok(declared + .into_iter() + .filter(|(name, data_type)| dict_get(&plain, name) != Some(data_type)) + .collect()) +} + /// Python's legacy analysis of a model the native engine handed back, or why it defers. pub(crate) fn legacy_analysis(input: &LegacyInput<'_>) -> Fact { let Some(parsed) = parse_one(input.cleaned_sql, input.profile.dialect)? else { @@ -472,11 +495,32 @@ struct Context<'a, 'b> { } impl Context<'_, '_> { + /// The model query's pass-through types, its SQL macro call items typed by `returns`. + fn model_passthrough_types( + &self, + root: &Expression, + facts: &Relations, + ) -> Fact> { + let types: Vec<(String, String)> = + self.passthrough_types_in(root, facts, Some(self.profile.declared))?; + Ok(self.declared_types(None, types)) + } + /// Python's `_polyglot_cte_passthrough_types_from_parsed`. fn passthrough_types( &self, root: &Expression, facts: &Relations, + ) -> Fact> { + self.passthrough_types_in(root, facts, None) + } + + /// The pass-through types of `root`, typing the declared items of its own CTEs. + fn passthrough_types_in( + &self, + root: &Expression, + facts: &Relations, + declared: Option<&[DeclaredOutput]>, ) -> Fact> { let ctes: Vec<(&str, bool, &Expression)> = top_level_ctes(root); if ctes.is_empty() { @@ -487,8 +531,11 @@ impl Context<'_, '_> { if has_column_aliases { continue; } - let inferred: Vec<(String, String)> = + let mut inferred: Vec<(String, String)> = self.select_output_types(Some(body), &relations)?; + if declared.is_some() { + inferred = self.declared_types(Some(cte_name), inferred); + } if !inferred.is_empty() { relations = dict_set(relations, cte_name, Rc::new(inferred)); } @@ -499,6 +546,32 @@ impl Context<'_, '_> { self.direct_select_types(Some(root), &relations) } + /// `types` of the main query or CTE `cte` with each declared SQL macro call item typed. + fn declared_types( + &self, + cte: Option<&str>, + mut types: Vec<(String, String)>, + ) -> Vec<(String, String)> { + for output in self.profile.declared { + let key: String = types + .iter() + .map(|(name, _)| name) + .find(|name| name.eq_ignore_ascii_case(&output.name)) + .cloned() + .unwrap_or_else(|| output.name.clone()); + let inferred: Option<&str> = dict_get(&types, &key).map(String::as_str); + if let Some(declared) = sql_macro_declared_type( + self.profile.declared, + (cte, &output.name), + inferred, + self.profile.dialect, + ) { + types = dict_set(types, &key, declared); + } + } + types + } + /// Python's `_polyglot_select_output_types`. fn select_output_types( &self, diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/enrichment.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/enrichment.rs index 2d69ab47a5..cefd99d2b9 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/enrichment.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/enrichment.rs @@ -113,6 +113,7 @@ fn legacy(input: &EnrichmentInput<'_>, cleaned: &str) -> Result = input diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/query_columns.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/query_columns.rs index 7a20a27572..f360b6b503 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/query_columns.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/query_columns.rs @@ -182,6 +182,7 @@ impl Tables<'_> { function_return_types: &self.request.function_return_types, rules: self.request.nullability_rules.as_ref(), callback: self.request.nullability_callback.as_ref(), + declared: &[], }, }) })?; diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs index 35bd85b0f4..077fb25565 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs @@ -19,7 +19,7 @@ use crate::assembly::analysis_session::_helpers::compact_batch::{ Batch, BatchMember, MemberAnalysis, MemberResult, }; use crate::assembly::analysis_session::_helpers::cte_facts::{ - LegacyAnalysis, LegacyInput, RecoveryProfile, legacy_analysis, + LegacyAnalysis, LegacyInput, RecoveryProfile, declared_output_types, legacy_analysis, }; use crate::assembly::analysis_session::_helpers::dynamic_pivot::{PivotFacts, pivot_outcome}; use crate::assembly::analysis_session::_helpers::enrichment::{ @@ -40,7 +40,9 @@ use crate::assembly::analysis_session::types::{Pairs, Shapes}; use crate::semantic_validation::models::DiagnosticRow; use crate::semantic_validation::models::ProjectCatalog; use crate::sql_macros::main::check_sql_macro_arguments::check_sql_macro_arguments; -use crate::sql_macros::models::SqlMacroFinding; +use crate::sql_macros::main::sql_macro_declared_outputs::sql_macro_declared_outputs; +use crate::sql_macros::main::sql_macro_declared_type::sql_macro_declared_type; +use crate::sql_macros::models::{DeclaredOutput, SqlMacroCheck, SqlMacroFinding}; use crate::sql_macros::types::ArgumentCheckInput; /// What an internal native failure of the session names. @@ -294,6 +296,63 @@ impl AnalysisSession { self.check_sql_macros(models) } + /// Type the outputs SQL macro call items reach by the macros' `returns`, through CTEs. + fn type_sql_macro_outputs(&mut self, models: &[usize]) -> Result<(), String> { + for model in models { + let declared: Vec = + sql_macro_declared_outputs(&self.request.models[*model].sql_macro_checks); + if declared.is_empty() { + continue; + } + self.ensure_cleaned(&[*model])?; + let Some(outcome) = self.outcomes[*model].as_ref() else { + continue; + }; + let Ok(cleaned) = resolved_cleaned_sql(outcome) else { + continue; + }; + let profile = RecoveryProfile { + dialect: &self.request.dialect, + function_return_types: &self.request.function_return_types, + rules: self.request.nullability_rules.as_ref(), + callback: self.request.nullability_callback.as_ref(), + declared: &declared, + }; + let typed: Vec<(String, String)> = catch_compiler_panic(|| { + declared_output_types(&cleaned, &self.available_types, &profile) + }) + .map_err(|reason| { + native_failure(&model_context(&self.request.models[*model]), &reason) + })?; + let dialect: String = self.request.dialect.clone(); + let Some(columns) = self.outcome_mut(*model)?.analysis.columns.as_mut() else { + continue; + }; + for column in columns { + let Some((name, returns)) = typed + .iter() + .find(|(name, _)| name.eq_ignore_ascii_case(&column.name)) + else { + continue; + }; + let reached: [DeclaredOutput; 1] = [DeclaredOutput { + cte: None, + name: name.clone(), + returns: returns.clone(), + }]; + if let Some(data_type) = sql_macro_declared_type( + &reached, + (None, &column.name), + column.data_type.as_deref(), + &dialect, + ) { + column.data_type = Some(data_type); + } + } + } + Ok(()) + } + /// Each model's SQL macro call arguments checked against its analysed types, cache hits too. fn check_sql_macros(&mut self, models: &[usize]) -> Result<(), String> { let checked: Vec = models @@ -301,7 +360,7 @@ impl AnalysisSession { .copied() .filter(|model| { let request: &ModelRequest = &self.request.models[*model]; - !request.sql_macro_checks.is_empty() && request.placeholders.is_empty() + checks_arguments(&request.sql_macro_checks) && request.placeholders.is_empty() }) .collect(); self.ensure_cleaned(&checked)?; @@ -355,6 +414,7 @@ impl AnalysisSession { function_return_types: &self.request.function_return_types, rules: self.request.nullability_rules.as_ref(), callback: self.request.nullability_callback.as_ref(), + declared: &[], }, }) }) @@ -476,6 +536,7 @@ impl AnalysisSession { /// Python's deferred binding validation, then the dataflow's shape publication. fn finish_wave(&mut self, wave: usize) -> Result<(), String> { let models: Vec = self.waves[wave].clone(); + self.type_sql_macro_outputs(&models)?; let validated: &[usize] = if self.cyclic { &[] } else { &models }; let mut revalidated: Vec<(usize, Shapes)> = Vec::new(); for model in validated { @@ -721,6 +782,11 @@ impl AnalysisSession { } } +/// Whether any of the checks has arguments the analysed SQL must show fit. +fn checks_arguments(checks: &[SqlMacroCheck]) -> bool { + checks.iter().any(|check| !check.arguments.is_empty()) +} + fn batch_member<'a>(model: &'a ModelRequest, schema: &'a Shapes) -> BatchMember<'a> { BatchMember { query_sql: &model.query_sql, diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/main/expression_shapes.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/main/expression_shapes.rs index b65c4a0ad7..27a9755d87 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/main/expression_shapes.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/main/expression_shapes.rs @@ -66,6 +66,7 @@ pub fn expression_shapes( function_return_types: &request.function_return_types, rules: request.nullability_rules.as_ref(), callback: request.nullability_callback.as_ref(), + declared: &[], }, }) })?; diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/main/recover_cte_facts.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/main/recover_cte_facts.rs index 92b9f05478..acd8cc734a 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/main/recover_cte_facts.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/main/recover_cte_facts.rs @@ -29,6 +29,7 @@ fn recovered_facts(request: &CteFactRequest) -> Result { function_return_types: &request.function_return_types, rules: request.nullability_rules.as_ref(), callback: None, + declared: &[], }, })?; let mut direct_outputs: Vec = recovered.direct_outputs.into_iter().collect(); diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/helpers.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/helpers.rs index 459254d0f7..77d31e4b7e 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/helpers.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/helpers.rs @@ -471,6 +471,7 @@ pub(crate) fn recovered_facts(sql: &str) -> Option<(Pairs, Pairs, Vec, V function_return_types: &Vec::new(), rules: Some(&rules), callback: None, + declared: &[], }, }) .ok()?; @@ -550,6 +551,7 @@ pub(crate) fn legacy_lines(sql: &str) -> Option> { function_return_types: &Vec::new(), rules: Some(&rules), callback: None, + declared: &[], }, }) .ok()?; diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/expansion.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/expansion.rs index 5b59135c67..d69d162310 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/expansion.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/expansion.rs @@ -24,7 +24,7 @@ use crate::sql_macros::errors::SqlMacroError; use crate::sql_macros::models::{ ArgumentCheck, ArgumentExpectation, ArgumentShape, BodyExpansion, BoundArgument, CallPosition, CallScope, InsertedNames, ParameterKind, RenderedArgument, SqlMacroCall, SqlMacroCheck, - SqlMacroDefinition, SqlMacroForm, SqlMacroLocation, TextContext, + SqlMacroDefinition, SqlMacroForm, SqlMacroLocation, TextContext, TypedOutput, }; use crate::sql_macros::types::{ConstantShape, SqlMacroHost, SqlMacroScope}; @@ -214,24 +214,34 @@ pub(crate) fn expand_call( inserted, )); } + let site: CallSite<'_> = CallSite::new( + context.text, + scanned, + (call.start, call.end), + call.replaced_end, + ); if matches!(context.scope, SqlMacroScope::Caller) && context.stack.is_empty() { + let output: Option = match (definition.form, &definition.returns) { + (SqlMacroForm::Expression, Some(returns)) => { + site.whole_item_alias().map(|name| TypedOutput { + name, + returns: returns.clone(), + }) + } + _ => None, + }; let check: SqlMacroCheck = SqlMacroCheck { macro_name: definition.name.clone(), scope: call_scope(context.text, scanned, call.start), plain_select: plain_select(context.text, scanned, call.start), arguments: argument_checks(&definition, &bound, &rendered), call: None, + output, }; - if !check.arguments.is_empty() { + if !check.arguments.is_empty() || check.output.is_some() { host.record_check(check, (call.start, call.end)); } } - let site: CallSite<'_> = CallSite::new( - context.text, - scanned, - (call.start, call.end), - call.replaced_end, - ); let line_continues: bool = context.text[call.replaced_end..] .split('\n') .next() diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/header.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/header.rs index 46e2d62be8..513b9e32d6 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/header.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/header.rs @@ -2,9 +2,10 @@ use crate::sql_macros::_helpers::lexing::{Token, TokenKind, closing_token, line_column, tokens}; use crate::sql_macros::constants::{ - ANY_KEYWORD, ARGUMENTS_KEY, COLUMN_KEYWORD, COMMA, CONST_KEYWORD, DEFAULT_KEYWORD, DESCRIPTION_KEY, - ENUM_KEYWORD, HEADER_KEYWORD, INVALID_DEFINITION_CODE, LIST_KEYWORD, OF_KEYWORD, - RAW_TYPE_NAMES, RELATION_COLUMN_MIN_TOKENS, RELATION_KEYWORD, RETURNS_KEY, SET_KEYWORD, + ANY_KEYWORD, ARGUMENTS_KEY, COLUMN_KEYWORD, COMMA, CONST_KEYWORD, DEFAULT_KEYWORD, + DESCRIPTION_KEY, ENUM_KEYWORD, HEADER_KEYWORD, INVALID_DEFINITION_CODE, LIST_KEYWORD, + OF_KEYWORD, RAW_TYPE_NAMES, RELATION_COLUMN_MIN_TOKENS, RELATION_KEYWORD, RETURNS_KEY, + SET_KEYWORD, }; use crate::sql_macros::errors::LexError; use crate::sql_macros::errors::SqlMacroError; @@ -278,8 +279,13 @@ impl HeaderParser<'_> { let element: &[Token] = &tokens[1..]; if element.len() == 1 && element[0].is_word(source, ANY_KEYWORD) { return Err(self - .error(element[0].start, format!("argument {name} {keyword} ANY: ANY is for scalar arguments only")) - .helped(format!("declare the element type, such as {name} {keyword} VARCHAR"))); + .error( + element[0].start, + format!("argument {name} {keyword} ANY: ANY is for scalar arguments only"), + ) + .helped(format!( + "declare the element type, such as {name} {keyword} VARCHAR" + ))); } if let Some((last, element)) = element.split_last() && last.is_word(source, CONST_KEYWORD) diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/literal_positions.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/literal_positions.rs index 71e8a26965..515eb037e5 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/literal_positions.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/literal_positions.rs @@ -232,7 +232,9 @@ pub(crate) fn literal_error( Some(ParameterKind::Expression { data_type }) => { format!("declare it {name} {data_type} CONST") } - Some(ParameterKind::Any) => format!("declare it with a type and CONST, such as {name} VARCHAR CONST"), + Some(ParameterKind::Any) => { + format!("declare it with a type and CONST, such as {name} VARCHAR CONST") + } _ => format!("pass the value through a CONST argument, such as {name}_value VARCHAR CONST"), }; SqlMacroError::new(LITERAL_REQUIRED_CODE, format!("{name} is used where a literal is required; {help}")) diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/placement.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/placement.rs index 209ddd5283..ac62490e92 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/placement.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/placement.rs @@ -2,7 +2,7 @@ use crate::sql_macros::_helpers::header::unquoted; use crate::sql_macros::_helpers::lexing::{Token, TokenKind, closing_token}; -use crate::sql_macros::constants::{AT_SIGN, COMMA, SELECT_KEYWORD, WITH_KEYWORD}; +use crate::sql_macros::constants::{AS_KEYWORD, AT_SIGN, COMMA, SELECT_KEYWORD, WITH_KEYWORD}; use crate::sql_macros::models::{CallPosition, CallScope}; /// Tokens before the `(` of `DISTINCT ON (...)`. @@ -252,6 +252,48 @@ impl<'a> CallSite<'a> { } } + /// The alias of the select-list item the call is the whole of, as `@sql.m(x) AS name`. + pub(crate) fn whole_item_alias(&self) -> Option { + let opens_item: bool = self.previous_token().is_some_and(|token| { + matches!( + token.text(self.sql).to_ascii_uppercase().as_str(), + "SELECT" | "DISTINCT" | "ALL" | "," + ) + }); + if !opens_item || self.position() != CallPosition::SelectList { + return None; + } + let next: usize = self.next?; + let alias_index: usize = if self.tokens[next].is_word(self.sql, AS_KEYWORD) { + next + 1 + } else { + next + }; + let alias: &Token = self.tokens.get(alias_index)?; + let alias_text: String = alias.text(self.sql).to_ascii_uppercase(); + let named: bool = match alias.kind { + TokenKind::Quoted => true, + TokenKind::Word => { + !SELECT_LIST_ENDS.contains(&alias_text.as_str()) + && !EXPRESSION_OPERATOR_WORDS.contains(&alias_text.as_str()) + && alias_text != AS_KEYWORD + } + _ => false, + }; + if !named { + return None; + } + let ends_item: bool = match self.tokens.get(alias_index + 1) { + None => true, + Some(after) => { + let text: String = after.text(self.sql).to_ascii_uppercase(); + matches!(text.as_str(), "," | ")" | ";") + || (after.kind == TokenKind::Word && SELECT_LIST_ENDS.contains(&text.as_str())) + } + }; + ends_item.then(|| unquoted(alias.text(self.sql))) + } + /// Whether the token after the call ends a select-list item. fn ends_select_item(&self) -> bool { let Some(next) = self.next_token() else { diff --git a/crates/sqlbuild-analysis/src/sql_macros/constants.rs b/crates/sqlbuild-analysis/src/sql_macros/constants.rs index 731dda31cc..646741e2ad 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/constants.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/constants.rs @@ -64,6 +64,8 @@ pub(crate) const CTE_NOUN: &str = "CTE"; /// Clause keywords a call position reads. pub(crate) const WITH_KEYWORD: &str = "WITH"; pub(crate) const SELECT_KEYWORD: &str = "SELECT"; +/// The keyword before an alias. +pub(crate) const AS_KEYWORD: &str = "AS"; /// Punctuation and operators the scans compare against. pub(crate) const COMMA: &str = ","; pub(crate) const AT_SIGN: &str = "@"; diff --git a/crates/sqlbuild-analysis/src/sql_macros/main/mod.rs b/crates/sqlbuild-analysis/src/sql_macros/main/mod.rs index 38f29462c4..ab74c41019 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/main/mod.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/main/mod.rs @@ -7,5 +7,8 @@ pub mod render_inline_collection; pub mod sql_macro_body_form; pub mod sql_macro_calls; pub mod sql_macro_cycle; +pub mod sql_macro_declared_outputs; +pub mod sql_macro_declared_type; +pub mod sql_macro_output_names; pub mod sql_macro_parameter_spans; pub mod with_list_sql_macro_calls; diff --git a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_outputs.rs b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_outputs.rs new file mode 100644 index 0000000000..d52b0b47a2 --- /dev/null +++ b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_outputs.rs @@ -0,0 +1,23 @@ +use crate::sql_macros::models::{CallScope, DeclaredOutput, SqlMacroCheck}; + +/// The select items of a model's main query and top-level CTEs that are one macro call each. +#[must_use] +pub fn sql_macro_declared_outputs(checks: &[SqlMacroCheck]) -> Vec { + checks + .iter() + .filter(|check| check.plain_select) + .filter_map(|check| { + let output = check.output.as_ref()?; + let cte: Option = match &check.scope { + CallScope::Main => None, + CallScope::Cte(name) => Some(name.clone()), + CallScope::WithList => return None, + }; + Some(DeclaredOutput { + cte, + name: output.name.clone(), + returns: output.returns.clone(), + }) + }) + .collect() +} diff --git a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs new file mode 100644 index 0000000000..a7170a4131 --- /dev/null +++ b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs @@ -0,0 +1,27 @@ +use crate::sql_macros::_helpers::arguments::type_family; +use crate::sql_macros::models::{DeclaredOutput, TypeFamily}; + +/// The declared `returns` type an item takes over its inferred type: unknown or same family. +#[must_use] +pub fn sql_macro_declared_type( + declared: &[DeclaredOutput], + (cte, name): (Option<&str>, &str), + inferred: Option<&str>, + dialect: &str, +) -> Option { + let output: &DeclaredOutput = declared.iter().find(|output| { + output.name.eq_ignore_ascii_case(name) + && match (&output.cte, cte) { + (None, None) => true, + (Some(declared_cte), Some(cte)) => declared_cte.eq_ignore_ascii_case(cte), + _ => false, + } + })?; + let Some(inferred) = inferred.filter(|inferred| !inferred.is_empty()) else { + return Some(output.returns.clone()); + }; + let inferred_family: TypeFamily = type_family(inferred, dialect); + let refines: bool = inferred_family == TypeFamily::Unknown + || inferred_family == type_family(&output.returns, dialect); + refines.then(|| output.returns.clone()) +} diff --git a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_output_names.rs b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_output_names.rs new file mode 100644 index 0000000000..b033cf0136 --- /dev/null +++ b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_output_names.rs @@ -0,0 +1,42 @@ +use crate::sql_macros::_helpers::expansion::sql_macro_calls; +use crate::sql_macros::_helpers::lexing::{Token, tokens}; +use crate::sql_macros::_helpers::placement::{CallSite, QueryParts, top_level_parts}; +use crate::sql_macros::models::{CallScope, SqlMacroCall}; + +/// The `(top-level CTE, alias)` of each select item of `text` that is one SQL macro call. +#[must_use] +pub fn sql_macro_output_names(text: &str) -> Vec<(Option, String)> { + let Ok(calls) = sql_macro_calls(text) else { + return Vec::new(); + }; + if calls.is_empty() { + return Vec::new(); + } + let Ok(scanned) = tokens(text) else { + return Vec::new(); + }; + let Some(parts) = top_level_parts(text, &scanned) else { + return Vec::new(); + }; + calls + .iter() + .filter(|call| !call.in_python && parts.plain_select(text, &scanned, call.start)) + .filter_map(|call| whole_item(text, (&scanned, &parts), call)) + .collect() +} + +/// The `(top-level CTE, alias)` of `call` when it is a whole aliased select item. +fn whole_item( + text: &str, + (scanned, parts): (&[Token], &QueryParts), + call: &SqlMacroCall, +) -> Option<(Option, String)> { + let cte: Option = match parts.scope_of(call.start) { + CallScope::Main => None, + CallScope::Cte(name) => Some(name), + CallScope::WithList => return None, + }; + CallSite::new(text, scanned, (call.start, call.end), call.replaced_end) + .whole_item_alias() + .map(|alias| (cte, alias)) +} diff --git a/crates/sqlbuild-analysis/src/sql_macros/models.rs b/crates/sqlbuild-analysis/src/sql_macros/models.rs index 5d93f65409..1cf1e7ba87 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/models.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/models.rs @@ -256,6 +256,27 @@ pub struct SqlMacroCheck { pub arguments: Vec, /// The call's authored file location and where its text ends, when the host knows them. pub call: Option<(SqlMacroLocation, (usize, usize))>, + /// The output column an expression call is the whole select-list item of, and its type. + pub output: Option, +} + +/// A select item that is one expression macro call, typed by the macro's `returns`. +#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +pub struct DeclaredOutput { + /// The top-level CTE the item is in, or `None` for the main query. + pub cte: Option, + /// The item's alias, unquoted. + pub name: String, + pub returns: String, +} + +/// A select-list item that is one expression macro call under an alias, as `@sql.m(x) AS name`. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct TypedOutput { + /// The alias, unquoted. + pub name: String, + /// The macro's declared `returns` type. + pub returns: String, } /// The top-level part of the caller's query a call stands in. diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests.rs b/crates/sqlbuild-analysis/src/sql_macros/tests.rs index cbdd71a321..ef65d03c09 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests.rs @@ -4,5 +4,7 @@ mod helpers; mod test_definitions; #[path = "tests/test_expansion.rs"] mod test_expansion; +#[path = "tests/test_output_types.rs"] +mod test_output_types; #[path = "tests/test_types.rs"] mod test_types; diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/helpers.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/helpers.rs index 8763b5a0ab..b4fea83df5 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests/helpers.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/helpers.rs @@ -4,7 +4,9 @@ use crate::sql_macros::_helpers::expansion::expand_text; use crate::sql_macros::errors::ExpansionFailure; use crate::sql_macros::errors::SqlMacroError; use crate::sql_macros::main::parse_sql_macro::parse_sql_macro; -use crate::sql_macros::models::{SqlMacroDefinition, TextContext}; +use crate::sql_macros::models::{ + CallScope, SqlMacroCheck, SqlMacroDefinition, TextContext, TypedOutput, +}; use crate::sql_macros::tests::test_types::{AsciiArguments, FakeHost}; use crate::sql_macros::types::{ConstantShape, SqlMacroHost, SqlMacroScope}; @@ -126,3 +128,18 @@ impl sqlbuild_render::macro_arguments::types::ArgumentHost for AsciiArguments { None } } + +/// A check recording output `label` of `shout()`, typed VARCHAR, at `scope`. +pub(super) fn typed_check(scope: CallScope, plain_select: bool) -> SqlMacroCheck { + SqlMacroCheck { + macro_name: "shout".to_owned(), + scope, + plain_select, + arguments: Vec::new(), + call: None, + output: Some(TypedOutput { + name: "label".to_owned(), + returns: "VARCHAR".to_owned(), + }), + } +} diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs new file mode 100644 index 0000000000..2ebf1eb935 --- /dev/null +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs @@ -0,0 +1,103 @@ +use crate::sql_macros::main::sql_macro_declared_outputs::sql_macro_declared_outputs; +use crate::sql_macros::main::sql_macro_declared_type::sql_macro_declared_type; +use crate::sql_macros::main::sql_macro_output_names::sql_macro_output_names; +use crate::sql_macros::models::CallScope; +use crate::sql_macros::tests::helpers::typed_check; +use crate::sql_macros::tests::test_types::{OutputNamesTestCase, OutputTypeTestCase}; + +#[test] +fn given_a_typed_macro_output_when_typing_the_column_then_returns_fills_unknown_and_refines() { + let test_cases = [ + OutputTypeTestCase { + description: "unknown inferred type takes returns", + check: typed_check(CallScope::Main, true), + item: (None, "label", None), + expected_type: Some("VARCHAR"), + }, + OutputTypeTestCase { + description: "same family takes returns", + check: typed_check(CallScope::Main, true), + item: (None, "LABEL", Some("TEXT")), + expected_type: Some("VARCHAR"), + }, + OutputTypeTestCase { + description: "a different known family keeps the inferred type", + check: typed_check(CallScope::Main, true), + item: (None, "label", Some("BIGINT")), + expected_type: None, + }, + OutputTypeTestCase { + description: "another column is untouched", + check: typed_check(CallScope::Main, true), + item: (None, "other", None), + expected_type: None, + }, + OutputTypeTestCase { + description: "a call in a CTE types that CTE's item", + check: typed_check(CallScope::Cte("base".to_owned()), true), + item: (Some("BASE"), "label", None), + expected_type: Some("VARCHAR"), + }, + OutputTypeTestCase { + description: "a call in a CTE leaves a main-query item of that name", + check: typed_check(CallScope::Cte("base".to_owned()), true), + item: (None, "label", None), + expected_type: None, + }, + OutputTypeTestCase { + description: "a call in a set operation is not typed", + check: typed_check(CallScope::Main, false), + item: (None, "label", None), + expected_type: None, + }, + ]; + + for test_case in test_cases { + let (cte, name, inferred) = test_case.item; + let declared = sql_macro_declared_outputs(&[test_case.check]); + let typed = sql_macro_declared_type(&declared, (cte, name), inferred, "duckdb"); + + assert_eq!( + typed.as_deref(), + test_case.expected_type, + "{}", + test_case.description + ); + } +} + +#[test] +fn given_model_sql_when_reading_sql_macro_outputs_then_only_whole_aliased_items_count() { + let test_cases = [ + OutputNamesTestCase { + description: "aliased whole items, with and without AS", + sql: "SELECT @sql.a(x) AS one, @sql.b(y) two, @sql.c(z) AS \"Three\" FROM t", + expected_names: &[(None, "one"), (None, "two"), (None, "Three")], + }, + OutputNamesTestCase { + description: "calls inside expressions or without an alias", + sql: "SELECT @sql.a(x) || 'x' AS one, UPPER(@sql.b(y)) AS two, @sql.c(z) FROM t", + expected_names: &[], + }, + OutputNamesTestCase { + description: "a call in a CTE, and one in a set operation", + sql: "WITH c AS (SELECT @sql.a(x) AS one FROM t) SELECT @sql.b(y) AS two FROM c UNION ALL SELECT 'x' FROM c", + expected_names: &[(Some("c"), "one")], + }, + OutputNamesTestCase { + description: "a call in the main query after CTEs", + sql: "WITH c AS (SELECT x FROM t) SELECT x, @sql.a(x) AS one FROM c", + expected_names: &[(None, "one")], + }, + ]; + + for test_case in test_cases { + let names: Vec<(Option, String)> = sql_macro_output_names(test_case.sql); + let found: Vec<(Option<&str>, &str)> = names + .iter() + .map(|(cte, name)| (cte.as_deref(), name.as_str())) + .collect(); + + assert_eq!(found, test_case.expected_names, "{}", test_case.description); + } +} diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/test_types.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/test_types.rs index 493f530145..9e484a48cb 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests/test_types.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/test_types.rs @@ -62,3 +62,19 @@ pub(super) struct PythonArgumentTestCase { /// Argument parsing without Unicode name lookups, which these arguments never use. pub(super) struct AsciiArguments; + +/// One recorded call, one analysed select item and the type the item then has, if changed. +pub(super) struct OutputTypeTestCase { + pub(super) description: &'static str, + pub(super) check: crate::sql_macros::models::SqlMacroCheck, + /// The item's CTE (`None` for the main query), name and inferred type. + pub(super) item: (Option<&'static str>, &'static str, Option<&'static str>), + pub(super) expected_type: Option<&'static str>, +} + +/// Model SQL and the scope and alias of each whole-item SQL macro call. +pub(super) struct OutputNamesTestCase { + pub(super) description: &'static str, + pub(super) sql: &'static str, + pub(super) expected_names: &'static [(Option<&'static str>, &'static str)], +} diff --git a/crates/sqlbuild-rules/src/rules/_helpers/explicit_output_types.rs b/crates/sqlbuild-rules/src/rules/_helpers/explicit_output_types.rs index 0002d70e6d..4dbc541cd7 100644 --- a/crates/sqlbuild-rules/src/rules/_helpers/explicit_output_types.rs +++ b/crates/sqlbuild-rules/src/rules/_helpers/explicit_output_types.rs @@ -1,3 +1,7 @@ +use std::cell::OnceCell; + +use sqlbuild_analysis::sql_macros::main::sql_macro_output_names::sql_macro_output_names; + use crate::constants::ENFORCED_CONTRACT; use crate::models::{Column, Fault, Model, RuleMetadata}; use crate::query_facts::models::{ItemFacts, QueryFacts, SelectFacts, SetFacts}; @@ -11,6 +15,8 @@ struct OutputScope<'a> { columns: &'a [Column], set_branch: bool, by_name: bool, + /// Where SQL macro call items are read: the main query (`Some(None)`), a CTE, or nowhere. + macro_scope: Option>, } struct OutputRule<'a> { @@ -18,6 +24,8 @@ struct OutputRule<'a> { model: &'a Model, rule: &'a RuleMetadata, faults: &'a FaultCollector, + /// The main query's outputs that are one SQL macro call each, read when first needed. + sql_macro_outputs: OnceCell, String)>>, } pub(crate) fn evaluate(parsed: &ParsedModel<'_>, rule: &RuleMetadata, faults: &FaultCollector) { @@ -44,14 +52,17 @@ pub(crate) fn evaluate(parsed: &ParsedModel<'_>, rule: &RuleMetadata, faults: &F model, rule, faults, + sql_macro_outputs: OnceCell::new(), }; if let Some((boundary, columns)) = final_cte_output_boundary(query, &model.columns) { + let final_cte: Option<&str> = query.ctes.last().map(|cte| cte.name.as_str()); evaluator.evaluate_set( &boundary.body, OutputScope { columns: &columns, set_branch: false, by_name: false, + macro_scope: final_cte.map(Some), }, ); return; @@ -63,6 +74,7 @@ pub(crate) fn evaluate(parsed: &ParsedModel<'_>, rule: &RuleMetadata, faults: &F columns: &columns, set_branch: false, by_name: false, + macro_scope: Some(None), }, ); } @@ -126,6 +138,7 @@ impl OutputRule<'_> { columns: scope.columns, set_branch: true, by_name: *by_name, + macro_scope: None, }; self.evaluate_set(left, branch_scope); self.evaluate_set(right, branch_scope); @@ -185,7 +198,11 @@ impl OutputRule<'_> { .cast_type .as_ref() .is_some_and(|data_type| type_spelling_equal(data_type, &column.data_type)); - let passes = cast_matches_contract || column.type_proven && direct && !scope.set_branch; + let passes = cast_matches_contract + || column.type_proven && direct && !scope.set_branch + || column.type_proven + && !explicit_cast + && self.sql_macro_output(scope.macro_scope, output_name.as_deref()); if passes { continue; } @@ -213,6 +230,24 @@ impl OutputRule<'_> { } } + /// Whether output `name` at `scope` is one SQL macro call, typed by its `returns`. + fn sql_macro_output(&self, scope: Option>, name: Option<&str>) -> bool { + let (Some(scope), Some(name)) = (scope, name) else { + return false; + }; + self.sql_macro_outputs + .get_or_init(|| sql_macro_output_names(&self.model.authored_sql)) + .iter() + .any(|(cte, output)| { + output.eq_ignore_ascii_case(name) + && match (cte.as_deref(), scope) { + (None, None) => true, + (Some(cte), Some(scope)) => cte.eq_ignore_ascii_case(scope), + _ => false, + } + }) + } + fn push(&self, offset: Option, message: String, remediation: String) { let (line, column) = offset.map_or((1, 1), |offset| self.parsed.source.line_column(offset)); self.faults.push(Fault { diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md index c0e2718f54..a064bf7832 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md @@ -49,6 +49,11 @@ my_project/ A file directly under a `macros/` folder fails with `D013`, which names the subfolder to move it to. Folder scopes use the same layout, such as `models/marts/_sqlbuild/_macros/sql/`. +Rules that bound declaration folders, such as `SQBRDECLARATION304` (files per folder) and +`SQBRDECLARATION306` (shared filename prefixes), count `python/` and `sql/` as separate folders. To +split a full `sql/` folder, move files into subfolders such as `macros/sql/text/`: a SQL macro is +named by its file, so it keeps its name and its callers stay unchanged. + A SQL macro file starts with a `MACRO` header, then the body: ```sql @@ -151,6 +156,7 @@ Every parameter has a type, and may have a `DEFAULT`: | Type | Accepts | |---|---| | `BIGINT`, `VARCHAR`, `TIMESTAMP`, ... | Any SQL expression of a compatible type, evaluated per row: a column, arithmetic, a function call, a literal or a constant. | +| `ANY` | Any scalar SQL expression, whatever its type: a column, a literal, an expression or a scalar constant. Its type is not checked. Lists and relations fail with `P017`. | | ` CONST` | Only a value known at compile time: a literal, a scalar constant, an enum value, or a `CONST` parameter of a calling macro. A column or expression fails with `P017`. Scalar types only. | | `LIST `, `SET ` | An inline list or set, or a list or set constant. Always compile-time, so they take no `CONST`. They render like constants. | | `ENUM ` | A member of that enum, such as `@enum("order_status").OPEN`. | @@ -161,6 +167,13 @@ SQL types are written as in [schema declarations](../models/schemas.md) and chec adapter's SQL dialect the same way, including the dialect's array types such as `VARCHAR[]`. A runtime array value uses the array type; `LIST VARCHAR` is a list known at compile time. +Use `ANY` when the body works for values of any type, typically because it casts the argument +itself, as in `CAST(@order_ref AS VARCHAR)`, so callers can pass a number or a string. The body is +still checked, and the argument's type counts as unknown there. `ANY` is not `VARIANT`: a +`VARIANT` parameter says the macro takes semi-structured data, while `ANY` says it takes a plain +value of any type. `ANY` takes a `DEFAULT` but not `CONST`, which needs a type to check values +against, and it cannot be a `LIST` or `SET` element type. + Leave `CONST` off unless the body needs a literal. `CONST` is part of the macro's contract: callers can no longer pass a column. @@ -264,6 +277,16 @@ parameters of a calling macro, and calls in models with runtime placeholders; th column checks still run on the expanded SQL. All other errors also point at the call and at the macro file. +A select-list item that is one expression macro call under an alias, as in +`@sql.cents_to_dollars(o.amount_cents) AS amount`, has the macro's `returns` type wherever the +compiler infers no type for the expanded SQL or a type of the same family, such as `DECIMAL` for +`DECIMAL(18, 2)`. This holds in the main query and in any top-level CTE, and the type flows on as +an inferred type would: through `SELECT * FROM final`, pass-through columns and joins, to contract +checks (`K002`, `K003`), `SQBRCONTRACT105` and models that read the column. Such a call needs no +`CAST` to satisfy a matching contract column. If the inferred type has another family, the inferred +type stands and `K002` reports the mismatch. A call inside a larger expression, a subquery or a +`UNION` branch is typed from the expanded SQL as before. + - `sqb format` formats SQL macro files, bodies of every form and the `MACRO` header, and normalises the commas around CTE-list and column-list calls. - [Rules](../rules.md) and [column lineage](../column-lineage.md) read the expanded diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py index 9a4e2311b4..75bd119bf0 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py @@ -1,22 +1,28 @@ -"""E2E coverage for ANY SQL macro arguments.""" +"""E2E coverage for ANY SQL macro arguments and the declared returns type of a call.""" from __future__ import annotations +import subprocess from pathlib import Path import pytest from tests.e2e.src.sqlbuild.cli.commands.main.compile.sql_macros._test_types import ( SqlMacroBuildTestCase, + SqlMacroCommandTestCase, + SqlMacroCompileTestCase, SqlMacroErrorTestCase, ) from tests.e2e.src.sqlbuild.cli.commands.main.compile.sql_macros.helpers import ( DATABASE, + contract_model, orders_model, output, run_project, + text_macro, + with_rules, ) -from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import query_duckdb +from tests.e2e.src.sqlbuild.cli.commands.shared.helpers import query_duckdb, run_sqb _ORDER_KEY: dict[str, str] = { "macros/sql/order_key.sql": ( @@ -27,6 +33,29 @@ ) } +_UNPROVEN: tuple[str, ...] = ("K003", "SQBRCONTRACT105") +_TYPED_COLUMNS: str = "order_id (type BIGINT), shouted (type VARCHAR), status_bar (type VARCHAR)" +_TYPED_RULES_AND_MACROS: dict[str, str] = { + **with_rules("SQBRCONTRACT105"), + **text_macro("shouted", "VARCHAR", "UPPER(@value)"), + **text_macro("status_bar", "VARCHAR", "bar(LENGTH(@value), 0, 10)"), +} +_TYPED_CALLS: dict[str, str] = { + **_TYPED_RULES_AND_MACROS, + **contract_model( + "orders", + _TYPED_COLUMNS, + "SELECT\n o.order_id,\n @sql.shouted(o.status) AS shouted,\n" + " @sql.status_bar(o.status) AS status_bar\n" + 'FROM __ref("stg_orders") AS o\n', + ), + **contract_model( + "order_bars", + "status_bar (type VARCHAR)", + 'SELECT b.status_bar FROM __ref("orders") AS b\n', + ), +} + @pytest.mark.parametrize( "test_case", @@ -106,3 +135,132 @@ def test_given_an_any_argument_misused_when_compiling_then_it_fails( assert f"error[{test_case.expected_code}]" in output(result) for fragment in test_case.expected_fragments: assert fragment in output(result) + + +@pytest.mark.parametrize( + "test_case", + [ + SqlMacroCompileTestCase( + description="a_main_query_call_and_a_downstream_passthrough", + files=_TYPED_CALLS, + expected_returncode=0, + expected_fragments=("0 errors, 0 warnings",), + unexpected_fragments=_UNPROVEN, + ), + SqlMacroCompileTestCase( + description="a_call_in_a_final_cte_read_with_a_star", + files={ + **_TYPED_RULES_AND_MACROS, + **contract_model( + "orders", + _TYPED_COLUMNS, + "WITH final AS (\n SELECT\n o.order_id,\n" + " @sql.shouted(o.status) AS shouted,\n" + " @sql.status_bar(o.status) AS status_bar\n" + ' FROM __ref("stg_orders") AS o\n)\nSELECT * FROM final\n', + ), + }, + expected_returncode=0, + expected_fragments=("0 errors, 0 warnings",), + unexpected_fragments=_UNPROVEN, + ), + SqlMacroCompileTestCase( + description="a_call_through_a_two_cte_chain_and_a_join", + files={ + **_TYPED_RULES_AND_MACROS, + **contract_model( + "orders", + _TYPED_COLUMNS, + "WITH typed AS (\n SELECT\n o.order_id,\n" + " @sql.shouted(o.status) AS shouted,\n" + " @sql.status_bar(o.status) AS status_bar\n" + ' FROM __ref("stg_orders") AS o\n),\n' + "final AS (\n SELECT t.order_id, t.shouted, b.status_bar\n" + " FROM typed AS t JOIN typed AS b ON b.order_id = t.order_id\n)\n" + "SELECT * FROM final\n", + ), + }, + expected_returncode=0, + expected_fragments=("0 errors, 0 warnings",), + unexpected_fragments=_UNPROVEN, + ), + SqlMacroCompileTestCase( + description="a_call_inside_a_larger_expression_is_typed_from_its_sql", + files={ + **_TYPED_RULES_AND_MACROS, + **contract_model( + "orders", + "order_id (type BIGINT), status_bar (type VARCHAR)", + "WITH final AS (\n SELECT\n o.order_id,\n" + " COALESCE(@sql.status_bar(o.status), @sql.status_bar(o.status)) " + "AS status_bar\n" + ' FROM __ref("stg_orders") AS o\n)\nSELECT * FROM final\n', + ), + }, + expected_returncode=1, + expected_fragments=( + "warning[K003]: column 'status_bar' type could not be proven against declared " + "VARCHAR", + 'error[SQBRCONTRACT105]: output "status_bar" is calculated without an outer ' + "explicit cast", + ), + ), + SqlMacroCompileTestCase( + description="an_inferred_type_of_another_family_still_disagrees_through_a_cte", + files={ + **text_macro("shouted", "BIGINT", "UPPER(@value)"), + **contract_model( + "orders", + "shouted (type BIGINT)", + "WITH final AS (\n" + ' SELECT @sql.shouted(o.status) AS shouted FROM __ref("stg_orders") AS o\n' + ")\nSELECT * FROM final\n", + ), + }, + expected_returncode=1, + expected_fragments=("error[K002]: column 'shouted' inferred as",), + ), + ], + ids=lambda case: case.description, +) +def test_given_a_returns_type_when_checking_contracts_then_the_call_has_that_type( + test_case: SqlMacroCompileTestCase, tmp_path: Path +) -> None: + _, result = run_project( + tmp_path=tmp_path, files=test_case.files, command=("compile", "--no-cache") + ) + + assert result.returncode == test_case.expected_returncode, output(result) + for fragment in test_case.expected_fragments: + assert fragment in output(result) + for fragment in test_case.unexpected_fragments: + assert fragment not in output(result) + + +@pytest.mark.parametrize( + "test_case", + [ + SqlMacroCommandTestCase( + description="a_warm_compile_types_the_call_the_same_way", + files=_TYPED_CALLS, + command=("compile",), + expected_returncode=0, + expected_fragments=("0 errors, 0 warnings",), + ), + ], + ids=lambda case: case.description, +) +def test_given_a_returns_type_when_compiling_warm_then_the_call_keeps_that_type( + test_case: SqlMacroCommandTestCase, tmp_path: Path +) -> None: + project_dir, cold = run_project( + tmp_path=tmp_path, files=test_case.files, command=("compile", "--no-cache") + ) + warm: subprocess.CompletedProcess[str] = run_sqb( + command=("--no-color", *test_case.command), project_dir=project_dir + ) + + assert cold.returncode == 0, output(cold) + assert warm.returncode == test_case.expected_returncode, output(warm) + for fragment in test_case.expected_fragments: + assert fragment in output(warm) diff --git a/website/src/content/docs/docs/concepts/macros/sql-macros.mdx b/website/src/content/docs/docs/concepts/macros/sql-macros.mdx index 3db9724684..d8b57a2caf 100644 --- a/website/src/content/docs/docs/concepts/macros/sql-macros.mdx +++ b/website/src/content/docs/docs/concepts/macros/sql-macros.mdx @@ -265,6 +265,16 @@ parameters of a calling macro, and calls in models with runtime placeholders; th column checks still run on the expanded SQL. All other errors also point at the call and at the macro file. +A select-list item that is one expression macro call under an alias, as in +`@sql.cents_to_dollars(o.amount_cents) AS amount`, has the macro's `returns` type wherever the +compiler infers no type for the expanded SQL or a type of the same family, such as `DECIMAL` for +`DECIMAL(18, 2)`. This holds in the main query and in any top-level CTE, and the type flows on as +an inferred type would: through `SELECT * FROM final`, pass-through columns and joins, to contract +checks (`K002`, `K003`), `SQBRCONTRACT105` and models that read the column. Such a call needs no +`CAST` to satisfy a matching contract column. If the inferred type has another family, the inferred +type stands and `K002` reports the mismatch. A call inside a larger expression, a subquery or a +`UNION` branch is typed from the expanded SQL as before. + - `sqb format` formats SQL macro files, bodies of every form and the `MACRO` header, and normalises the commas around CTE-list and column-list calls. - [Rules](/docs/concepts/rules) and [column lineage](/docs/concepts/column-lineage) read the expanded From c84c1910cff9eaa88f65bfbf993f99c08adf682c Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:09:09 +0100 Subject: [PATCH 04/11] fix: treat only absent or unparsed inferred types as unknown when typing SQL macro calls --- .../main/sql_macro_declared_type.rs | 21 +++++++++++++------ .../src/sql_macros/tests/test_output_types.rs | 12 +++++++++++ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs index a7170a4131..2fd8e80ba1 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs @@ -1,5 +1,6 @@ -use crate::sql_macros::_helpers::arguments::type_family; -use crate::sql_macros::models::{DeclaredOutput, TypeFamily}; +use crate::sql_macros::models::DeclaredOutput; +use crate::type_system::main::normalize_type::normalize_type; +use crate::type_system::models::TypeFamily; /// The declared `returns` type an item takes over its inferred type: unknown or same family. #[must_use] @@ -20,8 +21,16 @@ pub fn sql_macro_declared_type( let Some(inferred) = inferred.filter(|inferred| !inferred.is_empty()) else { return Some(output.returns.clone()); }; - let inferred_family: TypeFamily = type_family(inferred, dialect); - let refines: bool = inferred_family == TypeFamily::Unknown - || inferred_family == type_family(&output.returns, dialect); - refines.then(|| output.returns.clone()) + let Some(inferred_family) = known_family(inferred, dialect) else { + return Some(output.returns.clone()); + }; + (known_family(&output.returns, dialect) == Some(inferred_family)) + .then(|| output.returns.clone()) +} + +/// The family of `data_type`, `Other` included; `None` where it does not normalise. +fn known_family(data_type: &str, dialect: &str) -> Option { + normalize_type(data_type, dialect) + .ok() + .map(|normalization| normalization.normalized.family) } diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs index 2ebf1eb935..f97c8b4921 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs @@ -26,6 +26,18 @@ fn given_a_typed_macro_output_when_typing_the_column_then_returns_fills_unknown_ item: (None, "label", Some("BIGINT")), expected_type: None, }, + OutputTypeTestCase { + description: "a known time type is not unknown", + check: typed_check(CallScope::Main, true), + item: (None, "label", Some("TIME")), + expected_type: None, + }, + OutputTypeTestCase { + description: "a known array type is not unknown", + check: typed_check(CallScope::Main, true), + item: (None, "label", Some("TEXT[]")), + expected_type: None, + }, OutputTypeTestCase { description: "another column is untouched", check: typed_check(CallScope::Main, true), From 97c53ce2ba447e2c73e540027fd4700d6689dcb5 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:15:35 +0100 Subject: [PATCH 05/11] fix: give a SQL macro call its returns type only over an unknown or identical inferred type --- .../main/sql_macro_declared_type.rs | 20 +++--- .../src/sql_macros/tests/test_output_types.rs | 8 ++- .../docs/concepts/macros/sql-macros.md | 7 +- .../sql_macros/test_sql_macro_types.py | 65 +++++++++++++++++++ .../docs/docs/concepts/macros/sql-macros.mdx | 7 +- 5 files changed, 88 insertions(+), 19 deletions(-) diff --git a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs index 2fd8e80ba1..36c3dd5374 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs @@ -1,8 +1,8 @@ use crate::sql_macros::models::DeclaredOutput; use crate::type_system::main::normalize_type::normalize_type; -use crate::type_system::models::TypeFamily; +use crate::type_system::models::NormalizedType; -/// The declared `returns` type an item takes over its inferred type: unknown or same family. +/// The declared `returns` type an item takes over its inferred type: unknown or identical. #[must_use] pub fn sql_macro_declared_type( declared: &[DeclaredOutput], @@ -21,16 +21,16 @@ pub fn sql_macro_declared_type( let Some(inferred) = inferred.filter(|inferred| !inferred.is_empty()) else { return Some(output.returns.clone()); }; - let Some(inferred_family) = known_family(inferred, dialect) else { + let Some(inferred_type) = normalized(inferred, dialect) else { return Some(output.returns.clone()); }; - (known_family(&output.returns, dialect) == Some(inferred_family)) - .then(|| output.returns.clone()) + (normalized(&output.returns, dialect) == Some(inferred_type)).then(|| output.returns.clone()) } -/// The family of `data_type`, `Other` included; `None` where it does not normalise. -fn known_family(data_type: &str, dialect: &str) -> Option { - normalize_type(data_type, dialect) - .ok() - .map(|normalization| normalization.normalized.family) +/// `data_type` as compared, or `None` where it does not normalise. +fn normalized(data_type: &str, dialect: &str) -> Option { + match normalize_type(data_type, dialect) { + Ok(normalization) => Some(normalization.normalized), + Err(_unnormalizable) => None, + } } diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs index f97c8b4921..494c767d52 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs @@ -15,7 +15,7 @@ fn given_a_typed_macro_output_when_typing_the_column_then_returns_fills_unknown_ expected_type: Some("VARCHAR"), }, OutputTypeTestCase { - description: "same family takes returns", + description: "an identical type takes returns' spelling", check: typed_check(CallScope::Main, true), item: (None, "LABEL", Some("TEXT")), expected_type: Some("VARCHAR"), @@ -26,6 +26,12 @@ fn given_a_typed_macro_output_when_typing_the_column_then_returns_fills_unknown_ item: (None, "label", Some("BIGINT")), expected_type: None, }, + OutputTypeTestCase { + description: "a longer text type stays", + check: typed_check(CallScope::Main, true), + item: (None, "label", Some("VARCHAR(100)")), + expected_type: None, + }, OutputTypeTestCase { description: "a known time type is not unknown", check: typed_check(CallScope::Main, true), diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md index a064bf7832..c66d4f00f3 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md @@ -279,12 +279,11 @@ macro file. A select-list item that is one expression macro call under an alias, as in `@sql.cents_to_dollars(o.amount_cents) AS amount`, has the macro's `returns` type wherever the -compiler infers no type for the expanded SQL or a type of the same family, such as `DECIMAL` for -`DECIMAL(18, 2)`. This holds in the main query and in any top-level CTE, and the type flows on as +compiler infers no type for the expanded SQL, or exactly that type. This holds in the main query and in any top-level CTE, and the type flows on as an inferred type would: through `SELECT * FROM final`, pass-through columns and joins, to contract checks (`K002`, `K003`), `SQBRCONTRACT105` and models that read the column. Such a call needs no -`CAST` to satisfy a matching contract column. If the inferred type has another family, the inferred -type stands and `K002` reports the mismatch. A call inside a larger expression, a subquery or a +`CAST` to satisfy a matching contract column. Any other inferred type stands, even a +close one such as `VARCHAR(100)` for `returns VARCHAR(5)`, and `K002` reports the mismatch. A call inside a larger expression, a subquery or a `UNION` branch is typed from the expanded SQL as before. - `sqb format` formats SQL macro files, bodies of every form and the `MACRO` header, and diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py index 75bd119bf0..871da6c076 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py @@ -205,6 +205,71 @@ def test_given_an_any_argument_misused_when_compiling_then_it_fails( "explicit cast", ), ), + SqlMacroCompileTestCase( + description="a_time_body_keeps_its_inferred_type", + files={ + **text_macro("m", "VARCHAR", "CAST('10:00:00' AS TIME)"), + **contract_model( + "orders", + "v (type VARCHAR)", + 'SELECT @sql.m(o.status) AS v FROM __ref("stg_orders") AS o\n', + ), + }, + expected_returncode=1, + expected_fragments=("error[K002]: column 'v' inferred as",), + ), + SqlMacroCompileTestCase( + description="an_array_body_keeps_its_inferred_type", + files={ + **text_macro("m", "VARCHAR", "[@value]"), + **contract_model( + "orders", + "v (type VARCHAR)", + 'SELECT @sql.m(o.status) AS v FROM __ref("stg_orders") AS o\n', + ), + }, + expected_returncode=1, + expected_fragments=("error[K002]: column 'v' inferred as",), + ), + SqlMacroCompileTestCase( + description="a_longer_text_body_keeps_its_inferred_type", + files={ + **text_macro("m", "VARCHAR(5)", "CAST(@value AS VARCHAR(100))"), + **contract_model( + "orders", + "v (type VARCHAR(5))", + 'SELECT @sql.m(o.status) AS v FROM __ref("stg_orders") AS o\n', + ), + }, + expected_returncode=1, + expected_fragments=("error[K002]: column 'v' inferred as",), + ), + SqlMacroCompileTestCase( + description="a_wider_decimal_body_keeps_its_inferred_type", + files={ + **text_macro("m", "DECIMAL(4, 1)", "CAST(LENGTH(@value) AS DECIMAL(18, 6))"), + **contract_model( + "orders", + "v (type DECIMAL(4, 1))", + 'SELECT @sql.m(o.status) AS v FROM __ref("stg_orders") AS o\n', + ), + }, + expected_returncode=1, + expected_fragments=("error[K002]: column 'v' inferred as",), + ), + SqlMacroCompileTestCase( + description="a_timestamptz_body_keeps_its_inferred_type", + files={ + **text_macro("m", "TIMESTAMP", "CAST(@value AS TIMESTAMPTZ)"), + **contract_model( + "orders", + "v (type TIMESTAMP)", + 'SELECT @sql.m(o.status) AS v FROM __ref("stg_orders") AS o\n', + ), + }, + expected_returncode=1, + expected_fragments=("error[K002]: column 'v' inferred as",), + ), SqlMacroCompileTestCase( description="an_inferred_type_of_another_family_still_disagrees_through_a_cte", files={ diff --git a/website/src/content/docs/docs/concepts/macros/sql-macros.mdx b/website/src/content/docs/docs/concepts/macros/sql-macros.mdx index d8b57a2caf..7d71f106a9 100644 --- a/website/src/content/docs/docs/concepts/macros/sql-macros.mdx +++ b/website/src/content/docs/docs/concepts/macros/sql-macros.mdx @@ -267,12 +267,11 @@ macro file. A select-list item that is one expression macro call under an alias, as in `@sql.cents_to_dollars(o.amount_cents) AS amount`, has the macro's `returns` type wherever the -compiler infers no type for the expanded SQL or a type of the same family, such as `DECIMAL` for -`DECIMAL(18, 2)`. This holds in the main query and in any top-level CTE, and the type flows on as +compiler infers no type for the expanded SQL, or exactly that type. This holds in the main query and in any top-level CTE, and the type flows on as an inferred type would: through `SELECT * FROM final`, pass-through columns and joins, to contract checks (`K002`, `K003`), `SQBRCONTRACT105` and models that read the column. Such a call needs no -`CAST` to satisfy a matching contract column. If the inferred type has another family, the inferred -type stands and `K002` reports the mismatch. A call inside a larger expression, a subquery or a +`CAST` to satisfy a matching contract column. Any other inferred type stands, even a +close one such as `VARCHAR(100)` for `returns VARCHAR(5)`, and `K002` reports the mismatch. A call inside a larger expression, a subquery or a `UNION` branch is typed from the expanded SQL as before. - `sqb format` formats SQL macro files, bodies of every form and the `MACRO` header, and From 027788f55d502074424888e5117cba675f7e6cfa Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:23:59 +0100 Subject: [PATCH 06/11] fix: keep compiling when SQL macro output types cannot be propagated through a model --- .../analysis_session/_helpers/session.rs | 11 ++- .../sql_macros/test_sql_macro_types.py | 75 +++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs index 077fb25565..304d36896f 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs @@ -318,12 +318,19 @@ impl AnalysisSession { callback: self.request.nullability_callback.as_ref(), declared: &declared, }; - let typed: Vec<(String, String)> = catch_compiler_panic(|| { - declared_output_types(&cleaned, &self.available_types, &profile) + let propagated: Result, String> = catch_compiler_panic(|| { + Ok(declared_output_types( + &cleaned, + &self.available_types, + &profile, + )) }) .map_err(|reason| { native_failure(&model_context(&self.request.models[*model]), &reason) })?; + let Ok(typed) = propagated else { + continue; + }; let dialect: String = self.request.dialect.clone(); let Some(columns) = self.outcome_mut(*model)?.analysis.columns.as_mut() else { continue; diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py index 871da6c076..c2720a6bba 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py @@ -270,6 +270,81 @@ def test_given_an_any_argument_misused_when_compiling_then_it_fails( expected_returncode=1, expected_fragments=("error[K002]: column 'v' inferred as",), ), + SqlMacroCompileTestCase( + description="an_unusual_outer_query_star_replace_still_compiles", + files={ + **_TYPED_RULES_AND_MACROS, + **orders_model( + "WITH final AS (\n" + " SELECT o.order_id, @sql.shouted(o.status) AS shouted\n" + ' FROM __ref("stg_orders") AS o\n' + ")\n" + "SELECT * REPLACE (UPPER(shouted) AS shouted) FROM final\n" + ), + }, + expected_returncode=0, + expected_fragments=("0 errors",), + ), + SqlMacroCompileTestCase( + description="an_unusual_outer_query_star_exclude_still_compiles", + files={ + **_TYPED_RULES_AND_MACROS, + **orders_model( + "WITH final AS (\n" + " SELECT o.order_id, @sql.shouted(o.status) AS shouted\n" + ' FROM __ref("stg_orders") AS o\n' + ")\n" + "SELECT * EXCLUDE (order_id) FROM final\n" + ), + }, + expected_returncode=0, + expected_fragments=("0 errors",), + ), + SqlMacroCompileTestCase( + description="an_unusual_outer_query_columns_expression_still_compiles", + files={ + **_TYPED_RULES_AND_MACROS, + **orders_model( + "WITH final AS (\n" + " SELECT o.order_id, @sql.shouted(o.status) AS shouted\n" + ' FROM __ref("stg_orders") AS o\n' + ")\n" + "SELECT COLUMNS('shout.*') FROM final\n" + ), + }, + expected_returncode=0, + expected_fragments=("0 errors",), + ), + SqlMacroCompileTestCase( + description="an_unusual_outer_query_star_rename_still_compiles", + files={ + **_TYPED_RULES_AND_MACROS, + **orders_model( + "WITH final AS (\n" + " SELECT o.order_id, @sql.shouted(o.status) AS shouted\n" + ' FROM __ref("stg_orders") AS o\n' + ")\n" + "SELECT * RENAME (shouted AS loud) FROM final\n" + ), + }, + expected_returncode=0, + expected_fragments=("0 errors",), + ), + SqlMacroCompileTestCase( + description="an_unusual_outer_query_full_join_using_still_compiles", + files={ + **_TYPED_RULES_AND_MACROS, + **orders_model( + "WITH final AS (\n" + " SELECT o.order_id, @sql.shouted(o.status) AS shouted\n" + ' FROM __ref("stg_orders") AS o\n' + ")\n" + "SELECT f.* FROM final AS f FULL JOIN final AS g USING (order_id)\n" + ), + }, + expected_returncode=0, + expected_fragments=("0 errors",), + ), SqlMacroCompileTestCase( description="an_inferred_type_of_another_family_still_disagrees_through_a_cte", files={ From 9f051fa8323b8d51333e609534d3732c23cc9619 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:40:19 +0100 Subject: [PATCH 07/11] perf: skip SQL macro output typing for cached analyses that already hold it --- .../analysis_session/_helpers/session.rs | 7 ++- .../analysis_session/tests/helpers.rs | 23 +++++++ .../tests/test_analysis_cache.rs | 61 +++++++++++++++++-- .../analysis_session/tests/test_types.rs | 9 +++ 4 files changed, 95 insertions(+), 5 deletions(-) diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs index 304d36896f..5f48794267 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs @@ -301,7 +301,7 @@ impl AnalysisSession { for model in models { let declared: Vec = sql_macro_declared_outputs(&self.request.models[*model].sql_macro_checks); - if declared.is_empty() { + if declared.is_empty() || self.cache_hit(*model) { continue; } self.ensure_cleaned(&[*model])?; @@ -360,6 +360,11 @@ impl AnalysisSession { Ok(()) } + /// Whether the cache answered `model` with an outcome that already holds its declared types. + fn cache_hit(&self, model: usize) -> bool { + self.cache.as_ref().is_some_and(|cache| cache.hits[model]) + } + /// Each model's SQL macro call arguments checked against its analysed types, cache hits too. fn check_sql_macros(&mut self, models: &[usize]) -> Result<(), String> { let checked: Vec = models diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/helpers.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/helpers.rs index 77d31e4b7e..6234524b47 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/helpers.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/helpers.rs @@ -27,6 +27,7 @@ use crate::assembly::analysis_session::tests::test_types::{ }; use crate::assembly::analysis_session::types::{Pairs, Shapes}; use crate::semantic_validation::models::{CatalogInput, ProjectCatalog}; +use crate::sql_macros::models::{CallScope, SqlMacroCheck, TypedOutput}; const TRANSFORM_NAMES: [&str; 6] = [ "direct", @@ -179,6 +180,11 @@ pub(crate) fn cached_run(request: SessionRequest, store: NativeStore) -> CachedR (keys, hits, taken) }); CachedRun { + deferred: outcome + .models + .iter() + .map(|model| model.deferred_cleaning.is_some()) + .collect(), lines: ( steps.iter().map(step_lines).collect(), outcome.models.iter().map(described).collect(), @@ -699,3 +705,20 @@ pub(crate) fn injected_relations( request.forced_order = Some((relations.dependency_ordered, relations.cyclic)); request } + +/// The orders request over `models`, the first typing `label` in CTE `final` as VARCHAR. +pub(crate) fn declared_label_request(models: &[ModelSpec]) -> SessionRequest { + let mut requests: Vec = model_requests(models); + requests[0].sql_macro_checks.push(SqlMacroCheck { + macro_name: "label".to_owned(), + scope: CallScope::Cte("final".to_owned()), + plain_select: true, + arguments: Vec::new(), + call: None, + output: Some(TypedOutput { + name: "label".to_owned(), + returns: "VARCHAR".to_owned(), + }), + }); + orders_request(requests) +} diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/test_analysis_cache.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/test_analysis_cache.rs index 24c55a4b51..914908700f 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/test_analysis_cache.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/test_analysis_cache.rs @@ -3,13 +3,13 @@ use sqlbuild_cache::store::models::NativeStore; use crate::assembly::analysis_session::models::{ModelReference, SessionRequest}; use crate::assembly::analysis_session::tests::helpers::{ - amounts_family, cache_stats, cached_run, current_model_key, damaged, model_keys, - model_requests, orders_request, pairs, second_model, session_lines, shapes, started, - uncached_catalog, + amounts_family, cache_stats, cached_run, completed, current_model_key, damaged, + declared_label_request, described, model_keys, model_requests, orders_request, pairs, + second_model, session_lines, shapes, started, step_lines, uncached_catalog, }; use crate::assembly::analysis_session::tests::test_types::{ CacheEditTestCase, CacheKeyTestCase, CacheReuseTestCase, CachedRun, DamagedCacheTestCase, - ModelSpec, RelationDigestTestCase, + DeclaredHitTestCase, ModelSpec, RelationDigestTestCase, }; const STG_ORDERS: ModelSpec = ( @@ -73,6 +73,15 @@ const EVENTS_TOO_DEEP: ModelSpec = ( &[], ); +/// Labels orders with a function the inference does not type, in CTE `final`. +const LABELLED: ModelSpec = ( + "labelled", + "WITH final AS (SELECT order_id, bar(amount, 0, 10) AS label FROM __source(\"raw_orders\")) \ + SELECT * FROM final", + &["raw_orders"], + &[], +); + #[test] fn given_a_filled_cache_when_rerunning_then_hits_match_the_uncached_session() { let test_cases = [ @@ -519,3 +528,47 @@ fn given_a_read_relation_fact_changed_when_keying_then_the_reading_model_key_cha ); } } + +#[test] +fn given_a_declared_sql_macro_output_when_rerunning_then_the_hit_keeps_its_type_unpatched() { + let test_cases = [DeclaredHitTestCase { + description: "a call item in a final CTE, read with a star", + models: &[LABELLED], + expected_label: "label VARCHAR", + }]; + for test_case in test_cases { + let (uncached_steps, uncached_models, _) = + completed(started(declared_label_request(test_case.models))); + let cold: CachedRun = cached_run( + declared_label_request(test_case.models), + NativeStore::default(), + ); + let cold_lines = cold.lines; + let warm: CachedRun = cached_run(declared_label_request(test_case.models), cold.store); + let uncached = ( + uncached_steps.iter().map(step_lines).collect::>(), + uncached_models.iter().map(described).collect::>(), + ); + + assert!( + uncached.1[0] + .iter() + .any(|line| line.starts_with(test_case.expected_label)), + "{}: {:?}", + test_case.description, + uncached.1 + ); + assert_eq!( + (&cold_lines, &warm.lines), + (&uncached, &uncached), + "{}", + test_case.description + ); + assert_eq!( + (warm.hits, warm.deferred), + (vec![true], vec![true]), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/test_types.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/test_types.rs index 6a3236ccd9..9568fadd92 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/test_types.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/test_types.rs @@ -144,6 +144,8 @@ pub(crate) struct CachedRun { pub(crate) keys: Vec>, /// Whether each model's outcome came from the cache. pub(crate) hits: Vec, + /// Whether each model's outcome still defers its cleaned SQL. + pub(crate) deferred: Vec, /// The catalog's schema additions and analysis names, then the sorted fact models. pub(crate) catalog: ( crate::assembly::analysis_session::types::Shapes, @@ -217,3 +219,10 @@ pub(crate) struct DeltaSessionTestCase { pub(crate) model_index: usize, pub(crate) expected_whole_session_outcome: bool, } + +/// Models whose first one declares a SQL macro output, and that output's described column. +pub(crate) struct DeclaredHitTestCase { + pub(crate) description: &'static str, + pub(crate) models: &'static [ModelSpec], + pub(crate) expected_label: &'static str, +} From 08e5ad39b6d9aca4eba58a27ee986287ad82a936 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 23 Oct 2026 19:24:03 +0100 Subject: [PATCH 08/11] feat: type SQL macro call items in each branch of a set operation from their returns type --- .../_helpers/analysis_cache.rs | 1 + .../analysis_session/_helpers/cte_facts.rs | 131 ++++++++++++------ .../analysis_session/_helpers/session.rs | 3 +- .../analysis_session/tests/helpers.rs | 1 + .../src/sql_macros/_helpers/expansion.rs | 15 +- .../src/sql_macros/_helpers/placement.rs | 65 +++++++++ .../main/sql_macro_declared_outputs.rs | 4 +- .../main/sql_macro_declared_type.rs | 3 +- .../sql_macros/main/sql_macro_output_names.rs | 13 +- .../src/sql_macros/models.rs | 4 + .../src/sql_macros/tests/helpers.rs | 7 +- .../src/sql_macros/tests/test_output_types.rs | 73 +++++----- .../src/sql_macros/tests/test_types.rs | 11 +- .../rules/_helpers/explicit_output_types.rs | 38 +++-- .../docs/concepts/macros/sql-macros.md | 16 ++- .../sql_macros/test_sql_macro_types.py | 65 +++++++++ .../docs/docs/concepts/macros/sql-macros.mdx | 16 ++- 17 files changed, 350 insertions(+), 116 deletions(-) diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/analysis_cache.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/analysis_cache.rs index 8cb400c6a8..129c9ec5ce 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/analysis_cache.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/analysis_cache.rs @@ -183,6 +183,7 @@ impl KeyHasher { } self.text(&output.name); self.text(&output.returns); + self.count(output.branch); } } diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/cte_facts.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/cte_facts.rs index c35adf3b6d..fe242b06df 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/cte_facts.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/cte_facts.rs @@ -143,15 +143,63 @@ pub(crate) fn declared_output_types( return Ok(Vec::new()); }; let facts: Relations = referenced_types(&parsed, types); - let context = Context { profile }; - let declared: Vec<(String, String)> = context.model_passthrough_types(&parsed, &facts)?; - let plain: Vec<(String, String)> = context.passthrough_types(&parsed, &facts)?; + let declared: Vec<(String, String)> = Context { + profile, + branches: declared_branches(&parsed), + } + .query_types(&parsed, &facts)?; + let plain: Vec<(String, String)> = Context { + profile, + branches: Vec::new(), + } + .query_types(&parsed, &facts)?; + let main: HashSet = profile + .declared + .iter() + .filter(|output| output.cte.is_none() && output.branch == 0) + .map(|output| casefold(&output.name)) + .collect(); Ok(declared .into_iter() - .filter(|(name, data_type)| dict_get(&plain, name) != Some(data_type)) + .filter(|(name, data_type)| { + dict_get(&plain, name) != Some(data_type) || main.contains(&casefold(name)) + }) .collect()) } +/// Each select of the main query and top-level CTEs by address, with its scope and branch. +fn declared_branches(root: &Expression) -> Vec { + let mut branches: Vec = Vec::new(); + for (cte_name, has_column_aliases, body) in top_level_ctes(root) { + if !has_column_aliases + && unwrap_annotations(Some(body)).is_some_and(|body| top_level_ctes(body).is_empty()) + { + for (branch, select) in set_branches(Some(body)).into_iter().enumerate() { + branches.push((select, Some(cte_name.to_owned()), branch)); + } + } + } + for (branch, select) in set_branches(Some(root)).into_iter().enumerate() { + branches.push((select, None, branch)); + } + branches +} + +/// The addresses of the selects of `node`'s set operation, left to right, or of `node` itself. +fn set_branches(node: Option<&Expression>) -> Vec<*const Expression> { + let Some(node) = unwrap_annotations(node) else { + return Vec::new(); + }; + match set_operation(node) { + Some((left, right, _)) => { + let mut branches: Vec<*const Expression> = set_branches(Some(left)); + branches.extend(set_branches(Some(right))); + branches + } + None => vec![std::ptr::from_ref(node)], + } +} + /// Python's legacy analysis of a model the native engine handed back, or why it defers. pub(crate) fn legacy_analysis(input: &LegacyInput<'_>) -> Fact { let Some(parsed) = parse_one(input.cleaned_sql, input.profile.dialect)? else { @@ -189,6 +237,7 @@ pub(crate) fn legacy_analysis(input: &LegacyInput<'_>) -> Fact { let unqualified: Option<&(String, String)> = single_resource(&resources); let context = Context { profile: &input.profile, + branches: Vec::new(), }; let (cte_types, cte_nullability) = if input.recover { ( @@ -459,6 +508,7 @@ fn passthrough_facts( } let context = Context { profile: &input.profile, + branches: Vec::new(), }; recovery.types = context.passthrough_types(root, &input_types(input))?; recovery.nullability = context.passthrough_nullability(root, &input_nullability(input))?; @@ -489,53 +539,26 @@ fn direct_cte_outputs(root: &Expression, cte_names: &HashSet) -> HashSet outputs } +/// A select's address, its top-level CTE or `None` for the main query, and its branch. +type Branch = (*const Expression, Option, usize); + /// The profile every resolver reads. struct Context<'a, 'b> { profile: &'a RecoveryProfile<'b>, + /// The selects whose SQL macro call items take their declared types. + branches: Vec, } impl Context<'_, '_> { - /// The model query's pass-through types, its SQL macro call items typed by `returns`. - fn model_passthrough_types( - &self, - root: &Expression, - facts: &Relations, - ) -> Fact> { - let types: Vec<(String, String)> = - self.passthrough_types_in(root, facts, Some(self.profile.declared))?; - Ok(self.declared_types(None, types)) - } - - /// Python's `_polyglot_cte_passthrough_types_from_parsed`. - fn passthrough_types( - &self, - root: &Expression, - facts: &Relations, - ) -> Fact> { - self.passthrough_types_in(root, facts, None) - } - - /// The pass-through types of `root`, typing the declared items of its own CTEs. - fn passthrough_types_in( - &self, - root: &Expression, - facts: &Relations, - declared: Option<&[DeclaredOutput]>, - ) -> Fact> { - let ctes: Vec<(&str, bool, &Expression)> = top_level_ctes(root); - if ctes.is_empty() { - return Ok(Vec::new()); - } + /// The model query's output types, through its top-level CTEs. + fn query_types(&self, root: &Expression, facts: &Relations) -> Fact> { let mut relations: Relations = referenced_facts(root, facts); - for (cte_name, has_column_aliases, body) in ctes { + for (cte_name, has_column_aliases, body) in top_level_ctes(root) { if has_column_aliases { continue; } - let mut inferred: Vec<(String, String)> = + let inferred: Vec<(String, String)> = self.select_output_types(Some(body), &relations)?; - if declared.is_some() { - inferred = self.declared_types(Some(cte_name), inferred); - } if !inferred.is_empty() { relations = dict_set(relations, cte_name, Rc::new(inferred)); } @@ -546,12 +569,32 @@ impl Context<'_, '_> { self.direct_select_types(Some(root), &relations) } - /// `types` of the main query or CTE `cte` with each declared SQL macro call item typed. + /// Python's `_polyglot_cte_passthrough_types_from_parsed`. + fn passthrough_types( + &self, + root: &Expression, + facts: &Relations, + ) -> Fact> { + if top_level_ctes(root).is_empty() { + return Ok(Vec::new()); + } + self.query_types(root, facts) + } + + /// `types` of `select` with each of its SQL macro call items typed by its `returns`. fn declared_types( &self, - cte: Option<&str>, + select: &Expression, mut types: Vec<(String, String)>, ) -> Vec<(String, String)> { + let address: *const Expression = std::ptr::from_ref(select); + let Some((_, cte, branch)) = self + .branches + .iter() + .find(|(branch_select, _, _)| *branch_select == address) + else { + return types; + }; for output in self.profile.declared { let key: String = types .iter() @@ -562,7 +605,7 @@ impl Context<'_, '_> { let inferred: Option<&str> = dict_get(&types, &key).map(String::as_str); if let Some(declared) = sql_macro_declared_type( self.profile.declared, - (cte, &output.name), + (cte.as_deref(), *branch, &output.name), inferred, self.profile.dialect, ) { @@ -762,7 +805,7 @@ impl Context<'_, '_> { inferred = dict_set(inferred, output, inferred_type); } } - Ok(inferred) + Ok(self.declared_types(select, inferred)) } /// Python's `_polyglot_direct_expression_type`. diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs index 5f48794267..d8d1e7d87a 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/session.rs @@ -346,10 +346,11 @@ impl AnalysisSession { cte: None, name: name.clone(), returns: returns.clone(), + branch: 0, }]; if let Some(data_type) = sql_macro_declared_type( &reached, - (None, &column.name), + (None, 0, &column.name), column.data_type.as_deref(), &dialect, ) { diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/helpers.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/helpers.rs index 6234524b47..49eb329990 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/helpers.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/tests/helpers.rs @@ -718,6 +718,7 @@ pub(crate) fn declared_label_request(models: &[ModelSpec]) -> SessionRequest { output: Some(TypedOutput { name: "label".to_owned(), returns: "VARCHAR".to_owned(), + branch: 0, }), }); orders_request(requests) diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/expansion.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/expansion.rs index d69d162310..c8e3ed4db7 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/expansion.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/expansion.rs @@ -222,12 +222,14 @@ pub(crate) fn expand_call( ); if matches!(context.scope, SqlMacroScope::Caller) && context.stack.is_empty() { let output: Option = match (definition.form, &definition.returns) { - (SqlMacroForm::Expression, Some(returns)) => { - site.whole_item_alias().map(|name| TypedOutput { + (SqlMacroForm::Expression, Some(returns)) => site + .whole_item_alias() + .zip(call_branch(context.text, scanned, call.start)) + .map(|(name, branch)| TypedOutput { name, returns: returns.clone(), - }) - } + branch, + }), _ => None, }; let check: SqlMacroCheck = SqlMacroCheck { @@ -601,6 +603,11 @@ fn call_scope(text: &str, scanned: &[Token], offset: usize) -> CallScope { } } +/// The branch of its scope's set operation the call at byte `offset` of `text` stands in. +fn call_branch(text: &str, scanned: &[Token], offset: usize) -> Option { + top_level_parts(text, scanned)?.branch_of(text, scanned, offset) +} + /// Whether the call at byte `offset` of `text` stands in its scope's one select. fn plain_select(text: &str, scanned: &[Token], offset: usize) -> bool { top_level_parts(text, scanned).is_some_and(|parts| parts.plain_select(text, scanned, offset)) diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/placement.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/placement.rs index ac62490e92..a94ea344f3 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/placement.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/placement.rs @@ -11,6 +11,8 @@ const DISTINCT_ON_TOKENS: usize = 2; const QUERY_OPENERS: [&str; 4] = ["SELECT", "WITH", "VALUES", "FROM"]; /// Keywords that combine the selects of a query. const SET_OPERATIONS: [&str; 3] = ["UNION", "INTERSECT", "EXCEPT"]; +/// The words that may stand between a set operation and its parenthesised right branch. +const SET_OPERATION_MODIFIERS: [&str; 3] = ["ALL", "DISTINCT", "NAME"]; /// Tokens from `@` to a macro call's name: `@`, the kind, `.` and the name. const CALL_PREFIX_TOKENS: usize = 4; /// Keywords that open the clause a call stands in, read backwards from the call. @@ -611,6 +613,69 @@ impl QueryParts { } } +impl QueryParts { + /// The set-operation branch byte `offset` stands in; `None` where it cannot be counted. + pub(crate) fn branch_of(&self, text: &str, scanned: &[Token], offset: usize) -> Option { + let range: (usize, usize) = match self.scope_of(offset) { + CallScope::WithList => return None, + CallScope::Cte(name) => self.ctes.iter().find(|cte| cte.name == name)?.body, + CallScope::Main => (self.main, text.len()), + }; + let inside: Vec<(usize, &Token)> = scanned + .iter() + .enumerate() + .filter(|(_, token)| range.0 <= token.start && token.end <= range.1) + .collect(); + if inside + .first() + .is_some_and(|(_, token)| token.is_word(text, WITH_KEYWORD)) + { + return None; + } + let mut depth: usize = 0; + let mut branch: usize = 0; + let mut at_call: Option = None; + let mut previous_at_top: Option<&Token> = None; + for (index, token) in inside { + if at_call.is_none() && token.start >= offset { + if depth != 0 { + return None; + } + at_call = Some(branch); + } + if token.kind == TokenKind::Punct { + match token.text(text) { + "(" | "[" | "{" => { + let query: bool = scanned + .get(index + 1) + .is_some_and(|next| opens_query(text, next)); + let branch_start: bool = previous_at_top.is_none_or(|previous| { + let word: &str = previous.text(text); + is_set_operation(word) + || SET_OPERATION_MODIFIERS + .iter() + .any(|modifier| word.eq_ignore_ascii_case(modifier)) + }); + if depth == 0 && query && branch_start { + return None; + } + depth += 1; + } + ")" | "]" | "}" => depth = depth.saturating_sub(1), + _ => {} + } + } + if depth == 0 { + if token.kind == TokenKind::Word && is_set_operation(token.text(text)) { + branch += 1; + } + previous_at_top = Some(token); + } + } + at_call + } +} + /// Whether `token` opens a query, as the first token inside a parenthesis. fn opens_query(text: &str, token: &Token) -> bool { QUERY_OPENERS diff --git a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_outputs.rs b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_outputs.rs index d52b0b47a2..f9c379d720 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_outputs.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_outputs.rs @@ -1,11 +1,10 @@ use crate::sql_macros::models::{CallScope, DeclaredOutput, SqlMacroCheck}; -/// The select items of a model's main query and top-level CTEs that are one macro call each. +/// The select items, in any set-operation branch, of a model's top-level queries that are one call. #[must_use] pub fn sql_macro_declared_outputs(checks: &[SqlMacroCheck]) -> Vec { checks .iter() - .filter(|check| check.plain_select) .filter_map(|check| { let output = check.output.as_ref()?; let cte: Option = match &check.scope { @@ -17,6 +16,7 @@ pub fn sql_macro_declared_outputs(checks: &[SqlMacroCheck]) -> Vec, &str), + (cte, branch, name): (Option<&str>, usize, &str), inferred: Option<&str>, dialect: &str, ) -> Option { let output: &DeclaredOutput = declared.iter().find(|output| { output.name.eq_ignore_ascii_case(name) + && output.branch == branch && match (&output.cte, cte) { (None, None) => true, (Some(declared_cte), Some(cte)) => declared_cte.eq_ignore_ascii_case(cte), diff --git a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_output_names.rs b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_output_names.rs index b033cf0136..5e06e4bbf5 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_output_names.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_output_names.rs @@ -3,9 +3,9 @@ use crate::sql_macros::_helpers::lexing::{Token, tokens}; use crate::sql_macros::_helpers::placement::{CallSite, QueryParts, top_level_parts}; use crate::sql_macros::models::{CallScope, SqlMacroCall}; -/// The `(top-level CTE, alias)` of each select item of `text` that is one SQL macro call. +/// The `(top-level CTE, branch, alias)` of each select item of `text` that is one SQL macro call. #[must_use] -pub fn sql_macro_output_names(text: &str) -> Vec<(Option, String)> { +pub fn sql_macro_output_names(text: &str) -> Vec<(Option, usize, String)> { let Ok(calls) = sql_macro_calls(text) else { return Vec::new(); }; @@ -20,23 +20,24 @@ pub fn sql_macro_output_names(text: &str) -> Vec<(Option, String)> { }; calls .iter() - .filter(|call| !call.in_python && parts.plain_select(text, &scanned, call.start)) + .filter(|call| !call.in_python) .filter_map(|call| whole_item(text, (&scanned, &parts), call)) .collect() } -/// The `(top-level CTE, alias)` of `call` when it is a whole aliased select item. +/// The `(top-level CTE, branch, alias)` of `call` when it is a whole aliased select item. fn whole_item( text: &str, (scanned, parts): (&[Token], &QueryParts), call: &SqlMacroCall, -) -> Option<(Option, String)> { +) -> Option<(Option, usize, String)> { let cte: Option = match parts.scope_of(call.start) { CallScope::Main => None, CallScope::Cte(name) => Some(name), CallScope::WithList => return None, }; + let branch: usize = parts.branch_of(text, scanned, call.start)?; CallSite::new(text, scanned, (call.start, call.end), call.replaced_end) .whole_item_alias() - .map(|alias| (cte, alias)) + .map(|alias| (cte, branch, alias)) } diff --git a/crates/sqlbuild-analysis/src/sql_macros/models.rs b/crates/sqlbuild-analysis/src/sql_macros/models.rs index 1cf1e7ba87..8d60199c21 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/models.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/models.rs @@ -268,6 +268,8 @@ pub struct DeclaredOutput { /// The item's alias, unquoted. pub name: String, pub returns: String, + /// The select it stands in: 0, or its branch of a set operation, left to right. + pub branch: usize, } /// A select-list item that is one expression macro call under an alias, as `@sql.m(x) AS name`. @@ -277,6 +279,8 @@ pub struct TypedOutput { pub name: String, /// The macro's declared `returns` type. pub returns: String, + /// The select the item stands in: 0, or its branch of a set operation, left to right. + pub branch: usize, } /// The top-level part of the caller's query a call stands in. diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/helpers.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/helpers.rs index b4fea83df5..ec8f644996 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests/helpers.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/helpers.rs @@ -129,17 +129,18 @@ impl sqlbuild_render::macro_arguments::types::ArgumentHost for AsciiArguments { } } -/// A check recording output `label` of `shout()`, typed VARCHAR, at `scope`. -pub(super) fn typed_check(scope: CallScope, plain_select: bool) -> SqlMacroCheck { +/// A check recording output `label` of `shout()`, typed VARCHAR, at `scope`'s `branch`. +pub(super) fn typed_check(scope: CallScope, branch: usize) -> SqlMacroCheck { SqlMacroCheck { macro_name: "shout".to_owned(), scope, - plain_select, + plain_select: true, arguments: Vec::new(), call: None, output: Some(TypedOutput { name: "label".to_owned(), returns: "VARCHAR".to_owned(), + branch, }), } } diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs index 494c767d52..119262fc15 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs @@ -10,70 +10,76 @@ fn given_a_typed_macro_output_when_typing_the_column_then_returns_fills_unknown_ let test_cases = [ OutputTypeTestCase { description: "unknown inferred type takes returns", - check: typed_check(CallScope::Main, true), - item: (None, "label", None), + check: typed_check(CallScope::Main, 0), + item: (None, 0, "label", None), expected_type: Some("VARCHAR"), }, OutputTypeTestCase { description: "an identical type takes returns' spelling", - check: typed_check(CallScope::Main, true), - item: (None, "LABEL", Some("TEXT")), + check: typed_check(CallScope::Main, 0), + item: (None, 0, "LABEL", Some("TEXT")), expected_type: Some("VARCHAR"), }, OutputTypeTestCase { description: "a different known family keeps the inferred type", - check: typed_check(CallScope::Main, true), - item: (None, "label", Some("BIGINT")), + check: typed_check(CallScope::Main, 0), + item: (None, 0, "label", Some("BIGINT")), expected_type: None, }, OutputTypeTestCase { description: "a longer text type stays", - check: typed_check(CallScope::Main, true), - item: (None, "label", Some("VARCHAR(100)")), + check: typed_check(CallScope::Main, 0), + item: (None, 0, "label", Some("VARCHAR(100)")), expected_type: None, }, OutputTypeTestCase { description: "a known time type is not unknown", - check: typed_check(CallScope::Main, true), - item: (None, "label", Some("TIME")), + check: typed_check(CallScope::Main, 0), + item: (None, 0, "label", Some("TIME")), expected_type: None, }, OutputTypeTestCase { description: "a known array type is not unknown", - check: typed_check(CallScope::Main, true), - item: (None, "label", Some("TEXT[]")), + check: typed_check(CallScope::Main, 0), + item: (None, 0, "label", Some("TEXT[]")), expected_type: None, }, OutputTypeTestCase { description: "another column is untouched", - check: typed_check(CallScope::Main, true), - item: (None, "other", None), + check: typed_check(CallScope::Main, 0), + item: (None, 0, "other", None), expected_type: None, }, OutputTypeTestCase { description: "a call in a CTE types that CTE's item", - check: typed_check(CallScope::Cte("base".to_owned()), true), - item: (Some("BASE"), "label", None), + check: typed_check(CallScope::Cte("base".to_owned()), 0), + item: (Some("BASE"), 0, "label", None), expected_type: Some("VARCHAR"), }, OutputTypeTestCase { description: "a call in a CTE leaves a main-query item of that name", - check: typed_check(CallScope::Cte("base".to_owned()), true), - item: (None, "label", None), + check: typed_check(CallScope::Cte("base".to_owned()), 0), + item: (None, 0, "label", None), expected_type: None, }, OutputTypeTestCase { - description: "a call in a set operation is not typed", - check: typed_check(CallScope::Main, false), - item: (None, "label", None), + description: "a call in another branch of a set operation leaves this one", + check: typed_check(CallScope::Main, 1), + item: (None, 0, "label", None), expected_type: None, }, + OutputTypeTestCase { + description: "a call in a later branch of a set operation types that branch", + check: typed_check(CallScope::Cte("base".to_owned()), 1), + item: (Some("base"), 1, "label", Some("TEXT")), + expected_type: Some("VARCHAR"), + }, ]; for test_case in test_cases { - let (cte, name, inferred) = test_case.item; + let (cte, branch, name, inferred) = test_case.item; let declared = sql_macro_declared_outputs(&[test_case.check]); - let typed = sql_macro_declared_type(&declared, (cte, name), inferred, "duckdb"); + let typed = sql_macro_declared_type(&declared, (cte, branch, name), inferred, "duckdb"); assert_eq!( typed.as_deref(), @@ -90,7 +96,7 @@ fn given_model_sql_when_reading_sql_macro_outputs_then_only_whole_aliased_items_ OutputNamesTestCase { description: "aliased whole items, with and without AS", sql: "SELECT @sql.a(x) AS one, @sql.b(y) two, @sql.c(z) AS \"Three\" FROM t", - expected_names: &[(None, "one"), (None, "two"), (None, "Three")], + expected_names: &[(None, 0, "one"), (None, 0, "two"), (None, 0, "Three")], }, OutputNamesTestCase { description: "calls inside expressions or without an alias", @@ -98,22 +104,27 @@ fn given_model_sql_when_reading_sql_macro_outputs_then_only_whole_aliased_items_ expected_names: &[], }, OutputNamesTestCase { - description: "a call in a CTE, and one in a set operation", - sql: "WITH c AS (SELECT @sql.a(x) AS one FROM t) SELECT @sql.b(y) AS two FROM c UNION ALL SELECT 'x' FROM c", - expected_names: &[(Some("c"), "one")], + description: "a call in a CTE, and ones in each branch of a set operation", + sql: "WITH c AS (SELECT @sql.a(x) AS one FROM t) SELECT @sql.b(y) AS two FROM c UNION ALL SELECT 'x' FROM c UNION ALL SELECT @sql.c(z) AS three FROM c", + expected_names: &[(Some("c"), 0, "one"), (None, 0, "two"), (None, 2, "three")], + }, + OutputNamesTestCase { + description: "calls in subqueries, a nested WITH or beside a parenthesised branch", + sql: "WITH c AS (WITH d AS (SELECT 1 AS x) SELECT @sql.a(x) AS one FROM d), e AS (SELECT * FROM (SELECT @sql.b(y) AS two FROM t) AS s) SELECT @sql.c(z) AS three FROM c UNION ALL (SELECT 'x' FROM e)", + expected_names: &[], }, OutputNamesTestCase { description: "a call in the main query after CTEs", sql: "WITH c AS (SELECT x FROM t) SELECT x, @sql.a(x) AS one FROM c", - expected_names: &[(None, "one")], + expected_names: &[(None, 0, "one")], }, ]; for test_case in test_cases { - let names: Vec<(Option, String)> = sql_macro_output_names(test_case.sql); - let found: Vec<(Option<&str>, &str)> = names + let names: Vec<(Option, usize, String)> = sql_macro_output_names(test_case.sql); + let found: Vec<(Option<&str>, usize, &str)> = names .iter() - .map(|(cte, name)| (cte.as_deref(), name.as_str())) + .map(|(cte, branch, name)| (cte.as_deref(), *branch, name.as_str())) .collect(); assert_eq!(found, test_case.expected_names, "{}", test_case.description); diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/test_types.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/test_types.rs index 9e484a48cb..47970c78e9 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests/test_types.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/test_types.rs @@ -67,8 +67,13 @@ pub(super) struct AsciiArguments; pub(super) struct OutputTypeTestCase { pub(super) description: &'static str, pub(super) check: crate::sql_macros::models::SqlMacroCheck, - /// The item's CTE (`None` for the main query), name and inferred type. - pub(super) item: (Option<&'static str>, &'static str, Option<&'static str>), + /// The item's CTE (`None` for the main query), set-operation branch, name and inferred type. + pub(super) item: ( + Option<&'static str>, + usize, + &'static str, + Option<&'static str>, + ), pub(super) expected_type: Option<&'static str>, } @@ -76,5 +81,5 @@ pub(super) struct OutputTypeTestCase { pub(super) struct OutputNamesTestCase { pub(super) description: &'static str, pub(super) sql: &'static str, - pub(super) expected_names: &'static [(Option<&'static str>, &'static str)], + pub(super) expected_names: &'static [(Option<&'static str>, usize, &'static str)], } diff --git a/crates/sqlbuild-rules/src/rules/_helpers/explicit_output_types.rs b/crates/sqlbuild-rules/src/rules/_helpers/explicit_output_types.rs index 4dbc541cd7..d760285923 100644 --- a/crates/sqlbuild-rules/src/rules/_helpers/explicit_output_types.rs +++ b/crates/sqlbuild-rules/src/rules/_helpers/explicit_output_types.rs @@ -17,6 +17,8 @@ struct OutputScope<'a> { by_name: bool, /// Where SQL macro call items are read: the main query (`Some(None)`), a CTE, or nowhere. macro_scope: Option>, + /// The first select's branch of the scope's set operation, left to right. + branch: usize, } struct OutputRule<'a> { @@ -25,7 +27,7 @@ struct OutputRule<'a> { rule: &'a RuleMetadata, faults: &'a FaultCollector, /// The main query's outputs that are one SQL macro call each, read when first needed. - sql_macro_outputs: OnceCell, String)>>, + sql_macro_outputs: OnceCell, usize, String)>>, } pub(crate) fn evaluate(parsed: &ParsedModel<'_>, rule: &RuleMetadata, faults: &FaultCollector) { @@ -63,6 +65,7 @@ pub(crate) fn evaluate(parsed: &ParsedModel<'_>, rule: &RuleMetadata, faults: &F set_branch: false, by_name: false, macro_scope: final_cte.map(Some), + branch: 0, }, ); return; @@ -75,6 +78,7 @@ pub(crate) fn evaluate(parsed: &ParsedModel<'_>, rule: &RuleMetadata, faults: &F set_branch: false, by_name: false, macro_scope: Some(None), + branch: 0, }, ); } @@ -138,10 +142,17 @@ impl OutputRule<'_> { columns: scope.columns, set_branch: true, by_name: *by_name, - macro_scope: None, + macro_scope: scope.macro_scope, + branch: scope.branch, }; self.evaluate_set(left, branch_scope); - self.evaluate_set(right, branch_scope); + self.evaluate_set( + right, + OutputScope { + branch: scope.branch + set_branches(left), + ..branch_scope + }, + ); } SetFacts::Other => self.push( None, @@ -202,7 +213,7 @@ impl OutputRule<'_> { || column.type_proven && direct && !scope.set_branch || column.type_proven && !explicit_cast - && self.sql_macro_output(scope.macro_scope, output_name.as_deref()); + && self.sql_macro_output(scope, output_name.as_deref()); if passes { continue; } @@ -230,17 +241,18 @@ impl OutputRule<'_> { } } - /// Whether output `name` at `scope` is one SQL macro call, typed by its `returns`. - fn sql_macro_output(&self, scope: Option>, name: Option<&str>) -> bool { - let (Some(scope), Some(name)) = (scope, name) else { + /// Whether output `name` in `scope`'s select is one SQL macro call, typed by its `returns`. + fn sql_macro_output(&self, scope: OutputScope<'_>, name: Option<&str>) -> bool { + let (Some(macro_scope), Some(name)) = (scope.macro_scope, name) else { return false; }; self.sql_macro_outputs .get_or_init(|| sql_macro_output_names(&self.model.authored_sql)) .iter() - .any(|(cte, output)| { + .any(|(cte, branch, output)| { output.eq_ignore_ascii_case(name) - && match (cte.as_deref(), scope) { + && *branch == scope.branch + && match (cte.as_deref(), macro_scope) { (None, None) => true, (Some(cte), Some(scope)) => cte.eq_ignore_ascii_case(scope), _ => false, @@ -262,6 +274,14 @@ impl OutputRule<'_> { } } +/// The selects of `set_expr`'s set operation, a nested query counting as one. +fn set_branches(set_expr: &SetFacts) -> usize { + match set_expr { + SetFacts::SetOperation { left, right, .. } => set_branches(left) + set_branches(right), + SetFacts::Select(_) | SetFacts::Query(_) | SetFacts::Other => 1, + } +} + fn proven_dependency_wildcard(select: &SelectFacts, columns: &[Column]) -> bool { plain_select(select, true) && reads_dependency(select) diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md index c66d4f00f3..5ccc5999da 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/macros/sql-macros.md @@ -279,12 +279,16 @@ macro file. A select-list item that is one expression macro call under an alias, as in `@sql.cents_to_dollars(o.amount_cents) AS amount`, has the macro's `returns` type wherever the -compiler infers no type for the expanded SQL, or exactly that type. This holds in the main query and in any top-level CTE, and the type flows on as -an inferred type would: through `SELECT * FROM final`, pass-through columns and joins, to contract -checks (`K002`, `K003`), `SQBRCONTRACT105` and models that read the column. Such a call needs no -`CAST` to satisfy a matching contract column. Any other inferred type stands, even a -close one such as `VARCHAR(100)` for `returns VARCHAR(5)`, and `K002` reports the mismatch. A call inside a larger expression, a subquery or a -`UNION` branch is typed from the expanded SQL as before. +compiler infers no type for the expanded SQL, or exactly that type. This holds in the main query +and in any top-level CTE, including each branch of their `UNION`, `INTERSECT` or `EXCEPT`, and the +type flows on as an inferred type would: through `SELECT * FROM final`, pass-through columns and +joins, to contract checks (`K002`, `K003`), `SQBRCONTRACT105` and models that read the column. A +set operation's column combines its branches as usual, so a sibling branch of another type still +leaves it unproven, and a CTE holding one is read by column name, since `SELECT *` over it is not +typed. Such a call needs no `CAST` to satisfy a matching contract column. Any other +inferred type stands, even a close one such as `VARCHAR(100)` for `returns VARCHAR(5)`, and `K002` +reports the mismatch. A call inside a larger expression, a subquery, a nested `WITH` or a set +operation with a parenthesised branch is typed from the expanded SQL as before. - `sqb format` formats SQL macro files, bodies of every form and the `MACRO` header, and normalises the commas around CTE-list and column-list calls. diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py index c2720a6bba..ce08bb1ffb 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py @@ -56,6 +56,23 @@ ), } +_UNION_CALLS: dict[str, str] = { + **_TYPED_RULES_AND_MACROS, + **contract_model( + "orders", + "order_id (type BIGINT), status_bar (type VARCHAR)", + "WITH final AS (\n" + " SELECT CAST(o.order_id AS BIGINT) AS order_id,\n" + " @sql.status_bar(o.status) AS status_bar\n" + ' FROM __ref("stg_orders") AS o\n' + " UNION ALL\n" + " SELECT CAST(o.order_id AS BIGINT) AS order_id,\n" + " CAST(NULL AS VARCHAR) AS status_bar\n" + ' FROM __ref("stg_orders") AS o\n' + ")\nSELECT f.order_id, f.status_bar FROM final AS f\n", + ), +} + @pytest.mark.parametrize( "test_case", @@ -345,6 +362,47 @@ def test_given_an_any_argument_misused_when_compiling_then_it_fails( expected_returncode=0, expected_fragments=("0 errors",), ), + SqlMacroCompileTestCase( + description="a_call_in_a_union_branch_of_a_cte_types_the_union", + files=_UNION_CALLS, + expected_returncode=0, + expected_fragments=("0 errors, 0 warnings",), + unexpected_fragments=_UNPROVEN, + ), + SqlMacroCompileTestCase( + description="a_call_in_a_later_union_branch_of_the_main_query_types_the_union", + files={ + **_TYPED_RULES_AND_MACROS, + **contract_model( + "orders", + "status_bar (type VARCHAR)", + 'SELECT CAST(NULL AS VARCHAR) AS status_bar FROM __ref("stg_orders") AS o\n' + "UNION ALL\n" + 'SELECT @sql.status_bar(o.status) AS status_bar FROM __ref("stg_orders") AS o\n', + ), + }, + expected_returncode=0, + expected_fragments=("0 errors, 0 warnings",), + unexpected_fragments=_UNPROVEN, + ), + SqlMacroCompileTestCase( + description="a_union_sibling_of_another_known_type_still_reports", + files={ + **_TYPED_RULES_AND_MACROS, + **contract_model( + "orders", + "status_bar (type VARCHAR)", + "WITH final AS (\n" + ' SELECT @sql.status_bar(o.status) AS status_bar FROM __ref("stg_orders") AS o\n' + " UNION ALL\n" + " SELECT CAST(LENGTH(o.status) AS BIGINT) AS status_bar\n" + ' FROM __ref("stg_orders") AS o\n' + ")\nSELECT f.status_bar FROM final AS f\n", + ), + }, + expected_returncode=1, + expected_fragments=('error[SQBRCONTRACT105]: output "status_bar"',), + ), SqlMacroCompileTestCase( description="an_inferred_type_of_another_family_still_disagrees_through_a_cte", files={ @@ -387,6 +445,13 @@ def test_given_a_returns_type_when_checking_contracts_then_the_call_has_that_typ expected_returncode=0, expected_fragments=("0 errors, 0 warnings",), ), + SqlMacroCommandTestCase( + description="a_warm_compile_types_a_union_branch_call_the_same_way", + files=_UNION_CALLS, + command=("compile",), + expected_returncode=0, + expected_fragments=("0 errors, 0 warnings",), + ), ], ids=lambda case: case.description, ) diff --git a/website/src/content/docs/docs/concepts/macros/sql-macros.mdx b/website/src/content/docs/docs/concepts/macros/sql-macros.mdx index 7d71f106a9..c88fbe3f48 100644 --- a/website/src/content/docs/docs/concepts/macros/sql-macros.mdx +++ b/website/src/content/docs/docs/concepts/macros/sql-macros.mdx @@ -267,12 +267,16 @@ macro file. A select-list item that is one expression macro call under an alias, as in `@sql.cents_to_dollars(o.amount_cents) AS amount`, has the macro's `returns` type wherever the -compiler infers no type for the expanded SQL, or exactly that type. This holds in the main query and in any top-level CTE, and the type flows on as -an inferred type would: through `SELECT * FROM final`, pass-through columns and joins, to contract -checks (`K002`, `K003`), `SQBRCONTRACT105` and models that read the column. Such a call needs no -`CAST` to satisfy a matching contract column. Any other inferred type stands, even a -close one such as `VARCHAR(100)` for `returns VARCHAR(5)`, and `K002` reports the mismatch. A call inside a larger expression, a subquery or a -`UNION` branch is typed from the expanded SQL as before. +compiler infers no type for the expanded SQL, or exactly that type. This holds in the main query +and in any top-level CTE, including each branch of their `UNION`, `INTERSECT` or `EXCEPT`, and the +type flows on as an inferred type would: through `SELECT * FROM final`, pass-through columns and +joins, to contract checks (`K002`, `K003`), `SQBRCONTRACT105` and models that read the column. A +set operation's column combines its branches as usual, so a sibling branch of another type still +leaves it unproven, and a CTE holding one is read by column name, since `SELECT *` over it is not +typed. Such a call needs no `CAST` to satisfy a matching contract column. Any other +inferred type stands, even a close one such as `VARCHAR(100)` for `returns VARCHAR(5)`, and `K002` +reports the mismatch. A call inside a larger expression, a subquery, a nested `WITH` or a set +operation with a parenthesised branch is typed from the expanded SQL as before. - `sqb format` formats SQL macro files, bodies of every form and the `MACRO` header, and normalises the commas around CTE-list and column-list calls. From f98670f9c331e654b4eaccfe54b31fe75f285e98 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 23 Oct 2026 20:54:49 +0100 Subject: [PATCH 09/11] fix: treat the UNKNOWN placeholder type as unknown when typing SQL macro calls --- .../sql_macros/main/sql_macro_declared_type.rs | 5 ++++- .../src/sql_macros/tests/test_output_types.rs | 6 ++++++ .../compile/sql_macros/test_sql_macro_types.py | 18 ++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs index e61523fcf7..2e7aa03ef5 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/main/sql_macro_declared_type.rs @@ -1,3 +1,4 @@ +use crate::constants::UNKNOWN_SQL_TYPE; use crate::sql_macros::models::DeclaredOutput; use crate::type_system::main::normalize_type::normalize_type; use crate::type_system::models::NormalizedType; @@ -19,7 +20,9 @@ pub fn sql_macro_declared_type( _ => false, } })?; - let Some(inferred) = inferred.filter(|inferred| !inferred.is_empty()) else { + let Some(inferred) = inferred.filter(|inferred| { + !inferred.is_empty() && !inferred.eq_ignore_ascii_case(UNKNOWN_SQL_TYPE) + }) else { return Some(output.returns.clone()); }; let Some(inferred_type) = normalized(inferred, dialect) else { diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs index 119262fc15..cec6611d74 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs @@ -32,6 +32,12 @@ fn given_a_typed_macro_output_when_typing_the_column_then_returns_fills_unknown_ item: (None, 0, "label", Some("VARCHAR(100)")), expected_type: None, }, + OutputTypeTestCase { + description: "the unknown placeholder takes returns", + check: typed_check(CallScope::Main, 0), + item: (None, 0, "label", Some("UNKNOWN")), + expected_type: Some("VARCHAR"), + }, OutputTypeTestCase { description: "a known time type is not unknown", check: typed_check(CallScope::Main, 0), diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py index ce08bb1ffb..63e5a48f58 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py @@ -403,6 +403,24 @@ def test_given_an_any_argument_misused_when_compiling_then_it_fails( expected_returncode=1, expected_fragments=('error[SQBRCONTRACT105]: output "status_bar"',), ), + SqlMacroCompileTestCase( + description="an_untyped_upstream_column_in_a_cte_takes_returns", + files={ + **with_rules("SQBRCONTRACT105"), + **text_macro("m", "BIGINT", "@value"), + "models/marts/up.sql": ( + 'MODEL (description "Up");\n\nSELECT NULL AS s FROM __ref("stg_orders") AS o\n' + ), + **contract_model( + "orders", + "v (type BIGINT)", + 'WITH c AS (SELECT @sql.m(u.s) AS v FROM __ref("up") AS u)\nSELECT v FROM c\n', + ), + }, + expected_returncode=0, + expected_fragments=("0 errors, 0 warnings",), + unexpected_fragments=_UNPROVEN, + ), SqlMacroCompileTestCase( description="an_inferred_type_of_another_family_still_disagrees_through_a_cte", files={ From 5fdcef105ba1734c95038a455157f573e9c6121c Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 23 Oct 2026 20:54:49 +0100 Subject: [PATCH 10/11] fix: type SQL macro calls in any main-query set-operation branch like the first --- .../analysis_session/_helpers/cte_facts.rs | 59 +++++++++++++++---- .../sql_macros/test_sql_macro_types.py | 17 ++++++ 2 files changed, 64 insertions(+), 12 deletions(-) diff --git a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/cte_facts.rs b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/cte_facts.rs index fe242b06df..74685b243c 100644 --- a/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/cte_facts.rs +++ b/crates/sqlbuild-analysis/src/assembly/analysis_session/_helpers/cte_facts.rs @@ -153,12 +153,7 @@ pub(crate) fn declared_output_types( branches: Vec::new(), } .query_types(&parsed, &facts)?; - let main: HashSet = profile - .declared - .iter() - .filter(|output| output.cte.is_none() && output.branch == 0) - .map(|output| casefold(&output.name)) - .collect(); + let main: HashSet = main_output_columns(&parsed, profile.declared); Ok(declared .into_iter() .filter(|(name, data_type)| { @@ -175,28 +170,68 @@ fn declared_branches(root: &Expression) -> Vec { && unwrap_annotations(Some(body)).is_some_and(|body| top_level_ctes(body).is_empty()) { for (branch, select) in set_branches(Some(body)).into_iter().enumerate() { - branches.push((select, Some(cte_name.to_owned()), branch)); + branches.push(( + std::ptr::from_ref(select), + Some(cte_name.to_owned()), + branch, + )); } } } for (branch, select) in set_branches(Some(root)).into_iter().enumerate() { - branches.push((select, None, branch)); + branches.push((std::ptr::from_ref(select), None, branch)); } branches } -/// The addresses of the selects of `node`'s set operation, left to right, or of `node` itself. -fn set_branches(node: Option<&Expression>) -> Vec<*const Expression> { +/// The folded main-query output columns its SQL macro call items stand in, in any branch. +fn main_output_columns(root: &Expression, declared: &[DeclaredOutput]) -> HashSet { + let selects: Vec<&Expression> = set_branches(Some(root)); + let by_name: bool = set_operation(root).is_some_and(|(_, _, by_name)| by_name); + let Some(first) = selects.first() else { + return HashSet::new(); + }; + let first_names: Vec<&str> = projection_names(first); + let mut columns: HashSet = HashSet::new(); + for output in declared.iter().filter(|output| output.cte.is_none()) { + if by_name { + columns.insert(casefold(&output.name)); + continue; + } + let Some(select) = selects.get(output.branch) else { + continue; + }; + let position: Option = projection_names(select) + .iter() + .position(|name| name.eq_ignore_ascii_case(&output.name)); + if let Some(column) = position.and_then(|position| first_names.get(position)) { + columns.insert(casefold(column)); + } + } + columns +} + +/// The output name of each of `select`'s projections, in order. +fn projection_names(select: &Expression) -> Vec<&str> { + select + .get_expressions() + .iter() + .map(|projection| output_name(unwrap_annotations(Some(projection)))) + .collect() +} + +/// The selects of `node`'s set operation, left to right, or `node` itself. +fn set_branches(node: Option<&Expression>) -> Vec<&Expression> { let Some(node) = unwrap_annotations(node) else { return Vec::new(); }; match set_operation(node) { Some((left, right, _)) => { - let mut branches: Vec<*const Expression> = set_branches(Some(left)); + let mut branches: Vec<&Expression> = set_branches(Some(left)); branches.extend(set_branches(Some(right))); branches } - None => vec![std::ptr::from_ref(node)], + None => vec![node], } } diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py index 63e5a48f58..9620ef8e76 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/compile/sql_macros/test_sql_macro_types.py @@ -421,6 +421,23 @@ def test_given_an_any_argument_misused_when_compiling_then_it_fails( expected_fragments=("0 errors, 0 warnings",), unexpected_fragments=_UNPROVEN, ), + SqlMacroCompileTestCase( + description="a_call_in_a_later_main_query_branch_matching_its_sibling_is_typed", + files={ + **with_rules("SQBRCONTRACT105"), + **text_macro("m", "BIGINT", "NULL"), + **contract_model( + "orders", + "v (type BIGINT)", + 'SELECT CAST(NULL AS BIGINT) AS v FROM __ref("stg_orders") AS o\n' + "UNION ALL\n" + 'SELECT @sql.m(o.status) AS v FROM __ref("stg_orders") AS o\n', + ), + }, + expected_returncode=0, + expected_fragments=("0 errors, 0 warnings",), + unexpected_fragments=_UNPROVEN, + ), SqlMacroCompileTestCase( description="an_inferred_type_of_another_family_still_disagrees_through_a_cte", files={ From 4abcbf8292453fcd672a9e8b87bc806123f6d073 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 23 Oct 2026 20:54:49 +0100 Subject: [PATCH 11/11] fix: do not count a star EXCEPT modifier as a set operation when numbering SQL macro branches --- .../src/sql_macros/_helpers/placement.rs | 9 ++++++++- .../src/sql_macros/tests/test_output_types.rs | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/crates/sqlbuild-analysis/src/sql_macros/_helpers/placement.rs b/crates/sqlbuild-analysis/src/sql_macros/_helpers/placement.rs index a94ea344f3..9991907baf 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/_helpers/placement.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/_helpers/placement.rs @@ -11,6 +11,8 @@ const DISTINCT_ON_TOKENS: usize = 2; const QUERY_OPENERS: [&str; 4] = ["SELECT", "WITH", "VALUES", "FROM"]; /// Keywords that combine the selects of a query. const SET_OPERATIONS: [&str; 3] = ["UNION", "INTERSECT", "EXCEPT"]; +/// A star, which an `EXCEPT` column list may follow as a modifier, not a set operation. +const STAR: &str = "*"; /// The words that may stand between a set operation and its parenthesised right branch. const SET_OPERATION_MODIFIERS: [&str; 3] = ["ALL", "DISTINCT", "NAME"]; /// Tokens from `@` to a macro call's name: `@`, the kind, `.` and the name. @@ -666,7 +668,12 @@ impl QueryParts { } } if depth == 0 { - if token.kind == TokenKind::Word && is_set_operation(token.text(text)) { + let star_modifier: bool = + previous_at_top.is_some_and(|previous| previous.text(text) == STAR); + if token.kind == TokenKind::Word + && is_set_operation(token.text(text)) + && !star_modifier + { branch += 1; } previous_at_top = Some(token); diff --git a/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs b/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs index cec6611d74..9861f19e11 100644 --- a/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs +++ b/crates/sqlbuild-analysis/src/sql_macros/tests/test_output_types.rs @@ -114,6 +114,11 @@ fn given_model_sql_when_reading_sql_macro_outputs_then_only_whole_aliased_items_ sql: "WITH c AS (SELECT @sql.a(x) AS one FROM t) SELECT @sql.b(y) AS two FROM c UNION ALL SELECT 'x' FROM c UNION ALL SELECT @sql.c(z) AS three FROM c", expected_names: &[(Some("c"), 0, "one"), (None, 0, "two"), (None, 2, "three")], }, + OutputNamesTestCase { + description: "a star EXCEPT modifier is not a set operation", + sql: "SELECT * EXCEPT (a) FROM t UNION ALL SELECT @sql.b(y) AS two FROM t", + expected_names: &[(None, 1, "two")], + }, OutputNamesTestCase { description: "calls in subqueries, a nested WITH or beside a parenthesised branch", sql: "WITH c AS (WITH d AS (SELECT 1 AS x) SELECT @sql.a(x) AS one FROM d), e AS (SELECT * FROM (SELECT @sql.b(y) AS two FROM t) AS s) SELECT @sql.c(z) AS three FROM c UNION ALL (SELECT 'x' FROM e)",