Skip to content

Commit 920a933

Browse files
authored
fix: align SQL scanners and remove obsolete implementations (#815)
* fix(sql-lint): honour backtick identifiers for the project dialect SQL lint's interpolation-site lexer treated backticks as ordinary code, so in BigQuery and Databricks projects a close paren inside a backtick identifier ended a macro call early, and an apostrophe inside a backtick identifier hid every later macro site. `sqb format` then rewrote the hidden macro (for example `@label(...)` became `@LABEL(...)`), breaking compile. Lint now derives backtick quoting from the configured lint dialect using the same policy rules use, in both the native preparation pass and the Python fallback used for formatting and non-ASCII bodies. Postgres and SQL Server keep treating backticks as code; backslash escapes are unchanged. * fix(rules): skip comments in the generic parser fallback normalisation When a model's dialect parser rejected it, the generic fallback rewrote `:` paths and `->` lambdas while skipping only quoted text. An apostrophe in a `--` or `/* */` comment (for example `-- don't`) opened a phantom quote that never closed, so normalisation stopped and the fallback parse failed. The fallback now skips comments and quotes through the shared scanner. * fix(sql-tests): share the expected-CTE UNION splitter with the Polyglot fallback When Polyglot could not parse an expected CTE, the fallback split set operations with its own scanner: it ignored comments (an apostrophe in `-- don't` hid every later branch, and `UNION` inside a comment split), left `DISTINCT` at the start of a `UNION DISTINCT` branch so the branch parse raised a raw parser error, and split on INTERSECT/EXCEPT even though the parsed path rejects them. Both the fallback and the textual expected-projection path now use one shared `split_union_branches` over the comment- and quote-aware `iter_code_positions`, handling `UNION [ALL|DISTINCT]`. The fallback rejects top-level INTERSECT/EXCEPT with the parsed path's error, and defers to the textual path when a split branch still cannot be parsed. * fix(sql-tests): merge textual assertion CTEs into a leading WITH The textual assertion fallback prefixed lifted CTEs as `WITH ... {body}` even when the assertion body began with its own `WITH`, producing `WITH a AS (...) WITH b AS (...) SELECT ...`. That resolved SQL is what the renderer emits when an assertion's lifted CTEs conflict with earlier steps. The fallback now uses the shared `with_leading_ctes`, merging lifted CTEs into the body's leading WITH clause like every other planning path. * refactor(native): share one top-level code scan in the SQL-test extractor `split_unions`, `split_top_level` and `find_top_level_keyword` each repeated the same loop around the shared scanner: skip comments and quotes, track parenthesis depth, then act on top-level code. They now run on a single `scan_code` walker whose visitor advances, resumes after a consumed keyword, or stops with a result. Characterisation tests pin union splits, comma splits, keyword offsets, multibyte text, unbalanced parentheses and unclosed-input errors; results are unchanged. * fix(identity): include declared loader columns in version identity Loader version identity hashed the destination, write strategy, cursor, unique key and contract but not the declared `columns`, so editing a loader's column declarations (for example through a module-level column list) left its recorded version unchanged. Loaders now include `columns` in their identity decorator config, matching assets. Retry and connection mode remain excluded for every kind. Upgrading gives one round of new versions for loaders with declared columns: their first run after the upgrade records a new version hash. * fix(sql-tests): support INTERSECT and EXCEPT in expected CTEs and check every branch Expected CTEs now accept UNION, INTERSECT and EXCEPT, each with an optional ALL or DISTINCT, in model, macro, UDF and table-function tests, and every branch's projection names and count must match the first branch. - Native extractor: `split_unions` becomes `split_set_operations`, so an INTERSECT or EXCEPT branch is validated instead of being folded into the first branch; `* EXCEPT (...)` star modifiers are not treated as operators. - Polyglot path: Intersect and Except nodes are flattened like Union instead of being rejected as non-SELECT branches. - Shared Python fallback: `split_union_branches` becomes `split_set_operation_branches` and no longer rejects INTERSECT/EXCEPT. - Model-test expected-CTE errors now name the actual CTE (for example `__expected__orders`) instead of `__expected__<model>`, matching direct-logic tests; the message wording is otherwise unchanged. * refactor(compiler): remove unused code * refactor(cli): remove unused code * refactor(executor): remove unused code * refactor(virtual): remove unused code * refactor(runtime): remove unused code * refactor(integrations): remove unused code * refactor(adapters): remove unused code * refactor(core): remove unused code * refactor(tests): remove orphaned test case types * refactor(compiler): remove no-op cursor alias deprecation shim * refactor(compiler): remove unused legacy hook key copies * refactor(adapters): remove unused adapter contract methods Drop render_query_with_cursor_bounds, render_seed_select_after_cursor, get_relation_max_cursor and count_rows from BaseAdapter, StrictAdapter, DiffMixin, DuckDbBackedAdapter and every first-class adapter, plus PromotionStrategy.CREATE_NEW. No engine code called them. Tests that used count_rows for verification use a small COUNT(*) helper; the unsupported promotion strategy test now uses the catalogued but unhandled build_aside strategy. * refactor(executor): remove unwired same-target audit gate reuse decision same_target_audit_gate_reuse_decision was never called by the executor. Remove it, the BINDING_SET_CHANGED reason only it produced, and its tests. The invalid-metadata diagnostic test now exercises the live reuse_from decision, which shares the parsing path. * refactor(compiler): remove test-only header parser and state backend methods Remove the Python _ModelHeaderParser, which only served as a cross-check oracle for the native MODEL header parser, together with its corpus parity test. Remove the StateBackend methods replace_virtual_environment_function_refs, replace_virtual_environment_seed_refs and get_physical_relation_ancestry, which only tests called; tests now write refs through the node-ref APIs production uses. No state-table layout, stored format or reader used by the engine changes. append_microbatch_event is kept: retargeting its Postgres tests to append_microbatch_events exposed a type-resolution failure in the bulk path that needs a separate fix. * refactor(execution-history): remove deprecated EventLogStorage alias This removes a deprecated public alias: sqlbuild.execution_history.EventLogStorage is gone. Use LifecycleEventLogStorage, which it pointed to. docs/execution-history.md is updated accordingly. * refactor(lint): remove unused code Remove the unused fix-run machinery (FixRunResult, LintFixRecord, fix planning helpers and their CLI renderers), other unreferenced lint helpers and constants, and the test-only map_neutralized_offset. LINT_ENGINE_SQLBUILD is kept and now used by native.py instead of the duplicated "sqlbuild" literal. * refactor(rule-engine): remove unused code Remove unreferenced SQL helper modules (decision signatures, model-name grammar, passthrough detection), table_parts, ModelNameParts and unused constants. The public custom-rule API is unchanged: SqlDocument.subqueries and RuleOption.integer_list stay. * refactor(compiler): remove unused SQL analysis, scenario and SQL-test code Remove unused Polyglot analysis constants, the scenario _dedupe_names helper and the never-emitted S505 scenario code, the unused _LEGACY_MODEL_HOOK_KEYS copy in SQL-test attachment, and the SQL-test CTE SQLite cache module, which has had no production caller since #758. * refactor(python-nodes): remove unused code Remove the unused loader-to-node conversion module, the placeholder DiscoveredPython{Task,Asset,Check}Node classes, PythonSqlRunLifecyclePlan.python_phase, and fields that were set but never read: PythonNodeIdentity.object_module/object_qualname, PythonNodeDependencyEdge.upstream_function/downstream_function and PythonSqlRunLifecyclePlan.read_side_sql_keys. The identity definition payload and hashes are unchanged. * refactor(executor): remove unread microbatch applied intervals ModelExecutionResult.microbatch_applied_intervals and MicrobatchPhaseOutcome.applied_intervals were accumulated for every batch but never read. Remove the fields and their bookkeeping; execution behaviour is unchanged. * refactor(virtual): share ref model-version lookup virtual diff and promote each carried an identical private _read_model_versions. Both now use one read_ref_model_versions entry in virtual/state/main/environments. * refactor(planner): rename stale SchemaColumnSource.SQLGLOT member sqlglot was removed as a runtime dependency; the member is now SQL_ANALYSIS. Only the Python name changes: the enum value stays "sql_analysis", so nothing serialised or displayed changes. * fix(state): type bulk microbatch event inserts on Postgres * refactor(state): remove single microbatch event append * test(ci): run Python node identity regression in build-core shard
1 parent afa88d4 commit 920a933

249 files changed

Lines changed: 1889 additions & 8593 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎Makefile‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ E2E_DUCKDB_BUILD_CORE_PATHS := \
7373
tests/e2e/src/sqlbuild/cli/commands/main/build/test_no_tests_no_audits_flags.py \
7474
tests/e2e/src/sqlbuild/cli/commands/main/build/test_plan_command_surface.py \
7575
tests/e2e/src/sqlbuild/cli/commands/main/build/test_python_hooks.py \
76+
tests/e2e/src/sqlbuild/cli/commands/main/build/test_python_node_identity.py \
7677
tests/e2e/src/sqlbuild/cli/commands/main/build/test_sql_hook_identity.py \
7778
tests/e2e/src/sqlbuild/cli/commands/main/build/test_query_change_tracking.py \
7879
tests/e2e/src/sqlbuild/cli/commands/main/build/test_query_propagation.py \

‎crates/sqlbuild-rules-native/src/bindings/_helpers/functions.rs‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use pyo3::prelude::{
55
Bound, IntoPyObject, Py, PyAny, PyErr, PyModule, PyModuleMethods, PyResult, Python,
66
};
77
use pyo3::types::{PyDict, PyDictMethods, PyList, PyTuple};
8-
use pyo3::{pyfunction, wrap_pyfunction};
8+
use pyo3::{FromPyObject, pyfunction, wrap_pyfunction};
99

1010
use crate::configuration::main::load;
1111
use crate::constants::API_VERSION;
@@ -38,19 +38,37 @@ fn finalize_rule_findings_json(py: Python<'_>, request_json: &str) -> PyResult<S
3838
.map_err(value_error)
3939
}
4040

41+
/// One SQL lint preparation request, read from a Python mapping.
42+
#[derive(FromPyObject)]
43+
#[pyo3(from_item_all)]
44+
struct LintPreparationRequest {
45+
expanded: String,
46+
before_expansion: String,
47+
prior_sites: Vec<usize>,
48+
dialect: String,
49+
}
50+
4151
#[pyfunction]
4252
fn prepare_lint_sql(
4353
py: Python<'_>,
44-
expanded: &str,
45-
before_expansion: &str,
46-
prior_sites: Vec<usize>,
54+
request: LintPreparationRequest,
4755
) -> PyResult<Option<crate::sql_lint::types::PreparedSql>> {
4856
py.detach(|| {
49-
crate::sql_lint::main::preparation::prepare(expanded, before_expansion, &prior_sites)
57+
crate::sql_lint::main::preparation::prepare(
58+
&request.expanded,
59+
&request.before_expansion,
60+
&request.prior_sites,
61+
&request.dialect,
62+
)
5063
})
5164
.map_err(value_error)
5265
}
5366

67+
#[pyfunction]
68+
fn lint_backtick_identifiers(dialect: &str) -> bool {
69+
crate::sql_lint::main::backtick_identifiers::backtick_identifiers(dialect)
70+
}
71+
5472
#[pyfunction]
5573
fn lint_sql_batch_json(py: Python<'_>, request_json: &str) -> PyResult<String> {
5674
py.detach(|| crate::sql_lint::main::batch_engine::lint_batch_json(request_json))
@@ -325,6 +343,7 @@ pub(crate) fn register(module: &Bound<'_, PyModule>) -> PyResult<()> {
325343
module.add_function(wrap_pyfunction!(finalize_rule_findings_json, module)?)?;
326344
module.add_function(wrap_pyfunction!(lint_sql_json, module)?)?;
327345
module.add_function(wrap_pyfunction!(prepare_lint_sql, module)?)?;
346+
module.add_function(wrap_pyfunction!(lint_backtick_identifiers, module)?)?;
328347
module.add_function(wrap_pyfunction!(lint_sql_batch_json, module)?)?;
329348
module.add_function(wrap_pyfunction!(format_sql_json, module)?)?;
330349
module.add_function(wrap_pyfunction!(format_sql_batch_json, module)?)?;

‎crates/sqlbuild-rules-native/src/compiler/_helpers/sql_tests/extraction.rs‎

Lines changed: 83 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,8 @@ fn classify_model(ctes: Vec<Cte>, file: &str) -> Result<Classified, String> {
202202
authored.push(cte);
203203
} else if let Some(value) = name.strip_prefix("__expected__") {
204204
expected_models.push(required(value, "__expected__<model>", file)?);
205-
validate_expected(&cte, file, "__expected__<model>", true)?;
205+
let label = cte.0.clone();
206+
validate_expected(&cte, file, &label, true)?;
206207
expected.push(cte);
207208
} else if let Some(value) = name.strip_prefix("__assert__") {
208209
assertion_names.push(required(value, "__assert__<assertion>", file)?);
@@ -431,7 +432,7 @@ fn validate_expected(
431432
"SQL test '{file}' must not use SELECT * in {label} CTEs"
432433
));
433434
}
434-
let branches = split_unions(&cte.1)?;
435+
let branches = split_set_operations(&cte.1)?;
435436
let mut names: Vec<Vec<String>> = Vec::new();
436437
for branch in branches {
437438
names.push(projection_names(branch, file, label)?);
@@ -844,10 +845,20 @@ fn contains_select_star(sql: &str) -> Result<bool, String> {
844845
Ok(false)
845846
}
846847

847-
fn split_unions(sql: &str) -> Result<Vec<&str>, String> {
848-
let mut values: Vec<&str> = Vec::new();
849-
let mut start = 0;
850-
let mut index = 0;
848+
/// What a top-level scan does after visiting one code offset.
849+
enum CodeStep<T> {
850+
Advance,
851+
Resume(usize),
852+
Stop(T),
853+
}
854+
855+
/// Visit each code offset outside quotes and comments with its depth after that byte's parenthesis.
856+
fn scan_code<T>(
857+
sql: &str,
858+
start: usize,
859+
mut visit: impl FnMut(usize, isize) -> Result<CodeStep<T>, String>,
860+
) -> Result<Option<T>, String> {
861+
let mut index = start;
851862
let mut depth = 0;
852863
while index < sql.len() {
853864
let next = skip_non_code(sql, index)?;
@@ -860,83 +871,85 @@ fn split_unions(sql: &str) -> Result<Vec<&str>, String> {
860871
Some(b')') => depth -= 1,
861872
_ => {}
862873
}
863-
if depth == 0
864-
&& let Some(end) = consume_keyword(sql, index, "UNION")
865-
{
866-
let value = sql[start..index].trim();
867-
if !value.is_empty() {
868-
values.push(value);
869-
}
870-
index = skip_ignorable(sql, end)?;
871-
if let Some(quantifier_end) = consume_keyword(sql, index, "ALL")
872-
.or_else(|| consume_keyword(sql, index, "DISTINCT"))
873-
{
874-
index = skip_ignorable(sql, quantifier_end)?;
875-
}
876-
start = index;
877-
continue;
874+
match visit(index, depth)? {
875+
CodeStep::Advance => index += char_len(sql, index),
876+
CodeStep::Resume(resume) => index = resume,
877+
CodeStep::Stop(value) => return Ok(Some(value)),
878878
}
879-
index += char_len(sql, index);
880879
}
881-
let value = sql[start..].trim();
882-
if !value.is_empty() {
883-
values.push(value);
884-
}
885-
Ok(values)
880+
Ok(None)
881+
}
882+
883+
fn non_empty_trimmed(value: &str) -> Option<&str> {
884+
Some(value.trim()).filter(|trimmed| !trimmed.is_empty())
886885
}
887886

888-
fn split_top_level(sql: &str, separator: u8) -> Result<Vec<&str>, String> {
887+
/// Split on top-level `UNION`, `INTERSECT` and `EXCEPT`, each with an optional `ALL`/`DISTINCT`.
888+
pub(crate) fn split_set_operations(sql: &str) -> Result<Vec<&str>, String> {
889889
let mut values: Vec<&str> = Vec::new();
890890
let mut start = 0;
891-
let mut index = 0;
892-
let mut depth = 0;
893-
while index < sql.len() {
894-
let next = skip_non_code(sql, index)?;
895-
if next != index {
896-
index = next;
897-
continue;
891+
scan_code::<()>(sql, 0, |index, depth| {
892+
if depth != 0 {
893+
return Ok(CodeStep::Advance);
898894
}
899-
match byte_at(sql, index) {
900-
Some(b'(') => depth += 1,
901-
Some(b')') => depth -= 1,
902-
Some(value) if value == separator && depth == 0 => {
903-
let item = sql[start..index].trim();
904-
if !item.is_empty() {
905-
values.push(item);
906-
}
907-
start = index + 1;
908-
}
909-
_ => {}
895+
let Some(end) = set_operator_end(sql, index)? else {
896+
return Ok(CodeStep::Advance);
897+
};
898+
values.extend(non_empty_trimmed(&sql[start..index]));
899+
let mut resume = skip_ignorable(sql, end)?;
900+
if let Some(quantifier_end) =
901+
consume_keyword(sql, resume, "ALL").or_else(|| consume_keyword(sql, resume, "DISTINCT"))
902+
{
903+
resume = skip_ignorable(sql, quantifier_end)?;
910904
}
911-
index += char_len(sql, index);
905+
start = resume;
906+
Ok(CodeStep::Resume(resume))
907+
})?;
908+
values.extend(non_empty_trimmed(&sql[start..]));
909+
Ok(values)
910+
}
911+
912+
/// Return the end of a set operator at `index`; `* EXCEPT (...)` star modifiers are not operators.
913+
fn set_operator_end(sql: &str, index: usize) -> Result<Option<usize>, String> {
914+
if let Some(end) =
915+
consume_keyword(sql, index, "UNION").or_else(|| consume_keyword(sql, index, "INTERSECT"))
916+
{
917+
return Ok(Some(end));
912918
}
913-
let item = sql[start..].trim();
914-
if !item.is_empty() {
915-
values.push(item);
919+
match consume_keyword(sql, index, "EXCEPT") {
920+
Some(end) if previous_code_byte(sql, index)? != Some(b'*') => Ok(Some(end)),
921+
_ => Ok(None),
916922
}
917-
Ok(values)
918923
}
919924

920-
fn find_top_level_keyword(sql: &str, start: usize, keyword: &str) -> Result<Option<usize>, String> {
921-
let mut index = start;
922-
let mut depth = 0;
923-
while index < sql.len() {
924-
let next = skip_non_code(sql, index)?;
925-
if next != index {
926-
index = next;
927-
continue;
928-
}
929-
match byte_at(sql, index) {
930-
Some(b'(') => depth += 1,
931-
Some(b')') => depth -= 1,
932-
_ => {}
933-
}
934-
if depth == 0 && consume_keyword(sql, index, keyword).is_some() {
935-
return Ok(Some(index));
925+
pub(crate) fn split_top_level(sql: &str, separator: u8) -> Result<Vec<&str>, String> {
926+
let mut values: Vec<&str> = Vec::new();
927+
let mut start = 0;
928+
scan_code::<()>(sql, 0, |index, depth| {
929+
if depth == 0 && byte_at(sql, index) == Some(separator) {
930+
values.extend(non_empty_trimmed(&sql[start..index]));
931+
start = index + 1;
936932
}
937-
index += char_len(sql, index);
938-
}
939-
Ok(None)
933+
Ok(CodeStep::Advance)
934+
})?;
935+
values.extend(non_empty_trimmed(&sql[start..]));
936+
Ok(values)
937+
}
938+
939+
pub(crate) fn find_top_level_keyword(
940+
sql: &str,
941+
start: usize,
942+
keyword: &str,
943+
) -> Result<Option<usize>, String> {
944+
scan_code(sql, start, |index, depth| {
945+
Ok(
946+
if depth == 0 && consume_keyword(sql, index, keyword).is_some() {
947+
CodeStep::Stop(index)
948+
} else {
949+
CodeStep::Advance
950+
},
951+
)
952+
})
940953
}
941954

942955
fn find_top_level_clause_keyword(

‎crates/sqlbuild-rules-native/src/compiler/_helpers/sql_tests/planning.rs‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1056,15 +1056,7 @@ fn resolve_assertion_textual_sql(
10561056
);
10571057
let body_sql = resolution.sql;
10581058
let reached = resolution.reached;
1059-
let resolved_sql = if lifted_ctes.is_empty() {
1060-
body_sql.clone()
1061-
} else {
1062-
let cte_parts: Vec<String> = lifted_ctes
1063-
.iter()
1064-
.map(|(name, body)| cte_definition_sql(name, body))
1065-
.collect();
1066-
format!("WITH {} {body_sql}", cte_parts.join(", "))
1067-
};
1059+
let resolved_sql = with_leading_ctes(&lifted_ctes, &body_sql);
10681060
Ok((
10691061
TextualStep {
10701062
resolved_sql,

0 commit comments

Comments
 (0)