Skip to content

Commit aff8a45

Browse files
committed
fix: keep authored SQL tokens when formatting
sqb format regenerated SQL from the parse tree, so any spelling the SQL library canonicalised was written back into the user's file. In Snowflake that renamed STARTSWITH to STARTS_WITH, which Snowflake rejects, and also rewrote SUBSTR, TIMESTAMPDIFF and TRY_TO_DECIMAL to synonyms. The formatter's syntax-tree check could not see this because both spellings parse to the same node. After generating the layout, walk the authored and generated token streams together and write the authored text of every aligned token into the new layout. Only whitespace, the case of unquoted words, an added AS, and dropped optional terminators may differ; any other structural difference refuses the format and leaves the file unchanged. The same check runs on the idempotence pass. On a 1,337-file Snowflake project this removes all 82 function renames, with no new refusals.
1 parent f03daee commit aff8a45

6 files changed

Lines changed: 276 additions & 2 deletions

File tree

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

Lines changed: 99 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ use crate::sql_lint::models::{FormatRequest, FormatResponse};
1414

1515
const COMMENT_ATTACHMENT_FAILURE: &str =
1616
"native formatter could not preserve comment token attachments";
17+
const TOKEN_PRESERVATION_FAILURE: &str =
18+
"native formatter would change authored SQL tokens, not only layout";
1719
const UNSUPPORTED_SQL_FAILURE: &str =
1820
"native formatter could not safely restore literal or cast spelling";
1921

@@ -80,7 +82,11 @@ fn try_format_sql(request: FormatRequest) -> Result<FormatResponse, String> {
8082
original_tokens: &semantic_tokens,
8183
comments: &comments,
8284
};
83-
let formatted = format_once(&neutral, &formatted_context)?;
85+
let formatted = preserve_authored_tokens(
86+
&neutral,
87+
format_once(&neutral, &formatted_context)?,
88+
&dialect,
89+
)?;
8490
let formatted_tokens = dialect
8591
.tokenize(&formatted)
8692
.map_err(|error| error.to_string())?;
@@ -103,14 +109,105 @@ fn try_format_sql(request: FormatRequest) -> Result<FormatResponse, String> {
103109
original_tokens: &second_semantic_tokens,
104110
comments: &second_comments,
105111
};
106-
let second_pass = format_once(&second_neutral, &second_context)?;
112+
let second_pass = preserve_authored_tokens(
113+
&second_neutral,
114+
format_once(&second_neutral, &second_context)?,
115+
&dialect,
116+
)?;
107117
if second_pass != formatted {
108118
return Err("native formatter output is not idempotent".to_string());
109119
}
110120
let changed = formatted != original;
111121
response(formatted, changed, true, None)
112122
}
113123

