Skip to content

Commit e160823

Browse files
authored
fix(compiler)!: reject unknown config keys and keep scenario SQL as authored (#906)
* fix(compiler)!: reject unknown MODEL, model-default and source keys MODEL() headers, [defaults], [path_defaults.*] and sources/*.yml silently ignored unknown keys, so a typo or a removed setting had no effect. Every statement header (MODEL, FUNCTION, AUDIT, TEST, HOOK, SCENARIO, @udf) now shares one unsupported-keys error that names the key with its file and line and suggests the nearest supported key. Custom materialization values stay inside the free-form config block. BREAKING CHANGE: unknown keys in MODEL() headers, project defaults, path defaults and sources files now fail compilation. * perf(compiler): keep per-diagnostic work linear in diagnostic-heavy models One model with thousands of diagnostics compiled in quadratic time: locating each unknown column rescanned the authored SQL natively, and diagnostic recovery and explanation rebuilt per-model column sets, lineage lists, line splits and binary-expression scans for every diagnostic. Index each once per model and look positions up by bisect or hash. A one-model scaling guard compares 1k and 4k diagnostics. * fix(format): keep comma spacing before adjacent string literals When the layout oracle rewrote a substituted region, for example DuckDB 'north' 'east' printed as CONCAT('north', 'east'), the token after the region took the oracle's spacing after the CONCAT( it introduced, so `replace, 'north'` became `replace,'north'` and lost its line break. The spacing now comes from the oracle's own copy of the authored token that precedes it. * fix(dbt): apply sqb build plan safety policies on dbt run and build sqb dbt run and build enforced only the missing-origin and retention decrease policies before executing SQLBuild work, so a table_type_downgrade, snapshot full-refresh or max_models policy could be bypassed through the dbt interop path. Both commands now share the sqb build gate sequence. sqb dbt has no allow flags: a policy that needs confirmation prompts on a terminal and otherwise refuses with guidance naming the matching sqb build flag or policy setting. * fix(scenario): run scenario fixture and check SQL as authored Scenario expected, assertion, helper and fixture SQL was parsed and regenerated with the generic dialect to swap relation markers, so dialect syntax was rewritten or rejected (S506) and the executed SQL differed from what was authored. Markers are now replaced in the authored text, skipping comments and quoted text under the project adapter's lexical rules, also for --local replay: backslash escapes on Snowflake, BigQuery and Databricks, E'...' strings on DuckDB and Postgres, nested block comments where the dialect nests them, and BigQuery '#' and Snowflake '//' line comments. A marker the scenario cannot resolve, or SQL with an unclosed string or comment, fails planning with S511 naming the scenario file instead of being left in the executed SQL. Local replay no longer regenerates SQL captured from DuckDB.
1 parent 44c2d30 commit e160823

107 files changed

Lines changed: 3256 additions & 719 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
@@ -116,6 +116,7 @@ E2E_DUCKDB_BUILD_INCREMENTAL_PATHS := \
116116
E2E_DUCKDB_CLI_DATA_PATHS := \
117117
tests/e2e/src/sqlbuild/cli/commands/main/load \
118118
tests/e2e/src/sqlbuild/cli/commands/main/providers \
119+
tests/e2e/src/sqlbuild/cli/commands/main/scenario/test_authored_check_sql.py \
119120
tests/e2e/src/sqlbuild/cli/commands/main/scenario/test_scenario.py \
120121
tests/e2e/src/sqlbuild/cli/commands/main/scenario/test_capture.py \
121122
tests/e2e/src/sqlbuild/cli/commands/main/scenario/test_namespace.py

‎crates/sqlbuild-rules-native/src/compiler/_helpers/model_headers/tokenization.rs‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,10 @@ fn tokenize(header: &str) -> Result<Vec<HeaderToken>, String> {
438438
index += 1;
439439
continue;
440440
}
441+
if starts_comment(&characters, index) {
442+
index = comment_end(&characters, index)?;
443+
continue;
444+
}
441445
if starts_template(&characters, index) {
442446
let template_end = find_character(&characters, '}', index + 2)
443447
.ok_or_else(|| format!("unterminated template value at position {index}"))?;
@@ -470,7 +474,11 @@ fn tokenize(header: &str) -> Result<Vec<HeaderToken>, String> {
470474
continue;
471475
}
472476
let next = characters[index];
473-
if is_python_whitespace(next) || is_symbol(next) || next == ':' {
477+
if is_python_whitespace(next)
478+
|| is_symbol(next)
479+
|| next == ':'
480+
|| starts_comment(&characters, index)
481+
{
474482
break;
475483
}
476484
if matches!(next, '\'' | '"') {
@@ -518,6 +526,27 @@ fn read_quoted_string(characters: &[char], start: usize) -> Result<(String, usiz
518526
))
519527
}
520528

529+
fn starts_comment(characters: &[char], index: usize) -> bool {
530+
matches!(
531+
(characters.get(index), characters.get(index + 1)),
532+
(Some('-'), Some('-')) | (Some('/'), Some('*'))
533+
)
534+
}
535+
536+
fn comment_end(characters: &[char], start: usize) -> Result<usize, String> {
537+
if characters[start] == '-' {
538+
return Ok(find_character(characters, '\n', start).map_or(characters.len(), |end| end + 1));
539+
}
540+
let mut index = start + 2;
541+
while index + 1 < characters.len() {
542+
if characters[index] == '*' && characters[index + 1] == '/' {
543+
return Ok(index + 2);
544+
}
545+
index += 1;
546+
}
547+
Err(format!("unterminated block comment at position {start}"))
548+
}
549+
521550
fn starts_template(characters: &[char], index: usize) -> bool {
522551
characters.get(index) == Some(&'$') && characters.get(index + 1) == Some(&'{')
523552
}

‎crates/sqlbuild-rules-native/src/compiler/tests/helpers.rs‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,47 @@ pub(crate) fn invalid_headers_return_each_exact_error_in_order() -> bool {
144144
true
145145
}
146146

147+
pub(crate) fn header_comments_are_skipped_without_shifting_offsets() -> bool {
148+
let headers = vec![
149+
"materialized table -- keep model metadata\n columns (id ()) /* it's, (fine) */"
150+
.to_owned(),
151+
"materialized table--trailing note\n".to_owned(),
152+
"materialized table /* unclosed".to_owned(),
153+
];
154+
155+
let results = parse_batch(&headers).expect("worker pool builds");
156+
157+
let AuthoredValue::Map(values) = results[0].0.as_ref().expect("header values") else {
158+
panic!("header values must be a map");
159+
};
160+
assert_eq!(
161+
values
162+
.iter()
163+
.map(|(key, _)| key.as_str())
164+
.collect::<Vec<_>>(),
165+
vec!["materialized", "columns"]
166+
);
167+
assert_eq!(
168+
results[0].1.as_ref().expect("column offsets"),
169+
&vec![("id".to_owned(), 53, 2)]
170+
);
171+
let AuthoredValue::Map(values) = results[1].0.as_ref().expect("header values") else {
172+
panic!("header values must be a map");
173+
};
174+
assert_eq!(
175+
values
176+
.iter()
177+
.map(|(key, _)| key.as_str())
178+
.collect::<Vec<_>>(),
179+
vec!["materialized"]
180+
);
181+
assert_eq!(
182+
results[2].2.as_deref(),
183+
Some("unterminated block comment at position 19")
184+
);
185+
true
186+
}
187+
147188
pub(crate) fn nested_and_root_columns_return_only_root_offsets() -> bool {
148189
let headers = vec![
149190
"config (columns (nested (type INTEGER))), columns (top (type INTEGER))".to_owned(),

‎crates/sqlbuild-rules-native/src/compiler/tests/test_model_header_tokenization.rs‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use crate::compiler::tests::helpers::{
2-
batch_sizes_bound_workers_by_contract, invalid_headers_return_each_exact_error_in_order,
2+
batch_sizes_bound_workers_by_contract, header_comments_are_skipped_without_shifting_offsets,
3+
invalid_headers_return_each_exact_error_in_order,
34
nested_and_root_columns_return_only_root_offsets,
45
nested_authored_headers_preserve_values_and_offsets,
56
};
@@ -18,6 +19,11 @@ fn given_model_header_cases_when_exercising_native_parser_then_expected_behavior
1819
run: invalid_headers_return_each_exact_error_in_order,
1920
expected_success: true,
2021
},
22+
ModelHeaderTokenizationTestCase {
23+
description: "line and block comments are skipped without shifting offsets",
24+
run: header_comments_are_skipped_without_shifting_offsets,
25+
expected_success: true,
26+
},
2127
ModelHeaderTokenizationTestCase {
2228
description: "nested columns do not leak into root offsets",
2329
run: nested_and_root_columns_return_only_root_offsets,

‎crates/sqlbuild-rules-native/src/semantic_validation/_helpers/positions.rs‎

Lines changed: 51 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use pyo3::exceptions::PyValueError;
88
use pyo3::{PyResult, pymethods};
99
use regex::Regex;
1010
use std::collections::HashMap;
11-
use std::sync::LazyLock;
11+
use std::sync::{LazyLock, OnceLock};
1212

1313
const POPULAR_TOKEN_THRESHOLD: usize = 200;
1414
static TOKENS: LazyLock<Result<Regex, String>> =
@@ -42,6 +42,7 @@ impl BindingPositions {
4242
authored,
4343
offsets,
4444
passes,
45+
words: OnceLock::new(),
4546
})
4647
})
4748
}
@@ -59,21 +60,8 @@ impl BindingPositions {
5960
.captures(message)
6061
{
6162
let identifier = captures[1].rsplit('.').next().unwrap_or(&captures[1]);
62-
let mut occurrences =
63-
self.authored
64-
.match_indices(identifier)
65-
.filter(|(offset, _)| {
66-
let before = self.authored[..*offset].chars().next_back();
67-
let after = self.authored[*offset + identifier.len()..].chars().next();
68-
!before.is_some_and(word) && !after.is_some_and(word)
69-
});
70-
if let Some((offset, _)) = occurrences.next()
71-
&& occurrences.next().is_none()
72-
{
73-
return Ok(position(
74-
&self.lines,
75-
self.authored[..offset].chars().count(),
76-
));
63+
if let Some(offset) = self.unique_word_offset(identifier) {
64+
return Ok(position(&self.lines, offset));
7765
}
7866
}
7967
let Some(query_start) = self.query_start else {
@@ -116,6 +104,53 @@ impl BindingPositions {
116104
}
117105
}
118106

107+
impl BindingPositions {
108+
/// Char offset of the only whole-word occurrence of `identifier` in the authored SQL.
109+
pub(crate) fn unique_word_offset(&self, identifier: &str) -> Option<usize> {
110+
if identifier.is_empty() || !identifier.chars().all(word) {
111+
let mut occurrences = self
112+
.authored
113+
.match_indices(identifier)
114+
.filter(|(offset, _)| {
115+
let before = self.authored[..*offset].chars().next_back();
116+
let after = self.authored[*offset + identifier.len()..].chars().next();
117+
!before.is_some_and(word) && !after.is_some_and(word)
118+
});
119+
let (offset, _) = occurrences.next()?;
120+
return occurrences
121+
.next()
122+
.is_none()
123+
.then(|| self.authored[..offset].chars().count());
124+
}
125+
let (offset, unique) = *self
126+
.words
127+
.get_or_init(|| word_occurrences(&self.authored))
128+
.get(identifier)?;
129+
unique.then_some(offset)
130+
}
131+
}
132+
133+
/// Index every maximal word run once so identifier lookups do not rescan the SQL.
134+
fn word_occurrences(sql: &str) -> HashMap<String, (usize, bool)> {
135+
let mut occurrences: HashMap<String, (usize, bool)> = HashMap::new();
136+
let mut current = String::new();
137+
let mut start = 0;
138+
for (index, character) in sql.chars().chain(std::iter::once(' ')).enumerate() {
139+
if word(character) {
140+
if current.is_empty() {
141+
start = index;
142+
}
143+
current.push(character);
144+
} else if !current.is_empty() {
145+
occurrences
146+
.entry(std::mem::take(&mut current))
147+
.and_modify(|entry| entry.1 = false)
148+
.or_insert((start, true));
149+
}
150+
}
151+
occurrences
152+
}
153+
119154
fn word(character: char) -> bool {
120155
character == '_' || character.is_alphanumeric()
121156
}

‎crates/sqlbuild-rules-native/src/semantic_validation/models.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ pub(crate) struct BindingPositions {
6565
pub(super) cleaned_lines: Vec<usize>,
6666
pub(super) offsets: Vec<usize>,
6767
pub(super) passes: Vec<Vec<Expansion>>,
68+
pub(super) words: std::sync::OnceLock<HashMap<String, (usize, bool)>>,
6869
}
6970

7071
impl std::fmt::Debug for ProjectCatalog {

‎crates/sqlbuild-rules-native/src/semantic_validation/tests.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
#[path = "tests/test_function_names.rs"]
22
mod function_names;
3+
#[path = "tests/test_positions.rs"]
4+
mod positions;
35
#[path = "tests/test_semantic_validation.rs"]
46
mod semantic_validation;
57
#[path = "tests/test_types.rs"]
Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
use crate::semantic_validation::models::BindingPositions;
2+
use crate::semantic_validation::tests::test_types::{
3+
UniqueWordOffsetTestCase, UniqueWordScalingTestCase,
4+
};
5+
use std::sync::OnceLock;
6+
use std::time::{Duration, Instant};
7+
8+
#[test]
9+
fn given_identifier_when_locating_unique_word_then_returns_its_char_offset() {
10+
let test_cases = [
11+
UniqueWordOffsetTestCase {
12+
description: "single whole-word occurrence",
13+
authored: "SELECT o.order_id, o.amount FROM orders AS o",
14+
identifier: "amount",
15+
expected_offset: Some(21),
16+
},
17+
UniqueWordOffsetTestCase {
18+
description: "occurrence inside a longer word is not a match",
19+
authored: "SELECT order_id_v2, order_id FROM orders",
20+
identifier: "order_id",
21+
expected_offset: Some(20),
22+
},
23+
UniqueWordOffsetTestCase {
24+
description: "repeated word is ambiguous",
25+
authored: "SELECT amount FROM orders WHERE amount > 0",
26+
identifier: "amount",
27+
expected_offset: None,
28+
},
29+
UniqueWordOffsetTestCase {
30+
description: "absent word",
31+
authored: "SELECT amount FROM orders",
32+
identifier: "quantity",
33+
expected_offset: None,
34+
},
35+
UniqueWordOffsetTestCase {
36+
description: "offsets count characters after non-ASCII text",
37+
authored: "SELECT 'café' AS label, total FROM orders",
38+
identifier: "total",
39+
expected_offset: Some(24),
40+
},
41+
UniqueWordOffsetTestCase {
42+
description: "identifier with non-word characters falls back to a substring scan",
43+
authored: "SELECT \"order total\" FROM orders",
44+
identifier: "order total",
45+
expected_offset: Some(8),
46+
},
47+
];
48+
49+
for test_case in test_cases {
50+
let located = BindingPositions {
51+
authored: test_case.authored.to_owned(),
52+
query_start: Some(0),
53+
lines: vec![0],
54+
cleaned_lines: vec![0],
55+
offsets: Vec::new(),
56+
passes: Vec::new(),
57+
words: OnceLock::new(),
58+
};
59+
for lookup in ["first", "cached"] {
60+
assert_eq!(
61+
located.unique_word_offset(test_case.identifier),
62+
test_case.expected_offset,
63+
"{} ({lookup} lookup)",
64+
test_case.description
65+
);
66+
}
67+
}
68+
}
69+
70+
#[test]
71+
fn given_many_unique_identifiers_when_locating_each_then_work_stays_linear() {
72+
let test_cases = [UniqueWordScalingTestCase {
73+
description: "one wide query with an unknown column per projection",
74+
identifiers: 20_000,
75+
expected_max_duration: Duration::from_secs(2),
76+
}];
77+
78+
for test_case in test_cases {
79+
let located = BindingPositions {
80+
authored: (0..test_case.identifiers)
81+
.map(|index| format!("missing_{index}"))
82+
.collect::<Vec<_>>()
83+
.join(",\n"),
84+
query_start: Some(0),
85+
lines: vec![0],
86+
cleaned_lines: vec![0],
87+
offsets: Vec::new(),
88+
passes: Vec::new(),
89+
words: OnceLock::new(),
90+
};
91+
let started = Instant::now();
92+
let located_count = (0..test_case.identifiers)
93+
.filter(|index| {
94+
located
95+
.unique_word_offset(&format!("missing_{index}"))
96+
.is_some()
97+
})
98+
.count();
99+
assert_eq!(
100+
located_count, test_case.identifiers,
101+
"{}",
102+
test_case.description
103+
);
104+
assert!(
105+
started.elapsed() < test_case.expected_max_duration,
106+
"{}: took {:?}",
107+
test_case.description,
108+
started.elapsed()
109+
);
110+
}
111+
}

‎crates/sqlbuild-rules-native/src/semantic_validation/tests/test_types.rs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,3 +24,16 @@ pub(crate) struct LocatedFunctionDiagnosticTestCase {
2424
pub(crate) expected_location: (Option<usize>, Option<usize>),
2525
pub(crate) expected_span: (Option<usize>, Option<usize>),
2626
}
27+
28+
pub(crate) struct UniqueWordOffsetTestCase {
29+
pub(crate) description: &'static str,
30+
pub(crate) authored: &'static str,
31+
pub(crate) identifier: &'static str,
32+
pub(crate) expected_offset: Option<usize>,
33+
}
34+
35+
pub(crate) struct UniqueWordScalingTestCase {
36+
pub(crate) description: &'static str,
37+
pub(crate) identifiers: usize,
38+
pub(crate) expected_max_duration: std::time::Duration,
39+
}

‎crates/sqlbuild-rules-native/src/sql_lint/_helpers/token_layout.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -554,7 +554,10 @@ fn separators(
554554
" ".to_string()
555555
};
556556
separators[next] = if substitution {
557-
let oracle_gap = gap(next_at - 1, next_at);
557+
let oracle_gap = (previous_at + 1..next_at)
558+
.rev()
559+
.find(|&at| generated[at].key == authored[last].key)
560+
.map_or_else(|| gap(next_at - 1, next_at), |at| gap(at, at + 1));
558561
let unspaced = default_space(&authored[last], &authored[next]).is_empty();
559562
if unspaced && !oracle_gap.contains('\n') {
560563
String::new()

0 commit comments

Comments
 (0)