124+
/// Restores authored token text into generated layout, refusing any structural token change.
125+
pub(crate) fn preserve_authored_tokens(
126+
authored_sql: &str,
127+
mut formatted: String,
128+
dialect: &Dialect,
129+
) -> Result<String, String> {
130+
let authored = dialect
131+
.tokenize(authored_sql)
132+
.map_err(|error| error.to_string())?;
133+
let generated = dialect
134+
.tokenize(&formatted)
135+
.map_err(|error| error.to_string())?;
136+
let mut replacements: Vec<(usize, usize, String)> = Vec::new();
137+
let mut authored_index = 0;
138+
let mut generated_index = 0;
139+
while authored_index < authored.len() || generated_index < generated.len() {
140+
let before = authored.get(authored_index);
141+
let after = generated.get(generated_index);
142+
match (before, after) {
143+
(before, Some(after))
144+
if after.token_type == TokenType::As
145+
&& before.is_none_or(|token| token.token_type != TokenType::As) =>
146+
{
147+
generated_index += 1;
148+
}
149+
(Some(before), Some(after)) if tokens_align(before, after) => {
150+
let authored_text = raw_token_text(authored_sql, before)?;
151+
let generated_text = raw_token_text(&formatted, after)?;
152+
if authored_text != generated_text
153+
&& !(is_unquoted_word(&authored_text)
154+
&& authored_text.eq_ignore_ascii_case(&generated_text))
155+
{
156+
replacements.push((after.span.start, after.span.end, authored_text));
157+
}
158+
authored_index += 1;
159+
generated_index += 1;
160+
}
161+
(Some(before), after) if is_optional_terminator(&authored, authored_index, before) => {
162+
if after.is_some_and(|token| token.token_type == before.token_type) {
163+
generated_index += 1;
164+
}
165+
authored_index += 1;
166+
}
167+
(None, Some(after)) if after.token_type == TokenType::Semicolon => {
168+
generated_index += 1;
169+
}
170+
_ => return Err(TOKEN_PRESERVATION_FAILURE.to_string()),
171+
}
172+
}
173+
for (start, end, text) in replacements.into_iter().rev() {
174+
let start = char_to_byte(&formatted, start)?;
175+
let end = char_to_byte(&formatted, end)?;
176+
formatted.replace_range(start..end, &text);
177+
}
178+
Ok(formatted)
179+
}
180+
181+
fn tokens_align(before: &Token, after: &Token) -> bool {
182+
before.token_type == after.token_type || (is_word_token(before) && is_word_token(after))
183+
}
184+
185+
fn is_word_token(token: &Token) -> bool {
186+
is_unquoted_word(&token.text)
187+
}
188+
189+
fn is_unquoted_word(text: &str) -> bool {
190+
let mut characters = text.chars();
191+
characters
192+
.next()
193+
.is_some_and(|first| first.is_ascii_alphabetic() || first == '_')
194+
&& characters
195+
.all(|character| character.is_ascii_alphanumeric() || matches!(character, '_' | '$'))
196+
}
197+
198+
fn is_optional_terminator(tokens: &[Token], index: usize, token: &Token) -> bool {
199+
token.token_type == TokenType::Semicolon
200+
|| (token.token_type == TokenType::Comma
201+
&& tokens
202+
.get(index + 1)
203+
.is_some_and(|next| next.token_type == TokenType::From))
204+
}
205+
206+
fn raw_token_text(sql: &str, token: &Token) -> Result<String, String> {
207+
char_slice(sql, token.span.start, token.span.end)
208+
.ok_or_else(|| TOKEN_PRESERVATION_FAILURE.to_string())
209+
}
210+
114211
#[derive(Debug, Clone)]
115212
struct Comment {
116213
start: usize,

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
#[path = "tests/test_authored_tokens.rs"]
2+
mod authored_tokens;
13
#[path = "tests/test_batch_lint.rs"]
24
mod batch_lint;
35
#[path = "tests/test_comment_formatter.rs"]
Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
use serde_json::{Value, json};
2+
3+
use crate::sql_lint::_helpers::formatter::preserve_authored_tokens;
4+
use crate::sql_lint::main::formatter::format_json;
5+
use crate::sql_lint::tests::test_types;
6+
7+
#[test]
8+
fn given_generator_respellings_when_formatting_then_authored_spellings_are_kept()
9+
-> Result<(), String> {
10+
let test_cases = [
11+
test_types::FormatTestCase {
12+
description: "Snowflake STARTSWITH keeps its authored name",
13+
sql: "select startswith(order_code, 'EU') as is_eu_order from orders",
14+
expected_sql: "SELECT\n startswith(order_code, 'EU') AS is_eu_order\nFROM orders",
15+
expected_changed: true,
16+
},
17+
test_types::FormatTestCase {
18+
description: "function synonyms keep their authored names",
19+
sql: "select SUBSTR(order_code, 1, 2) as region, TIMESTAMPDIFF(day, ordered_at, shipped_at) as days_to_ship, TRY_TO_DECIMAL(amount_text, 10, 2) as amount from orders",
20+
expected_sql: "SELECT\n SUBSTR(order_code, 1, 2) AS region,\n TIMESTAMPDIFF(DAY, ordered_at, shipped_at) AS days_to_ship,\n TRY_TO_DECIMAL(amount_text, 10, 2) AS amount\nFROM orders",
21+
expected_changed: true,
22+
},
23+
test_types::FormatTestCase {
24+
description: "operator and function spellings stay authored",
25+
sql: "select IFNULL(status, 'open') as status from orders where quantity != 0",
26+
expected_sql: "SELECT\n IFNULL(status, 'open') AS status\nFROM orders\nWHERE\n quantity != 0",
27+
expected_changed: true,
28+
},
29+
];
30+
for test_case in &test_cases {
31+
let response = format_json(
32+
&json!({"version": 1, "sql": test_case.sql, "dialect": "snowflake"}).to_string(),
33+
)?;
34+
let payload: Value = serde_json::from_str(&response).map_err(|error| error.to_string())?;
35+
assert_eq!(
36+
payload["sql"], test_case.expected_sql,
37+
"{}",
38+
test_case.description
39+
);
40+
assert_eq!(
41+
payload["changed"], test_case.expected_changed,
42+
"{}",
43+
test_case.description
44+
);
45+
}
46+
Ok(())
47+
}
48+
49+
#[test]
50+
fn given_generated_token_changes_when_preserving_authored_tokens_then_only_layout_survives() {
51+
let dialect = polyglot_sql::Dialect::get(polyglot_sql::DialectType::Snowflake);
52+
let test_cases = [
53+
test_types::AuthoredTokenTestCase {
54+
description: "renamed function is restored to its authored spelling",
55+
authored: "select startswith(code, 'EU') from orders",
56+
generated: "SELECT\n STARTS_WITH(code, 'EU')\nFROM orders",
57+
expected_sql: Some("SELECT\n startswith(code, 'EU')\nFROM orders"),
58+
},
59+
test_types::AuthoredTokenTestCase {
60+
description: "swapped arguments are restored to authored order",
61+
authored: "select datediff(day, a, b) from orders",
62+
generated: "SELECT\n DATEDIFF(DAY, b, a)\nFROM orders",
63+
expected_sql: Some("SELECT\n DATEDIFF(DAY, a, b)\nFROM orders"),
64+
},
65+
test_types::AuthoredTokenTestCase {
66+
description: "an explicit alias keyword may be added",
67+
authored: "select a b from orders",
68+
generated: "SELECT\n a AS b\nFROM orders",
69+
expected_sql: Some("SELECT\n a AS b\nFROM orders"),
70+
},
71+
test_types::AuthoredTokenTestCase {
72+
description: "a structural change refuses the format",
73+
authored: "select a from orders where a <> 1",
74+
generated: "SELECT\n a\nFROM orders\nWHERE\n NOT a = 1",
75+
expected_sql: None,
76+
},
77+
test_types::AuthoredTokenTestCase {
78+
description: "a dropped token refuses the format",
79+
authored: "select a from orders left outer join items using (id)",
80+
generated: "SELECT\n a\nFROM orders\nLEFT JOIN items USING (id)",
81+
expected_sql: None,
82+
},
83+
];
84+
for test_case in test_cases {
85+
let actual = preserve_authored_tokens(
86+
test_case.authored,
87+
test_case.generated.to_string(),
88+
&dialect,
89+
);
90+
assert_eq!(
91+
actual.ok().as_deref(),
92+
test_case.expected_sql,
93+
"{}",
94+
test_case.description
95+
);
96+
}
97+
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,3 +104,11 @@ pub(crate) struct FunctionDepthFailureTestCase {
104104
pub expected_code: &'static str,
105105
pub expected_limit: &'static str,
106106
}
107+
108+
pub(crate) struct AuthoredTokenTestCase {
109+
pub description: &'static str,
110+
pub authored: &'static str,
111+
pub generated: &'static str,
112+
/// `None` when the formatter must refuse the change.
113+
pub expected_sql: Option<&'static str>,
114+
}

‎tests/integration/src/sqlbuild/cli/commands/main/_test_types.py‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,16 @@ class FormatCompileIntegrationTestCase:
149149
expected_literal: str
150150

151151

152+
@dataclass(frozen=True)
153+
class AuthoredSpellingFormatIntegrationTestCase:
154+
"""One authored function or operator spelling that formatting must keep."""
155+
156+
description: str
157+
adapter: str
158+
authored_expression: str
159+
expected_expression: str
160+
161+
152162
@dataclass(frozen=True)
153163
class BacktickDialectFormatIntegrationTestCase:
154164
"""One backtick-identifier adapter whose macro call must survive formatting."""

‎tests/integration/src/sqlbuild/cli/commands/main/test_format.py‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
from sqlbuild.cli.commands.main.entrypoint.entry import main
1212
from tests.integration.src.sqlbuild.cli.commands.main._test_types import (
13+
AuthoredSpellingFormatIntegrationTestCase,
1314
BacktickDialectFormatIntegrationTestCase,
1415
CanonicalFixtureFormatIntegrationTestCase,
1516
DescriptionFormatIntegrationTestCase,
@@ -32,6 +33,65 @@
3233
_UNFORMATTED_ORDERS_SQL: str = "MODEL (materialized table);\nselect 1 as order_id\n"
3334

3435

36+
@pytest.mark.parametrize(
37+
"test_case",
38+
[
39+
AuthoredSpellingFormatIntegrationTestCase(
40+
"Snowflake STARTSWITH is not renamed",
41+
"snowflake",
42+
"STARTSWITH(order_code, 'EU')",
43+
"STARTSWITH(order_code, 'EU')",
44+
),
45+
AuthoredSpellingFormatIntegrationTestCase(
46+
"Snowflake SUBSTR synonym is kept",
47+
"snowflake",
48+
"SUBSTR(order_code, 1, 2)",
49+
"SUBSTR(order_code, 1, 2)",
50+
),
51+
AuthoredSpellingFormatIntegrationTestCase(
52+
"Snowflake TIMESTAMPDIFF synonym is kept",
53+
"snowflake",
54+
"TIMESTAMPDIFF(day, ordered_at, shipped_at)",
55+
"TIMESTAMPDIFF(DAY, ordered_at, shipped_at)",
56+
),
57+
AuthoredSpellingFormatIntegrationTestCase(
58+
"Snowflake TRY_TO_DECIMAL synonym is kept",
59+
"snowflake",
60+
"TRY_TO_DECIMAL(amount_text, 10, 2)",
61+
"TRY_TO_DECIMAL(amount_text, 10, 2)",
62+
),
63+
AuthoredSpellingFormatIntegrationTestCase(
64+
"DuckDB IFNULL and != spellings are kept",
65+
"duckdb",
66+
"IFNULL(status, 'open') != 'x'",
67+
"IFNULL(status, 'open') != 'x'",
68+
),
69+
],
70+
ids=lambda case: case.description,
71+
)
72+
def test_given_authored_function_spelling_when_formatting_then_only_layout_changes(
73+
test_case: AuthoredSpellingFormatIntegrationTestCase,
74+
tmp_path: Path,
75+
capsys: pytest.CaptureFixture[str],
76+
) -> None:
77+
"""Formatting must never rewrite a function name, including to one the warehouse rejects."""
78+
(tmp_path / "sqlbuild_project.toml").write_text(
79+
f'name = "orders"\nadapter = "{test_case.adapter}"\n'
80+
)
81+
model: Path = tmp_path / "models" / "orders.sql"
82+
model.parent.mkdir()
83+
model.write_text(
84+
'MODEL (description "Order flags");\n'
85+
f'select {test_case.authored_expression} as order_flag from __source("raw_orders")\n'
86+
)
87+
assert main(["--project-dir", str(tmp_path), "format", "--json"]) == 0
88+
capsys.readouterr()
89+
formatted: str = model.read_text()
90+
assert f" {test_case.expected_expression} AS order_flag\n" in formatted
91+
assert main(["--project-dir", str(tmp_path), "format", "--check", "--json"]) == 0
92+
assert model.read_text() == formatted
93+
94+
3595
@pytest.mark.parametrize(
3696
"test_case",
3797
[

0 commit comments

Comments
 (0)